[PATCH v4 0/2] fetch: fix non-conflicting tags not being committed

This fixes the bug reported by David Bohman [1].

The 'git-fetch(1)' uses batched updates to perform reference updates when not using 'atomic' transactions. 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. This also extends to backfilling tags.

The first commit, extracts out common code for committing a reference transaction and handling rejected updates. The second commit ensures any failures would also commit pending updates.

[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v4:
- Cleanup the code in the first commit to make it simpler to read.
- In the second commit, we were specifically checking for `retcode > 0`
  for committing the transaction. This is a bit confusing since that
  begs the questions why not `retcode < 0`. There is no real reason
  there, so I've change the code to simple do `if (retcode && ...)`.
  I've also added more information about the flows which would commit
  the transaction in the commit message.
- Link to v3: https://patch.msgid.link/20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com

Changes in v3:
- Split the patch into two commits. One for extracting out existing code
  into a new commit and the other to perform the fix.
- Add back error handling when commit via the normal flow.
- Instead of calling the commit function at every failure, make it part
  of the cleanup code.
- Link to v2: https://patch.msgid.link/20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com

Changes in v2:
- Add a comment to explain the purpose of `commit_ref_transaction()` and
  how it works.
- Also extend the same logic towards backfilling tags. While I was able
  to add a test for the happy path, I couldn't figure out how to test
  when `backfill_tags()` tags would fail.
  Tangentially, this flow seems to only be triggered when using the now
  deprecated 'branches/' remote format.
- Remove unneeded subshells from the tests.
- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com

---
 builtin/fetch.c  | 67 ++++++++++++++++++++++++++++++++++----------------------
 t/t5510-fetch.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 26 deletions(-)

Karthik Nayak (2):
      fetch: extract out reference committing logic
      fetch: fix non-conflicting tags not being committed

Range-diff versus v3:

1:  ee20b46cc2 ! 1:  49fa9a85ef fetch: extract out reference committing logic
    @@ Commit message
         rejection handling logic into a separate function called
         `commit_ref_transaction()`.
     
    +    Helped-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## builtin/fetch.c ##
    @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna
     +				  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 (retcode)
    ++		goto out;
     +
    -+	if (*transaction && !is_atomic) {
    ++	if (!is_atomic) {
     +		struct ref_rejection_data data = {
     +			.conflict_msg_shown = 0,
     +			.remote_name = remote_name,
    @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna
     +		ref_transaction_for_each_rejected_update(*transaction,
     +							 ref_transaction_rejection_handler,
     +							 &data);
    -+
    -+		ref_transaction_free(*transaction);
    -+		*transaction = NULL;
     +	}
     +
    ++out:
    ++	ref_transaction_free(*transaction);
    ++	*transaction = NULL;
     +	return retcode;
     +}
     +
2:  543b67c97c ! 2:  12c71b602d fetch: fix non-conflicting tags not being committed
    @@ Commit message
         extends to backfilling tags which is done when fetching specific
         refspecs which contains tags in their history.
     
    -    Fix this by committing the transaction even when we have an error code.
    -    This ensures other references are applied. Add tests to check for this
    -    regression. While here, add a missing cleanup from previous test.
    +    Fix this by committing the transaction when we have an error code and
    +    not using an atomic transaction. This ensures other references are
    +    applied even when some updates fail.
    +
    +    The cleanup section is reached with `retcode` set in several scenarios:
    +
    +       - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`
    +         before the transaction is created, so no commit is attempted.
    +
    +       - `prune_refs()` sets `retcode` after creating the transaction, so
    +         the commit will now proceed. Before batched updates, `prune_refs()`
    +         created its own transaction internally with all-or-nothing
    +         semantics. This was done since all deletions were made without an
    +         old OID, which meant they were assumed to never fail. This change
    +         allows partial deletions to succeed, consistent with how other
    +         reference updates behave during fetch.
    +
    +       - `fetch_and_consume_refs()` and `backfill_tags()` are the primary
    +         cases this fix targets, both setting a positive `retcode` to
    +         trigger the committing of the transaction.
    +
    +    This simplifies error handling and ensures future modifications to
    +    `do_fetch()` don't need special handling for batched updates.
    +
    +    Add tests to check for this regression. While here, add a missing
    +    cleanup from previous test.
     
         Reported-by: David Bohman <debohman@gmail.com>
         Helped-by: Patrick Steinhardt <ps@pks.im>
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
     +	 * When using batched updates, we want to commit the non-rejected
     +	 * updates and also handle the rejections.
     +	 */
    -+	if (retcode > 0 && !atomic_fetch && transaction)
    ++	if (retcode && !atomic_fetch && transaction)
     +		commit_ref_transaction(&transaction, false,
     +				       transport->remote->name, &err);
     +

base-commit: a99f379adf116d53eb11957af5bab5214915f91d change-id: 20251103-fix-tags-not-fetching-0f1621a474d4

Thanks
- Karthik