On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak 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.

One scenario which was missed here, was fetching tags. When fetching
conflicting tags, the `fetch_and_consume_refs()` function returns '1',
which skipped committing the transaction and directly jumped to the
cleanup section. This mean that no updates were applied.

Okay, this is obviously broken indeed.

This also extends to backfilling tags when using the now deprecated
'branches/' format for remotes.

I'm a bit lost here -- what does backfilling have to do with the
"branches/" directory? The backfill is supposed to create tags that
point into the history that one has just fetched. So:

  - With `--tags` we fetch all tags announced by the remote.

  - With `--no-tags` we fetch no tags.

  - Otherwise we fetch those tags that point into our history.

The last behaviour is a bit more on the esoteric side, but it's
described as such in git-fetch(1):

    By default, any tag that points into the histories being fetched is
    also fetched; the effect is to fetch tags that point at branches
    that you are interested in. This default behavior can be changed by
    using the --tags or --no-tags options or by configuring
    remote.<name>.tagOpt. By using a refspec that fetches tags
    explicitly, you can fetch tags that do not point into branches you
    are interested in as well.

The following test demonstrates this behaviour:

	test_expect_success "fetch single branch without explicit tag option" '
		git init source &&
		git -C source commit --allow-empty --message common &&
		git clone file://"$(pwd)"/source target &&
		(
			cd source &&
			git commit --allow-empty --message discard-me &&
			git tag discard-me &&
			git commit --amend --allow-empty --message fetch-me &&
			git tag fetch-me
		) &&

		# The "discard-me" tag does not point into the history that we are
		# about to fetch, so it should not have been created.
		git -C target fetch origin &&
		git -C target tag -l >actual &&
		echo "fetch-me" >expect &&

		# But with "--tags" we instruct git-fetch(1) to fetch all tags, so we
		# should now see it.
		git -C target fetch origin --tags &&
		git -C target tag -l >actual &&
		cat >expect <<-\EOF &&
		discard-me
		fetch-me
		EOF
		test_cmp expect actual
	'

Okay. Do we need to discern cases where this is called and we haven't
managed to even queue a single reference update?

+	return retcode;
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs,
 		    const struct fetch_config *config)

Nit: it might make sense to have a preparatory commit that extracts the
function but that is otherwise a no-op change.

Hm. Don't we also have to unset the transaction now? Ah, no, you pass
the pointer to the transaction here and set it to `NULL` in
`commit_ref_transaction()`. Makes sense.

We now have three different callsites where we commit the transaction.
It gets better due to the newly introduced function, but it overall
feels somewhat fragile regardless of that.

Thanks!

Patrick