On Tue, Apr 8, 2025 at 2:17 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Anthony Wang <anthonywang513@xxxxxxxxx> 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. > I see, I agree that the test is written just to check that the feature does the intended thing, and not properly written as a tests. I will make the changes and submit a new version. > 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. > To this point, I have a question about when to modify code when making patches. My understanding is that we should try to only modify the code neccesary to fix the bug, and not modify other parts of the code. However, because in this case the test itself does not correctly test for the intended behavior, we should modify because we are already touching this piece of code. Is this correct? Would it then be desired to check the rest of the tests in this file for further oversights and correct them as well, or would that be overstepping boundaries? Sorry for the questions, I just want to understand the best practices as well as I can. Thanks, Anthony
