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

[Michael J Gruber <git@drmicha.warpmail.net>]

Jonathan Nieder venit, vidit, dixit 21.03.2011 19:45:
> 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

But why? Basing it on something earlier would have served no purpose
(that I know of) at all.

I should have mentioned the dependency in v1.

> 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.

Yes, revs->no_merges and revs->merges are gone, so a series based on
master would produce a compile failure when applied to next.

> 
>> --- 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.
> 

Well, we discussed this under the v1 thread (after v2 was sent). Junio,
should I do a v3 with that or this?

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

For me, "-1" is a quite natural way to reset a count type parameter, and
you don't even have to think "unsigned" or "mod max_int" for that.

There is no problem parsing for "--max-parents=infinity" and/or
"--no-max-parents" or even (better?) "--max-parents=" without number,
it's only a matter of bike shedding decisions.

>> ++
>> +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).

Hmmm, are there whitespace issues which am warns about and diff does
not, or have I missed a warning?

> 
> 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?

The test checks "override and infinities", where the plural indicates
the fact that it tests different ways of spelling (practical) infinity
such as the very suggestive "8" which nobody cares about but me.

Michael