Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes

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

Anthony Wang <anthonywang513@gmail.com> writes:

>> If so, instead of grepping around, we should be testing that in a
>> more direct way, perhaps with something like
>> 
>>         git show-ref --verify refs/tags/TAG_F1_1 &&
>>         git show-ref --verify refs/tags/TAG_F1_2 &&
>>         test_must_fail  git show-ref --verify refs/tags/TAG_F1_ONLY &&
>> 
>> no?
>> 
>
> Possibly, but I believe adding the test_must_fail check would be modifying
> the original intent of the test, as it would pass even with the existence of
> TAG_F1_ONLY. However, if we are only performing actions to cause TAG_F1_1 
> and TAG_F1_2 to exist, then it would be an issue if TAG_F1_ONLY existed.

I view it a bit differently.

Use of "grep" over the output of "git tag" is simply a sloppy
programming.  If the test wanted to verify "TAG_F1_1 exists", it
shouldn't have grepped for TAG_F1_1, because another tag T_TAG_F1_1
would produce a false positive hit if the earlier test gets updated.

Similarly, not verifying what should not exist is being sloppy.
People who come up with a new feature (in this case, "git p4 sync"
involving tags) tend to test positive effects to show how their
shiny new toy does things, and forgets to test lack of effects to
ensure that their shiny new toy does *not* do what they should not
do.

If the original test were written solidly and use of pipe hiding
exit code were the only problem it had, I would agree that making
minimum change should be preferrable, but the original test seems to
be so sloppy in this case.

Thanks.