Jonathan Nieder venit, vidit, dixit 18.03.2011 22:14:
> Hi,
>
> Michael J Gruber wrote:
>
>> [Subject: rev-list --min-parents,--max-parents: doc and test and completion]
>>
>> This also adds test for "--merges" and "--no-merges" which we did not
>> have so far.
>
> Minor nit: the life of reviewers is easier when the documentation and
> tests are squashed with the implementation (or if anything, the
> documentation comes first) so they can look at what the code tries to
> do before seeing how it does it.
Come on, with a cover-letter saying doc and tests are in 3/3, how much
effort is it to read that before 1/3 if you care?
The tests are bogus before the code and the doc pointless before it.
Squashing 1 and 3 is okay, of course. For my own digestion, smaller
bites are better.
>> --- a/t/t6009-rev-list-parent.sh
>> +++ b/t/t6009-rev-list-parent.sh
>> @@ -1,6 +1,6 @@
> [...]
>> -test_description='properly cull all ancestors'
>> +test_description='ancestor culling and limitting by parent number'
>
> sp: limiting
>
> Thanks for updating the description. It's easy to forget.
I would have preferred to assign an extra test number (since the
relation between the above is a bit weak) but the numbers are all used
up in that range.
>
>> @@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '
> [...]
>> +check_revlist () {
>> + > expect &&
>> + for c in $2; do echo "$c" >> expect; done &&
>> + git rev-list $1 master | git name-rev --name-only --tags --stdin >actual &&
>> + test_cmp expect actual
>> +}
>
> Pedantic nits:
>
> * if global functions are defined before all test_expect_success stanzas,
> (1) it is easier to reuse them in tests throughout the script;
> (2) it is easy to see at a glance what primitives a test script
> will be using when getting acquainted with it and what
> syntax might be worth lifting to test-lib or other test
> scripts when shopping for that
> * style: no space between >redirection operators and their argument
> for consistency (maybe because > &2 is a syntax error)
> * might be simpler to pass one rev per argument. Like so:
>
> rev_list_args=$1 &&
> shift &&
> printf '%s\n' "$@" >expect &&
> git rev-list $rev_list_args master ...
>
> Avoiding the for loop means errors from 'echo' before the last
> iteration are not ignored; a more verbose way to write the same
Do we really need to safe-guard echo and prints?
> thing is
>
> for commit
> do
> printf '%s\n' "$commit" ||
> return
> done >expect &&
> git rev-list ...
> * this does not catch errors from rev-list! how about
>
> git rev-list $rev_list_args master >rev-list &&
> git name-rev ... <rev-list >actual &&
> test_cmp expect actual
Thanks for both of the above, that makes things much better. Although I
have to treat the case with empty rev-list specially now, or use the
verbose version.
>
> Junio will probably complain that this is not meant to be a test
> for name-rev; indeed, filling "expect" by computing commit names
> might be even better (because faster).
Well, we do this in other places as well, and it makes the test much
more readable. Alternatively, I could use rev-parse on the args, which
is faster than name-rev on the rev-list output. That makes it less
readable in case a test breaks, though. Anyways, I'll go for that.
>
>> +test_expect_success 'rev-list roots' '
>> +test_expect_success 'rev-list no merges' '
>> +test_expect_success 'rev-list no octopusses' '
>> +test_expect_success 'rev-list no roots' '
>> +test_expect_success 'rev-list merges' '
>
> Neat. :)
>
>
>> +test_expect_success 'rev-list octopus' '
>> +
>> + check_revlist "--min-parents=3" "octopus"
>> +'
>
> Might make sense to check a dodecapus (with --min-parents=3 still) as
> well.
Dodeka, really? I leave that to you.
I might add a tetrapus, though.
>
>> +test_expect_success 'rev-list override and infinities' '
>
> Ah, here's the test I suggested in reply to patch 1. But this is not
> about overriding but the lack thereof, no? So I'd be happier reading
> something like
>
> test_expect_success '--no-merges --merges yields the empty set' '
> check_revlist "--no-merges --merges" &&
> check_revlist "--min-parents=2 --no-merges"
> '
>
> test_expect_success 'last --max-parents wins' '
wins against? Against itself, i.e. it overrides previous occurences. But
I'll separate these.
> check_revlist "--min-parents=2 --no-merges --max-parents=3" octopus normalmerge &&
> check_revlist "--min-parents=2 --max-parents=3 --no-merges" ...
> '
>
> test_expect_success '--max-parents=infinity' '
> ...
> '
>
> test_expect_success '--min-parents=infinity' '
> ...
> '
>
> test_expect_failure '--max-parents=gobbledegood errors out' '
> ...
> '
I don't really want to parse for the string "infinity" nor go for strtol
instead of atoi. Why shouldn't something "unparseable" be 0?
Michael
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html