From: Karthik Nayak Date: Fri, 07 Nov 2025 13:15:32 GMT Subject: Re: [PATCH v2] fetch: fix non-conflicting tags not being committed Message-ID: In-Reply-To: Patrick Steinhardt writes: > 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: > 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..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. >> 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. >> + 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. >> @@ -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. > >> @@ -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