Re: [PATCH v4 1/2] t6400: preserve git ls-files exit status code

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +check_ls_files_count() {
>
> style: funcname () {
> ...
> I also &&-chain the `local` declaration:
>
>     local ops val &&
>     if test "$#" -le 2
> ...
> A quick grep of the tests indicates that they are consistent about
> using lowercase for the first word in a BUG():

Thanks for a pair of sharp eyes, Eric, in your review.

> -	test 5 -eq $(git ls-files -s | wc -l) &&
> -	test 4 -eq $(git ls-files -u | wc -l) &&
> +	check_ls_files_count = 5 -s &&
> +	check_ls_files_count = 4 -u &&

I have one more comment on the main part of the patch.  It is easy
to see that this conversion is correctly done in this particular
patch from the way 5/4 and -s/u are reproduced from the preimage to
the postimage, but I doubt that readers in the future, who long have
forgotten that the "-s" came from "ls-files -s", would find the new
form easy to read and understand.

Do we have the same helper duplicated across two test scripts?

I wonder if it is worth adding a single copy that forces the callers
to spell out the command name in test-lib.sh and make the above into
something like

	test_output_wc_l = 5 ls-files -s

or even

	test_output_wc_l = 5 git ls-files -s

That way, it is easier to see what command is being run (yes, I know
you have _ls_files_ in the middle of the name of the custom helper,
but the thing is that "-s" and "_ls_files_" in the middle of the
helper are so far apart that it is not immediately obvious what the
argument "-s" is about), and by not having two identical copies, we
have less risk of them drifting apart.

Hmm?