Re: [PATCH 1/7] branch -h: show usage even in an invalid repository

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2010, 18:30 UTC
Message-ID: <7v8w1qnkr1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20101022064258.GB6081@burratino>

Jonathan Nieder <jrnieder@gmail.com> 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 ;-).

Show 18 quoted lines

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