Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity

From: Anthony Wang <anthonywang513@gmail.com>
Date: Apr 9, 2025, 08:15 UTC
Message-ID: <CAOSofofyfDpFqFzApwMnmYdFb4nk8DS7WwWmYKLPbe+okOE=Tw@mail.gmail.com>
In-Reply-To: <CAPig+cSo2hPpA0DOn7Tr3pcYfN6jxyyo+n0Y0QetvKvNVoLqbA@mail.gmail.com>

On Tue, Apr 8, 2025 at 11:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 7 quoted lines

>
> On Tue, Apr 8, 2025 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote:
> >     t9811: be more precise to check tag creation
> >
> >     The tests grep tagnames they expect to exist from "git tag"
>
> s/tagnames/tag names/ perhaps?

How does "t9811: be more precise to check importing of tags" sound? The tags are created in the p4 repository, and what we care about is if `git p4 sync --import-labels` correctly imports the labels from the p4 repository. Specifying tags instead of tag indicates that there are multiple tags, which is the whole purpose of the test.

Show 17 quoted lines

>
> >     output, which can be fooled by false positive if an unexpected
> >     tag whose name has the expected tagname as its substring.  Fix
> >     them by using "git show-ref --verify" instead.
> >
> >     While we are at it, add a negative test to verify that a tag
> >     that is involved in earlier tests that is not supposed to appear
> >     in the result does indeed not appear in the resulting
> >     repository.
> >
> >     Incidentally, this would also correct the problem the original
> >     had, which lost the exit status of "git tag" that was placed
> >     upstream of a pipe.
> >
> > or something, perhaps?
>
> Yes, better and much more illuminating.

I see, that message does communicate the intentions of the changes much better than I did. I will expand the message and write something along the lines of what you have recommended.

Show 10 quoted lines

>
> > > -             git tag | grep TAG_F1 &&
> > > -             git tag | grep -q TAG_F1_1 &&
> > > -             git tag | grep -q TAG_F1_2 &&
> > > +             git tag &&
> > > +             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 &&
>
> Do we still need the standalone `git tag` invocation above?

The original intent of the patch was to expose the exit code of `git tag`. By keeping the standalone `git tag` we are able to pick up the exit code if there are any issues, such as if `git p4 sync --import-labels` somehow breaks `git tag`. I believe this is the intent of the standalone `git tag` in the section below, and as such, I added it to the top section in order to test the `git tag` functionality, as we removed the other `git tag` instances. However, I can understand if it is unnecessary, and I will remove it.

Show 11 quoted lines

>
> > > @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '
> > >               git p4 submit --verbose &&
> > >               git tag &&
> > > -             git tag | grep TAG_F1_1
> > > +             git show-ref --verify refs/tags/TAG_F1_1 &&
>
> Similarly, it's not clear why there is a standalone `git tag`
> invocation here. Does it buy us anything or am I missing something
> obvious? The originating commit[*] doesn't explain its purpose.
>

Again, as we have removed the other `git tag` instances, it might still have value to test the result of running the `git tag` command.

> [*] e71f6a53e2 (git p4: add test for tag import/export enabled via
> config, 2012-05-11)

Thank you very much for the valuable feedback, it has been very eye opening to see how you two approach patching code like this. I wanted to bump one of my previous questions, as I am very curious about what the best practice is here:

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