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...
[...]
> --- 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?
> + 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.
[...]
> +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.
> +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.
> -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.
[...]
> 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
--
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