Re: [PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Jonathan Nieder <jrnieder@gmail.com>]

Michael J Gruber wrote:

> Based on mg/rev-list-one-side-only (in next) to save Junio a build conflict
> resolution.

Not a serious problem, but I wish it hadn't been.  What particular
functionality from that branch does this use?

Ah, now that I check it seems that that is to change a use of
no_merges in the implementation of --cherry to use the new API?  Makes
sense (and good catch).  With that hunk skipped, the patches apply to
master.

> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -72,11 +72,24 @@ endif::git-rev-list[]
>  
>  --merges::
>  
> -	Print only merge commits.
> +	Print only merge commits. This is equivalent to `--min-parents=2`.
>  
>  --no-merges::
>  
> -	Do not print commits with more than one parent.
> +	Do not print commits with more than one parent. This is
> +	equivalent to `--max-parents=1`.
> +
> +--min-parents::
> +--max-parents::
> +
> +	Show only commits which have at least resp. at most that many

ENOPARSE.  I guess parentheses around "resp. at most" would work as
a minimal fix, but it might be better to say:

 --min-parents=<n>::
	Show only commits which have at least <n> parents.

 --max-parents=<n>::
	Show only commits which have at least <n> parents.

and perhaps to put

 git log --max-parents=0::
	Lists all root commits.

 git log --min-parents=3::
	Lists all octopus merges.

under EXAMPLES.

> +	commits, where negative parameters for `--max-parents=` denote
> +	infinity (i.e. no upper	limit).

Seems hackish.  Maybe --no-max-parents could denote infinity?

> ++
> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is
> +`--merges` (only), `--max-parents=0` gives all root commits and
> +`--min-parents=3` all octopuses.
> +
>  
>  --first-parent::

It seems there is an extra newline here.

> --- a/t/t6009-rev-list-parent.sh
> +++ b/t/t6009-rev-list-parent.sh
> @@ -1,9 +1,17 @@
>  #!/bin/sh
>  
> -test_description='properly cull all ancestors'
> +test_description='ancestor culling and limiting by parent number'
>  
>  . ./test-lib.sh
>  
> +check_revlist () {
> +	rev_list_args="$1" &&
> +	shift &&
> +	git rev-parse "$@" >expect &&
> +	git rev-list $rev_list_args --all >actual &&
> +	test_cmp expect actual
> +}
> + 

"git am" warns about trailing whitespace on the line after the closing
brace (nothing that --whitespace=fix can't fix, though).

Thanks for factoring this out btw.  It makes the tests themselves
very easy to read.

> +test_expect_success 'rev-list override and infinities' '
> +
> +	check_revlist "--min-parents=2 --max-parents=1 --max-parents=3" tripus normalmerge &&
> +	check_revlist "--min-parents=1 --min-parents=2 --max-parents=7" tetrapus tripus normalmerge &&
> +	check_revlist "--min-parents=2 --max-parents=8" tetrapus tripus normalmerge &&
> +	check_revlist "--min-parents=2 --max-parents=-1" tetrapus tripus normalmerge
> +'

7 and 8 don't mean infinity any more, do they?  What is this test
checking?