Re: [PATCH 1/2] t5704 (bundle): rewrite for larger coverage

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 15, 2011, 21:16 UTC
Message-ID: <20111215211632.GB16160@elie.hsd1.il.comcast.net>
In-Reply-To: <1323967528-10537-2-git-send-email-artagnon@gmail.com>

Hi,

Ramkumar Ramachandra wrote:

> Rewrite

Always a scary word. Very rarely justified, especially when the original and rewritten versions of something are not going to be able to coexist for a period while the bugs are ironed out. It doesn't leave me optimistic.

>         the git-bundle testsuite to exercise more of its
> functionality.

Luckily, this goal suggests that I am going to see some new tests added, without the existing coverage being removed or mangled, so maybe I can ignore the fears awakened by the word "Rewrite". Let's see...

[...]

Show 7 quoted lines

> --- a/t/t5704-bundle.sh
> +++ b/t/t5704-bundle.sh
> @@ -1,56 +1,99 @@
>  #!/bin/sh
>  
> -test_description='some bundle related tests'
> +test_description='Test git-bundle'

Why?

>  . ./test-lib.sh
>  
>  test_expect_success 'setup' '
> +	git config core.logAllRefUpdates false &&

Why?

Show 7 quoted lines

> +	test_commit initial &&
> +	git checkout -b branch &&
> +	test_commit second &&
> +	test_commit third &&
> +	git checkout master &&
> +	test_commit fourth file
> +'

No explicit tags in the setup this time. Now all commits are referred to by tags, which worsens the coverage, since if some future change caused commits not referred to by a tag to be dropped, it would be missed. Paraphrasing

	>file &&
	git add file &&
	test_tick &&
	git commit -m initial &&
	git tag -m initial initial

to

	test_commit initial file

when not preparing to make some other change in the same place and if the original was not too confusing feels like gratuitous churn.

[...]

Show 12 quoted lines

> +test_expect_success 'create succeeds' '
> +	git bundle create bundle second third &&
> +	cat >expect <<-\EOF &&
> +	OBJID	refs/tags/third
> +	OBJID	refs/tags/second
> +	EOF
> +	{
> +		git ls-remote bundle |
> +		sed "s/$_x40/OBJID/g"
> +	} >actual &&
> +	test_cmp expect actual
> +'

A new test. What assertion is it testing? Why censor out the object names when comparing the expected object names to the actual ones, instead of computing the appropriate object names for the expected result? Is this new test useful, or does it cover ground already tested in t5510-fetch.sh?

> +test_expect_success 'verify succeeds' '
> +	git bundle create bundle second third &&
> +	git bundle verify bundle
>  '

A test for "git bundle verify" is a welcome addition.

Show 13 quoted lines

> +test_expect_success 'list-heads succeeds' '
> +	git bundle create bundle second third &&
> +	cat >expect <<-\EOF &&
> +	OBJID refs/tags/second
> +	OBJID refs/tags/third
> +	EOF
> +	{
> +		git bundle list-heads bundle |
> +		sed "s/$_x40/OBJID/g"
> +	} >actual &&
> +	test_cmp expect actual
> +'
> +

Based on 'git grep -e "git bundle list-heads" -- t', there don't seem to be any existing tests for "git bundle list-heads" except for t5510-fetch.sh, but I'm not sure what this adds on top of that one.

Show 5 quoted lines

> -test_expect_success 'tags can be excluded by rev-list options' '
> -
> -	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
> -	git ls-remote bundle > output &&
> -	! grep tag output

Dropped?

> +test_expect_success 'create dies on invalid bundle filename' '
> +	mkdir adir &&
> +	test_expect_code 128 git bundle create adir --all
> +'

How is "invalid bundle filename" a clearer explanation than "bundle file cannot be created"?

>  
> +test_expect_success 'disallow stray command-line options' '
> +	test_must_fail git bundle create --junk bundle second third
>  '

Ok. Might also be useful to check that no "--junk" or "bundle" file is created.

> +test_expect_failure 'disallow stray command-line arguments' '
> +	git bundle create bundle second third &&
> +	test_must_fail git bundle verify bundle junk
> +'

In this case, "stray command-line arguments" actually means "extra arguments to 'verify'", I guess?

What happens if I run "git bundle verify *.bundle" in a directory with multiple bundles? What should happen?

> +test_expect_success 'create accepts rev-list options to narrow refs' '
> +	git bundle create bundle --all -- file &&

I don't understand what "options to narrow refs" means. Does that mean options like --remotes=origin which yield refs from some subset of the ref namespace, unlike --all?

[...]

> +test_expect_success 'unbundle succeeds' '

A test for "git bundle unbundle" is a welcome addition.

[...]

Show 7 quoted lines

>  test_expect_failure 'bundle --stdin' '
> -
>  	echo master | git bundle create stdin-bundle.bdl --stdin &&
>  	git ls-remote stdin-bundle.bdl >output &&
>  	grep master output
> -
>  '

Seems like a gratuitous change to mix into a patch that introduces functional changes.

I found this hard to review, since it doesn't seem very focussed --- it mixes style cleanups, removal of code, and introduction of new code. I'd be way happier to see a new patch that just adds new tests to the script without potentially breaking anything on the way. Then if the style cleanups still seem important to you, they can be reviewed as a separate patch.

Hoping that clarifies a little, Jonathan