Re: [PATCH] fetch: fix non-conflicting tags not being committed

From: Karthik Nayak <karthik.188@gmail.com>
Date: Nov 3, 2025, 21:22 UTC
Message-ID: <CAOLa=ZT0DFG8jx8x=OHouFxinobBbqAbdegaUgkNxy0xLY910A@mail.gmail.com>
In-Reply-To: <CAPig+cRF1hb_RQQCuzZWrnu4AvmOUgVT1mVh=LhP17f7_hYVGQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 33 quoted lines

> On Mon, Nov 3, 2025 at 8:49 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>> The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)
>> updated the 'git-fetch(1)' command to use batched updates. This batches
>> updates to gain performance improvements. When fetching references, each
>> update is added to the transaction. Finally, when committing, individual
>> updates are allowed to fail with reason, while the transaction itself
>> succeeds.
>> [...]
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> @@ -1577,6 +1578,46 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc
>> +test_expect_success 'fetch --tags fetches existing tags' '
>> +       test_when_finished rm -rf base repo &&
>> +       (
>> +               git init base &&
>> +               git -C base commit --allow-empty -m "empty-commit" &&
>> +
>> +               git clone --bare base repo &&
>> +
>> +               git -C base tag tag-1 &&
>> +               git -C repo for-each-ref >out &&
>> +               test_grep ! "tag-1" out &&
>> +               git -C repo fetch --tags &&
>> +               git -C repo for-each-ref >out &&
>> +               test_grep "tag-1" out
>> +       )
>> +'
>
> What is the purpose of wrapping this code in a subshell?
>
> Same question regarding the other test added by this patch.
>

It's not needed, I first created two subshells with cd into each of them. I let it be when I merged them. So let me remove them.

Show 20 quoted lines

>> +test_expect_success 'fetch --tags fetches non-conflicting tags' '
>> +       test_when_finished rm -rf base repo &&
>> +       (
>> +               git init base &&
>> +               git -C base commit --allow-empty -m "empty-commit" &&
>> +               git -C base tag tag-1 &&
>> +
>> +               git clone --bare base repo &&
>> +
>> +               git -C base tag tag-2 &&
>> +               git -C repo for-each-ref >out &&
>> +               test_grep ! "tag-2" out &&
>> +
>> +               git -C base commit --allow-empty -m "second empty-commit" &&
>> +               git -C base tag -f tag-1 &&
>> +
>> +               ! git -C repo fetch --tags 2>out &&
>
> Should this be using `test_must_fail` rather than `!`?
>

Yes, will fix!

Show 5 quoted lines

>> +               test_grep "tag-1  (would clobber existing tag)" out &&
>> +               git -C repo for-each-ref >out &&
>> +               test_grep "tag-2" out
>> +       )
>> +'