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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 29, 2021, 22:49 UTC
Message-ID: <xmqqk0mcuw0e.fsf@gitster.g>
In-Reply-To: <CAPig+cSKOzebGRyoytUGORhq56P0rijYrKO6uu7q7fWnzwiQkw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 11 quoted lines

>> +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?