From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 03 Nov 2025 17:53:16 GMT
Subject: Re: [PATCH] fetch: fix non-conflicting tags not being committed
Message-ID: <CAPig+cRF1hb_RQQCuzZWrnu4AvmOUgVT1mVh=LhP17f7_hYVGQ@mail.gmail.com>
In-Reply-To: <20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com>

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.

> +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 `!`?

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