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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2025, 00:17 UTC
Message-ID: <xmqqtt6zjyma.fsf@gitster.g>
In-Reply-To: <20250407212834.53183-1-anthonywang03@icloud.com>

Anthony Wang <anthonywang513@gmail.com> writes:

Show 14 quoted lines

>> 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.