Jonathan Nieder <jrnieder@xxxxxxxxx> writes: > [jn: simplified; new commit message, test] Overall (not just this 1/7) the messages justify the changes a lot better. Thanks for a pleasant read ;-). > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index f54a533..f308235 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -26,6 +26,17 @@ test_expect_success \ > ! test -f .git/refs/heads/--help > ' > > +test_expect_success 'branch -h in broken repository' ' > + mkdir broken && > + ( > + cd broken && > + git init && > + >.git/refs/heads/master && > + test_expect_code 129 git branch -h >usage 2>&1 > + ) && > + grep "[Uu]sage" broken/usage > +' A handful of points that struck me (not just test, but taking tests from other patches together) as slightly odd, all minor: - Why '[Uu]sage"? Should we make the messages consistent further? - The final "grep" check may want to make sure that the message is free of "fatal" (or "error", "warning", etc.) as well? - Each test seems to anticipate a specific kind of breakage tailored for the command being tested (e.g. "branch" test does not corrupt config nor the index). Perhaps "lib-corrupt.sh" helper to run the same test in variety of corrupt repositories would help improve coverage? [*1*] - Some tests redirect both the standard output and the standard error (like this patch) and check the combined result, while some others (e.g. 2/7) check only the standard error stream. Don't we want to be testing them more uniformly? > test_expect_success \ > 'git branch abc should create a branch' \ > 'git branch abc && test -f .git/refs/heads/abc' Don't we want to rather use resolve-ref or "rev-parse --verify", just in case we may later change "git branch" to update packed-refs directly? [Footnote] *1* I am of two minds, as for example a corrupt "gc.pruneexpire" cannot possibly matter for correct operation of "git branch", but am just throwing out an idea to see if somebody else have clever ideas. -- 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