Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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?
