Patrick Steinhardt [off-list ref] writes:

On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:

quoted

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.

quoted

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:

I didn't read the code well enough. Let me walk through what I read:

The block for backfilling tags, is only triggered in `do_fetch()`, if

    if (tags == TAGS_DEFAULT && autotags) { ... }

This means that the autotags must be '1'. And I see at the start of the
function that:

   int autotags = (transport->remote->fetch_tags == 1);

So I went into looking when `transport->remote->fetch_tags` would be set
to '1'. This is only done in `read_branches_file()` which is done when
parsing the now deprecated 'branches/' directory.

I was correct until here. But, there is something I missed.

We also pass a pointer to `autotags` to the `get_ref_map()` function. In
this function, we set `autotags` to '1' for any of the following
conditions:

   - When there is a refspec specified by the user.

   - We have a default branch with a remote specified.

So this means there are other scenarios we use the backfill() command.

That brings us to the second part of it, if we specify the '--tags'
flag, then we fetch all tags, even the ones which aren't part of our
history. This also happens as part of the `get_ref_map()` function. This
flow also skips the 'backfill()' function.

So in effect, we only backfill tags, when the user doesn't specify
either '--tags' or '--no-tags'.

  - 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.

But backfilling isn't about diverged history, no? It's about fetching
history of refs being requested.

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 &&

Here, we don't really backfill, but rather we request all tags from the
remote, hence we end up with the 'discard-me' tag. Not because of the
diverged history. I also confirmed this by adding a breakpoint into the
`backfill_tags()` function, while running this test.

		git -C target tag -l >actual &&
		cat >expect <<-\EOF &&
		discard-me
		fetch-me
		EOF
		test_cmp expect actual
	'

But I was able to slightly modify the test to get the required affect:

  test_expect_success "backfill tags when providing a refspec" '
  	git init source &&
  	git -C source commit --allow-empty --message common &&
  	git clone file://"$(pwd)"/source target &&
  	(
  	    cd source &&
  	    git commit --allow-empty --message history &&
  	    git tag history &&
  	    git commit --allow-empty --message fetch-me &&
  	    git tag fetch-me
  	) &&

  	# The "history" tag is backfilled eventhough we requested
  	# to only fetch the master
  	git -C target fetch origin master:branch &&
  	git -C target tag -l >actual &&
  	cat >expect <<-\EOF &&
  	fetch-me
  	history
  	EOF
  	test_cmp expect actual
  '

I will add this in. Thanks for the explanation, it really helped
consolidate my understanding here.

quoted

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c7ff3480fb..d5aee5af10 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
 	*data->retcode = 1;
 }

+/*
+ * Commit the reference transaction. If it isn't an atomic transaction, handle
+ * rejected updates as part of using batched updates.
+ */
+static int commit_ref_transaction(struct ref_transaction **transaction,
+				  bool is_atomic, const char *remote_name,
+				  struct strbuf *err)
+{
+	int retcode = ref_transaction_commit(*transaction, err);
+	if (retcode) {
+		/*
+		 * Explicitly handle transaction cleanup to avoid
+		 * aborting an already closed transaction.
+		 */
+		ref_transaction_free(*transaction);
+		*transaction = NULL;
+	}
+
+	if (*transaction && !is_atomic) {
+		struct ref_rejection_data data = {
+			.conflict_msg_shown = 0,
+			.remote_name = remote_name,
+			.retcode = &retcode,
+		};
+
+		ref_transaction_for_each_rejected_update(*transaction,
+							 ref_transaction_rejection_handler,
+							 &data);
+
+		ref_transaction_free(*transaction);
+		*transaction = NULL;
+	}

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

I don't see a reason. This is anyways a post-commit action, if there are
no updates, there will be no rejections. So this will be a no-op.

quoted

+	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.

Let me do that. I was thinking the change is small. But perhaps that'd
be easier for reviewing.

quoted

@@ -1826,6 +1862,10 @@ static int do_fetch(struct transport *transport,

 	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
 				   &fetch_head, config)) {
+		/* As we're using batched updates, commit any pending updates. */
+		if (!atomic_fetch)
+			commit_ref_transaction(&transaction, false,
+					       transport->remote->name, &err);
 		retcode = 1;
 		goto cleanup;
 	}

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.

quoted

@@ -1848,8 +1888,12 @@ static int do_fetch(struct transport *transport,
 			 * the transaction and don't commit anything.
 			 */
 			if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
-					  &fetch_head, config))
+					  &fetch_head, config)) {
+				if (!atomic_fetch)
+					commit_ref_transaction(&transaction, false,
+							       transport->remote->name, &err);
 				retcode = 1;
+			}
 		}

 		free_refs(tags_ref_map);

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.

Yeah I must agree with that. I could think of a cleaner way, but will
spend some time here.

Thanks!

Patrick

Thanks,
Karthik