v3, 4 partsrefs: add function to translate errors to strings

The git-fetch(1) and git-receive-pack(1) commands update references as part of the flow. Each reference update is treated as a single entity and a transaction is created for each update.

This can be really slow, specifically in reference backends where there are optimizations which ensure a single transaction with 'N' reference update perform much faster than 'N' individual transactions. Also having 'N' individual transactions has buildup and teardown costs. These costs add up in repositories with a large number of references.

Also specifically in the reftable backend, 'N' individual transactions would also trigger auto-compaction for every transaction.

The reasoning for using individual transactions is because we want to allow partial updates of references in these commands. Using a single transaction would be an all-or-nothing scenario.

Recently we introduced an in-between solution called batched reference updates in 23fc8e4f61 (refs: implement batch reference update support, 2025-04-08). This allows us to batch a set of reference updates, where individual updates can pass/fail without affecting the batch.

This patch series, modifies both 'git-fetch(1)' and 'git-receive-pack(1)' to use this mechanism. With this, we see a significant performance boost:

+---------------------+---------------+------------------+
|                     | files backend | reftable backend |
+---------------------+---------------+------------------+
| git-fetch(1)        | 1.25x         | 22x              |
| git-receive-pack(1) | 1.21x         | 18x              |
+---------------------+---------------+------------------+

The first and third patch handle the changes for 'git-fetch(1)' and 'git-receive-pack(1)' respectively. The second patch fixes a small memory leak I encountered while working on this series.

This is based on top of master: 7a1d2bd0a5 (Merge branch 'master' of https://github.com/j6t/gitk, 2025-05-09). There were no conflicts observed with next or seen.

Junio, since the release window for 2.50 is closing in. I would prefer we mark this for 2.51, so perhaps when/if we merge it to 'next', we let it bake there till the next release window opens. While all the tests pass, any bugs here would be high impact and it would be nice to catch it before it hits a release.

---
Changes in v3:
- Modify `ref_transaction_error_msg()` to no longer provided an
  allocated string, but rather a 'const char *' string literal, this
  cleans up unnecessary allocation/free calls.
- Remove some unnecessary type casts.
- Fix some typos.
- Link to v2: https://lore.kernel.org/r/20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-0-80cbaaa55d2e@gmail.com

Changes in v2:
- Added a new commit to introduce a mapping from 'ref_transaction_error'
  to a human readable string. We now use this to map the errors
  observed.
- Add a TODO regarding handling pruning and creation of refs in the same
  transaction in 'git-fetch(1)'.
- Cleanup commit message typos.
- Fix logic in 'builtin/receive-pack.c' around error reporting to be
  cleaner. Thanks Patrick for the suggestion.
- Link to v1: https://lore.kernel.org/r/20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-0-7c65f46493d4@gmail.com

---
 builtin/fetch.c                  | 127 ++++++++++++++++++++++-----------------
 builtin/receive-pack.c           |  64 +++++++++++++++-----
 builtin/update-ref.c             |  25 +-------
 refs.c                           |  20 ++++++
 refs.h                           |   5 ++
 send-pack.c                      |   7 +++
 t/t1416-ref-transaction-hooks.sh |   2 -
 t/t5408-send-pack-stdin.sh       |  15 ++++-
 8 files changed, 166 insertions(+), 99 deletions(-)

Karthik Nayak (4):
      refs: add function to translate errors to strings
      fetch: use batched reference updates
      send-pack: fix memory leak around duplicate refs
      receive-pack: use batched reference updates

Range-diff versus v2:

1:  5196124be4 ! 1:  5bd1ffe4d2 refs: add function to translate errors to strings
    @@ Commit message
         to 'refs.c' as `ref_transaction_error_msg()` and use the same within the
         'builtin/update-ref.c'.
     
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## builtin/update-ref.c ##
    @@ builtin/update-ref.c: static void print_rejected_refs(const char *refname,
     -	default:
     -		reason = "unkown failure";
     -	}
    -+	char *reason = ref_transaction_error_msg(err);
    ++	const char *reason = ref_transaction_error_msg(err);
      
      	strbuf_addf(&sb, "rejected %s %s %s %s\n", refname,
      		    new_oid ? oid_to_hex(new_oid) : new_target,
    -@@ builtin/update-ref.c: static void print_rejected_refs(const char *refname,
    - 		    reason);
    - 
    - 	fwrite(sb.buf, sb.len, 1, stdout);
    -+	free(reason);
    - 	strbuf_release(&sb);
    - }
    - 
     
      ## refs.c ##
     @@ refs.c: int ref_update_expects_existing_old_ref(struct ref_update *update)
    @@ refs.c: int ref_update_expects_existing_old_ref(struct ref_update *update)
      		(!is_null_oid(&update->old_oid) || update->old_target);
      }
     +
    -+char *ref_transaction_error_msg(enum ref_transaction_error err)
    ++const char *ref_transaction_error_msg(enum ref_transaction_error err)
     +{
    -+	const char *reason = "";
    -+
     +	switch (err) {
     +	case REF_TRANSACTION_ERROR_NAME_CONFLICT:
    -+		reason = "refname conflict";
    -+		break;
    ++		return "refname conflict";
     +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
    -+		reason = "reference already exists";
    -+		break;
    ++		return "reference already exists";
     +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
    -+		reason = "reference does not exist";
    -+		break;
    ++		return "reference does not exist";
     +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
    -+		reason = "incorrect old value provided";
    -+		break;
    ++		return "incorrect old value provided";
     +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
    -+		reason = "invalid new value provided";
    -+		break;
    ++		return "invalid new value provided";
     +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
    -+		reason = "expected symref but found regular ref";
    -+		break;
    ++		return "expected symref but found regular ref";
     +	default:
    -+		reason = "unkown failure";
    ++		return "unknown failure";
     +	}
    -+
    -+	return xstrdup(reason);
     +}
     
      ## refs.h ##
    @@ refs.h: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr
     +/*
     + * Translate errors to human readable error messages.
     + */
    -+char *ref_transaction_error_msg(enum ref_transaction_error err);
    ++const char *ref_transaction_error_msg(enum ref_transaction_error err);
     +
      /*
       * Free `*transaction` and all associated data.
2:  c2014b959f ! 2:  dadabf1918 fetch: use batched reference updates
    @@ builtin/fetch.c: static int set_head(const struct ref *remote_refs, struct remot
     +					      enum ref_transaction_error err,
     +					      void *cb_data)
     +{
    -+	struct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;
    ++	struct ref_rejection_data *data = cb_data;
     +
     +	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
     +		error(_("some local refs could not be updated; try running\n"
    @@ builtin/fetch.c: static int set_head(const struct ref *remote_refs, struct remot
     +			"branches"), data->remote_name);
     +		data->conflict_msg_shown = 1;
     +	} else {
    -+		char *reason = ref_transaction_error_msg(err);
    ++		const char *reason = ref_transaction_error_msg(err);
     +
     +		error(_("fetching ref %s failed: %s"), refname, reason);
    -+		free(reason);
     +	}
     +
     +	*data->retcode = 1;
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
     +	 *
     +	 * TODO: if reference transactions gain logical conflict resolution, we
     +	 * can delete and create refs (with F/D conflicts) in the same transaction
    -+	 * and this can be moved about the 'prune_refs()' block.
    ++	 * and this can be moved above the 'prune_refs()' block.
     +	 */
     +	if (!transaction) {
     +		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
3:  43f0dd3c31 = 3:  4006905807 send-pack: fix memory leak around duplicate refs
4:  a976afa025 ! 4:  0549b78b48 receive-pack: use batched reference updates
    @@ Commit message
         batch updates use a transaction under the hood. This explains the change
         in 't1416'.
     
    +    Helped-by: Jeff King <peff@peff.net>
         Helped-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
    @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com
     +{
     +	struct strmap *failed_refs = cb_data;
     +
    -+	strmap_put(failed_refs, refname, ref_transaction_error_msg(err));
    ++	strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));
     +}
     +
      static void execute_commands_non_atomic(struct command *commands,
    @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com
     +		if (reported_error)
     +			cmd->error_string = reported_error;
     +		else if (strmap_contains(&failed_refs, cmd->ref_name))
    -+			cmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));
    ++			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
      	}
     +
     +cleanup:
     +	ref_transaction_free(transaction);
    -+	strmap_clear(&failed_refs, 1);
    ++	strmap_clear(&failed_refs, 0);
      	strbuf_release(&err);
      }
      
    @@ t/t5408-send-pack-stdin.sh: test_expect_success 'stdin mixed with cmdline' '
      	echo A:foo >input &&
      	test_must_fail git send-pack remote.git --stdin B:foo <input &&
     -	verify_push B foo
    ++	test_grep "multiple updates for ref ${SQ}refs/heads/foo${SQ} not allowed" err &&
     +	test_must_fail git --git-dir=remote.git rev-parse foo
      '
      

base-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0 change-id: 20250423-501-update-git-fetch-1-to-use-partial-transactions-7fb66339fe79

Thanks
- Karthik

The commit 76e760b999 (refs: introduce enum-based transaction error types, 2025-04-08) introduced enum-based transaction error types. The refs transaction logic was also modified to propagate these errors. For clients of the ref transaction system, it would be beneficial to provide human readable messages for these errors.

There is already an existing mapping in 'builtin/update-ref.c', move it to 'refs.c' as `ref_transaction_error_msg()` and use the same within the 'builtin/update-ref.c'.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/update-ref.c | 25 +------------------------
 refs.c               | 20 ++++++++++++++++++++
 refs.h               |  5 +++++
 3 files changed, 26 insertions(+), 24 deletions(-)

The reference updates performed as a part of 'git-fetch(1)', take place one at a time. For each reference update, a new transaction is created and committed. This is necessary to ensure we can allow individual updates to fail without failing the entire command. The command also supports an '--atomic' mode, which uses a single transaction to update all of the references. But this mode has an all-or-nothing approach, where if a single update fails, all updates would fail.

In 23fc8e4f61 (refs: implement batch reference update support, 2025-04-08), we introduced a new mechanism to batch reference updates. Under the hood, this uses a single transaction to perform a batch of reference updates, while allowing only individual updates to fail. Utilize this newly introduced batch update mechanism in 'git-fetch(1)'. This provides a significant bump in performance, especially when dealing with repositories with large number of references.

Adding support for batched updates is simply modifying the flow to also create a batch update transaction in the non-atomic flow.

With the reftable backend there is a 22x performance improvement, when performing 'git-fetch(1)' with 10000 refs:

  Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = master)
    Time (mean ± σ):      3.403 s ±  0.775 s    [User: 1.875 s, System: 1.417 s]
    Range (min … max):    2.454 s …  4.529 s    10 runs

  Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
    Time (mean ± σ):     154.3 ms ±  17.6 ms    [User: 102.5 ms, System: 56.1 ms]
    Range (min … max):   145.2 ms … 220.5 ms    18 runs

  Summary
    fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
     22.06 ± 5.62 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = master)

In similar conditions, the files backend sees a 1.25x performance improvement:

  Benchmark 1: fetch: many refs (refformat = files, refcount = 10000, revision = master)
    Time (mean ± σ):     605.5 ms ±   9.4 ms    [User: 117.8 ms, System: 483.3 ms]
    Range (min … max):   595.6 ms … 621.5 ms    10 runs

  Benchmark 2: fetch: many refs (refformat = files, refcount = 10000, revision = HEAD)
    Time (mean ± σ):     485.8 ms ±   4.3 ms    [User: 91.1 ms, System: 396.7 ms]
    Range (min … max):   477.6 ms … 494.3 ms    10 runs

  Summary
    fetch: many refs (refformat = files, refcount = 10000, revision = HEAD) ran
      1.25 ± 0.02 times faster than fetch: many refs (refformat = files, refcount = 10000, revision = master)

With this we'll either be using a regular transaction or a batch update transaction. This helps cleanup some code which is no longer needed as we'll now always have some type of 'ref_transaction' object being propagated.

One big change is that earlier, each individual update would propagate a failure. Whereas now, the `ref_transaction_for_each_rejected_update` function is called at the end of the flow to capture the exit status for 'git-fetch(1)' and also to print F/D conflict errors. This does change the order of the errors being printed, but the behavior stays the same.

Since transaction errors are now explicitly defined as part of 76e760b999 (refs: introduce enum-based transaction error types, 2025-04-08), utilize them and get rid of custom errors defined within 'builtin/fetch.c'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fetch.c | 127 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 73 insertions(+), 54 deletions(-)

The 'git-send-pack(1)' allows users to push objects to a remote repository and explicitly list the references to be pushed. The status of each reference pushed is captured into a list mapped by refname.

If a reference fails to be updated, its error message is captured in the `ref->remote_status` field. While the command allows duplicate ref inputs, the list doesn't accommodate this behavior as a particular refname is linked to a single `struct ref*` element. So if the user inputs a reference twice like:

  git send-pack remote.git A:foo B:foo

where the user is trying to update the same reference 'foo' twice and the reference fails to be updated, we first fill `ref->remote_status` with error message for the input 'A:foo' then we override the same field with the error message for 'B:foo'. This override happens without first free'ing the previous value. Fix this leak.

The current tests already incorporate the above example, but in the test 'A:foo' succeeds while 'B:foo' fails, meaning that the memory leak isn't triggered. Add a new test with multiple duplicates.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 send-pack.c                | 7 +++++++
 t/t5408-send-pack-stdin.sh | 6 ++++++
 2 files changed, 13 insertions(+)

The reference updates performed as a part of 'git-receive-pack(1)', take place one at a time. For each reference update, a new transaction is created and committed. This is necessary to ensure we can allow individual updates to fail without failing the entire command. The command also supports an 'atomic' mode, which uses a single transaction to update all of the references. But this mode has an all-or-nothing approach, where if a single update fails, all updates would fail.

In 23fc8e4f61 (refs: implement batch reference update support, 2025-04-08), we introduced a new mechanism to batch reference updates. Under the hood, this uses a single transaction to perform a batch of reference updates, while allowing only individual updates to fail. Utilize this newly introduced batch update mechanism in 'git-receive-pack(1)'. This provides a significant bump in performance, especially when dealing with repositories with large number of references.

With the reftable backend there is a 18x performance improvement, when performing receive-pack with 10000 refs:

  Benchmark 1: receive: many refs (refformat = reftable, refcount = 10000, revision = master)
    Time (mean ± σ):      4.276 s ±  0.078 s    [User: 0.796 s, System: 3.318 s]
    Range (min … max):    4.185 s …  4.430 s    10 runs

  Benchmark 2: receive: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
    Time (mean ± σ):     235.4 ms ±   6.9 ms    [User: 75.4 ms, System: 157.3 ms]
    Range (min … max):   228.5 ms … 254.2 ms    11 runs

  Summary
    receive: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
     18.16 ± 0.63 times faster than receive: many refs (refformat = reftable, refcount = 10000, revision = master)

In similar conditions, the files backend sees a 1.21x performance improvement:

  Benchmark 1: receive: many refs (refformat = files, refcount = 10000, revision = master)
    Time (mean ± σ):      1.121 s ±  0.021 s    [User: 0.128 s, System: 0.975 s]
    Range (min … max):    1.097 s …  1.156 s    10 runs

  Benchmark 2: receive: many refs (refformat = files, refcount = 10000, revision = HEAD)
    Time (mean ± σ):     927.9 ms ±  22.6 ms    [User: 99.0 ms, System: 815.2 ms]
    Range (min … max):   903.1 ms … 978.0 ms    10 runs

  Summary
    receive: many refs (refformat = files, refcount = 10000, revision = HEAD) ran
      1.21 ± 0.04 times faster than receive: many refs (refformat = files, refcount = 10000, revision = master)

As using batched updates requires the error handling to be moved to the end of the flow, create and use a 'struct strset' to track the failed refs and attribute the correct errors to them.

This change also uncovers an issue when a client provides multiple updates to the same reference. For example:

  $ git send-pack remote.git A:foo B:foo
  Enumerating objects: 3, done.
  Counting objects: 100% (3/3), done.
  Delta compression using up to 20 threads
  Compressing objects: 100% (2/2), done.
  Writing objects: 100% (3/3), 226 bytes | 226.00 KiB/s, done.
  Total 3 (delta 1), reused 0 (delta 0), pack-reused 0 (from 0)
  remote: error: cannot lock ref 'refs/heads/foo': reference already exists
  To remote.git
   ! [remote rejected] A -> foo (failed to update ref)
   ! [remote failure]  B -> foo (remote failed to report status)

As you can see, the remote runs into an error because it cannot lock the target reference for the second update. Furthermore, the remote complains that the first update has been rejected whereas the second update didn't receive any status update because we failed to lock it. Reading this status message alone a user would probably expect that `foo` has not been updated at all. But that's not the case: while we claim that the ref wasn't updated, it surprisingly points to `A` now.

One could argue that this is merely an error in how we report the result of this push. But ultimately, the user's request itself is already broken and doesn't make any sense in the first place and cannot ever lead to a sensible outcome that honors the full request.

The conversion to batched transactions fixes the issue because we now try to queue both updates in the same transaction. As such, the transaction itself will notice this conflict and refuse the update altogether before we commit any of the values.

Note that this requires changes to a couple of tests in t5408 that happened to exercise this behaviour. Given that the generated output is misleading and given that the user request cannot ever be fully honored this really feels more like a bug than properly designed behaviour. As such, changing the behaviour feels like the right thing to do.

Since now reference updates are batched, the 'reference-transaction' hook will be invoked with all updates together. Currently git will 'die' when the hook returns with a non-zero exit status in the 'prepared' stage. For 'git-receive-pack(1)', this allowed users to reject an individual reference update, git would have applied previous updates but immediately abort further execution. This is definitely an incorrect usage of this hook, since the right place to do this would be the 'update' hook. This patch retains the latter behavior, but 'reference-transaction' hook now changes to a all-or-nothing behavior when a non-zero exit status is returned in the 'prepared' stage, since batch updates use a transaction under the hood. This explains the change in 't1416'.

Helped-by: Jeff King <peff@peff.net>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c           | 64 ++++++++++++++++++++++++++++++----------
 t/t1416-ref-transaction-hooks.sh |  2 --
 t/t5408-send-pack-stdin.sh       | 13 ++++----
 3 files changed, 56 insertions(+), 23 deletions(-)

Karthik Nayak <karthik.188@gmail.com> writes:

Very nice.

I've read the difference since the last iteration, "git diff @{1}", and everything looked sensible.

Not an issue with this series at all, but one thing I wondered is if it makes sense to change the type of strmap_get/strmap_put to deal with "const void *". That way, it would not be necessary to cast away the constness like so:

without harming the other side, namely

this piece of code.

It may not make sense, and even if it did, of course, it is totally outside of this series.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Thanks for the review!

It definitely does, The only other typecast I did find for `strmap_put` was within 'strmap.h'. Nevertheless, I think it makes sense to make that change. strmap shouldn't modify the data provided. Perhaps #leftoverbits.

Thanks

Karthik Nayak <karthik.188@gmail.com> writes:

OK.  Thanks.

On Tue, May 20, 2025 at 02:05:09AM -0700, Karthik Nayak wrote:

I'm not sure that is a good idea. Even though strmap does not touch the void data pointer itself, it is accessible to the callers, and we do not know if they stored const data or not, or how they plan to access it.

If we store a "const void *" pointer and returned that via strmap_get(), then there will be callers who need to cast away the constness.

If we store and return a "void *" pointer as we do now, but accept a const pointer via strmap_put(), then we're casting away potentially important const-ness without the caller even seeing it. I think it's safer for the client to do the cast explicitly (since they are the ones who know how they plan to use it).

I don't think we can really represent what we want in C's type system. But if we wanted a safe(r) interface that didn't involve type-casting, we might be able to do something like:

  - the strmap stores an extra flag for "the data pointer is const",
    which can be set by strmap_init(). (It is tempting to replace
    strmap_clear()'s free_entries parameter by checking this flag, but
    the two are not always going to be exactly the same).

  - introduce strmap_get_const() and strmap_put_const() which give and
    receive const pointers

  - in the const functions, BUG() if the "pointers are const" flag is
    not set

But it feels gross, and it only gives runtime checking, not compile-time. What we really want are generics that can carry the type annotation for a particular instantiated map. That is probably pretty easy in most modern languages, but not really in C without horrible macros. :)

-Peff

Jeff King <peff@peff.net> writes:

But isn't that the case now anyways? We always lose the const-ness since we only accept a 'void *'. But by only changing 'strmap_put()' to accept a 'const void *', but storing and returning a 'void *'. We simply modify the current construct to also say that any data received is not modified. But I do see your point, we'll have to cast there anyways and might as well do it on the client side.

Agreed, and then there is additional load to ensure users will use 'strmap_put()' and 'strmap_put_const()' as required and simply not cast away.

Overall I agree with what you're saying. Thanks for spelling it out.

Karthik

On Thu, May 22, 2025 at 01:50:18AM -0700, Karthik Nayak wrote:

Right, there is no winning here in the type system. But by pushing the cast onto the calling side, it is more visible and closer to the code that knows how the result will be used.

That part I'm not too worried about, since casts are easy-ish to catch and flag in reviews. But I'm not sure if it's materially making the world a better place to have the extra complexity.

-Peff