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.
> --- 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.
> @@ -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
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
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).
> +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.
> +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' '
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' '
...
'
Thanks for your attention to detail, and hope that helps.
Ciao,
Jonathan
--
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