Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jeff King wrote:

> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" "
>  	test_i18ncmp expect actual
>  "
> 
> +test_expect_success 'generate builtin list' '
> +	git --list-builtins >builtins
> +'
> +
> +while read builtin
> +do
> +	test_expect_success "$builtin can handle -h" '
> +		test_expect_code 129 git $builtin -h >output 2>&1 &&
> +		test_i18ngrep usage output
> +	'
> +done <builtins

I love this.  A few thoughts:

- I think the "generate builtin list" test should be marked as setup
  so people know it can't be skipped.  I'll send a followup to do that.

- By the same token, if "generate builtin list" fails then these
  commands afterward would fail in an unpleasant way.  Fortunately
  that's straightforward to fix, too.

- This checks both stdout and stderr to verify that the usage appears
  somewhere.  I would have expected commands to be consistent ---
  should they always send usage to stdout, since the user requested it,
  or to stderr, since that's what we have done historically?

  So I understand why this test is agnostic about that but I suspect
  it's pointing to some inconsistency that could be fixed independently.

- Do we plan to translate the "usage:" prefix some day?  I could go
  both ways --- on one hand, having a predictable error:/usage:/fatal:
  prefix may make life easier for e.g. GUI authors trying to show
  different kinds of error messages in different ways, but on the other
  hand, always using English for the prefix may be unfriendly to non
  English speakers.

  In the past I had assumed it would never be translated but now I'm
  not so sure.  What do you think?

- Should we have something like this for non-builtins, too?

In any case,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>