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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 18, 2011, 21:14 UTC
Message-ID: <20110318211418.GA23407@elie>
In-Reply-To: <182c5ece22835559a9762e242adb701547cf6575.1300459017.git.git@drmicha.warpmail.net>

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

[...]

Show 6 quoted lines

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

Show 5 quoted lines

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