patch, 3 partsfetch/receive: use batched reference updates

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

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 5279997c96..1558f6d1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1688,6 +1644,32 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>  	return result;
>  }
>  
> +struct ref_rejection_data {
> +	int *retcode;
> +	int conflict_msg_shown;
> +	const char *remote_name;
> +};
> +
> +static void ref_transaction_rejection_handler(const char *refname UNUSED,
> +					      const struct object_id *old_oid UNUSED,
> +					      const struct object_id *new_oid UNUSED,
> +					      const char *old_target UNUSED,
> +					      const char *new_target UNUSED,
> +					      enum ref_transaction_error err,
> +					      void *cb_data)
> +{
> +	struct ref_rejection_data *data = (struct ref_rejection_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"
> +			" 'git remote prune %s' to remove any old, conflicting "
> +			"branches"), data->remote_name);
> +		data->conflict_msg_shown = 1;
> +	}
> +
> +	*data->retcode = 1;
> +}
> +
>  static int do_fetch(struct transport *transport,
>  		    struct refspec *rs,
>  		    const struct fetch_config *config)

> @@ -1808,6 +1790,20 @@ static int do_fetch(struct transport *transport,
>  			retcode = 1;
>  	}
>  
> +	/*
> +	 * If not atomic, we can still use batched updates, which would be much
> +	 * more performent. We don't initiate the transaction before pruning,

> +	 * since pruning must be an independent step, to avoid F/D conflicts.
> +	 */
> +	if (!transaction) {
> +		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +							  REF_TRANSACTION_ALLOW_FAILURE, &err);
> +		if (!transaction) {
> +			retcode = -1;
> +			goto cleanup;
> +		}
> +	}
> +
>  	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
>  				   &fetch_head, config)) {
>  		retcode = 1;

> Yup, this makes sense, as well. We lose a bunch of overhead by creating
> separate transactions for each of the updates, but the slow path is
> still that we have to create 10000 files for each of the references. So
> it's expected to see a small performance improvement, but nothing game
> changing.
>

>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 5279997c96..1558f6d1e8 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1688,6 +1644,32 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>>  	return result;
>>  }
>>
>> +struct ref_rejection_data {
>> +	int *retcode;
>> +	int conflict_msg_shown;
>> +	const char *remote_name;
>> +};
>> +
>> +static void ref_transaction_rejection_handler(const char *refname UNUSED,
>> +					      const struct object_id *old_oid UNUSED,
>> +					      const struct object_id *new_oid UNUSED,
>> +					      const char *old_target UNUSED,
>> +					      const char *new_target UNUSED,
>> +					      enum ref_transaction_error err,
>> +					      void *cb_data)
>> +{
>> +	struct ref_rejection_data *data = (struct ref_rejection_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"
>> +			" 'git remote prune %s' to remove any old, conflicting "
>> +			"branches"), data->remote_name);
>> +		data->conflict_msg_shown = 1;
>> +	}
>> +
>> +	*data->retcode = 1;
>> +}
>> +
>>  static int do_fetch(struct transport *transport,
>>  		    struct refspec *rs,
>>  		    const struct fetch_config *config)
>
> Okay, so we now handle errors over here. Is the handled error the only
> error that we may see, or do we also accept other errors like D/F now?
> If the latter, wouldn't it mean that we don't print any error messages
> for those other failures at all? That might be quite confusing.
>

>> @@ -1808,6 +1790,20 @@ static int do_fetch(struct transport *transport,
>>  			retcode = 1;
>>  	}
>>
>> +	/*
>> +	 * If not atomic, we can still use batched updates, which would be much
>> +	 * more performent. We don't initiate the transaction before pruning,
>
> s/performent/performant/
>

>> +	 * since pruning must be an independent step, to avoid F/D conflicts.
>> +	 */
>> +	if (!transaction) {
>> +		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> +							  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> +		if (!transaction) {
>> +			retcode = -1;
>> +			goto cleanup;
>> +		}
>> +	}
>> +
>>  	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
>>  				   &fetch_head, config)) {
>>  		retcode = 1;
>
> Don't transactions handle D/F conflicts for us? Isn't that the sole
> reason why for example `refs_verify_refname_available()` accepts an
> "extras" parameter that is supposed to contain refs that are about to be
> deleted?
>

> Patrick Steinhardt <ps@pks.im> writes:
> >> +	 * since pruning must be an independent step, to avoid F/D conflicts.
> >> +	 */
> >> +	if (!transaction) {
> >> +		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> >> +							  REF_TRANSACTION_ALLOW_FAILURE, &err);
> >> +		if (!transaction) {
> >> +			retcode = -1;
> >> +			goto cleanup;
> >> +		}
> >> +	}
> >> +
> >>  	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
> >>  				   &fetch_head, config)) {
> >>  		retcode = 1;
> >
> > Don't transactions handle D/F conflicts for us? Isn't that the sole
> > reason why for example `refs_verify_refname_available()` accepts an
> > "extras" parameter that is supposed to contain refs that are about to be
> > deleted?
> >
> 
> My understanding was a little different, from the documentation for the
> function:
> 
>   If extras is non-NULL, it is a list of additional refnames with which
>   refname is not allowed to conflict.
> 
> This is to capture additional conflicts. We want a way to avoid said
> conflicts. That said, there is a 'skip' parameter which does exactly
> what you're saying.

> But the transaction logic doesn't incorporate this
> entirely. Specifically in the files backend, where we create a lock in
> the filesystem, this would cause a conflict, consider the following:
> 
>   ❯ eza --tree .git/refs/remotes/
>   .git/refs/remotes
>   └── origin
>       ├── dir
>       │   └── file.lock
>       ├── dir.lock
>       └── HEAD
> 
> This is from the test 'branchname D/F conflict resolved by --prune', the
> test prunes the existing reference 'refs/remotes/origin/dir/file' while
> adding 'refs/remotes/origin/dir'. In 'lock_raw_ref()' we lock both and
> read the reference, but this causes an issue since
> 'refs/remotes/origin/dir' exists as a directory already.
> 
> I would say this is logically solvable if we start treating conflict
> resolution within updates as a first class problem. But perhaps that's
> something of a patch series in itself and better solved outside of this?

> On Thu, May 15, 2025 at 11:13:32AM +0000, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> >> +	 * since pruning must be an independent step, to avoid F/D conflicts.
>> >> +	 */
>> >> +	if (!transaction) {
>> >> +		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> >> +							  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> >> +		if (!transaction) {
>> >> +			retcode = -1;
>> >> +			goto cleanup;
>> >> +		}
>> >> +	}
>> >> +
>> >>  	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
>> >>  				   &fetch_head, config)) {
>> >>  		retcode = 1;
>> >
>> > Don't transactions handle D/F conflicts for us? Isn't that the sole
>> > reason why for example `refs_verify_refname_available()` accepts an
>> > "extras" parameter that is supposed to contain refs that are about to be
>> > deleted?
>> >
>>
>> My understanding was a little different, from the documentation for the
>> function:
>>
>>   If extras is non-NULL, it is a list of additional refnames with which
>>   refname is not allowed to conflict.
>>
>> This is to capture additional conflicts. We want a way to avoid said
>> conflicts. That said, there is a 'skip' parameter which does exactly
>> what you're saying.
>
> Oh, right, my mistake -- that's what I actually meant.
>
>> But the transaction logic doesn't incorporate this
>> entirely. Specifically in the files backend, where we create a lock in
>> the filesystem, this would cause a conflict, consider the following:
>>
>>   ❯ eza --tree .git/refs/remotes/
>>   .git/refs/remotes
>>   └── origin
>>       ├── dir
>>       │   └── file.lock
>>       ├── dir.lock
>>       └── HEAD
>>
>> This is from the test 'branchname D/F conflict resolved by --prune', the
>> test prunes the existing reference 'refs/remotes/origin/dir/file' while
>> adding 'refs/remotes/origin/dir'. In 'lock_raw_ref()' we lock both and
>> read the reference, but this causes an issue since
>> 'refs/remotes/origin/dir' exists as a directory already.
>>
>> I would say this is logically solvable if we start treating conflict
>> resolution within updates as a first class problem. But perhaps that's
>> something of a patch series in itself and better solved outside of this?
>
> Fair enough, makes sense. Might be worth it to add a TODO comment there
> then?
>

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

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index be314879e8..b4fceb3837 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1843,35 +1843,91 @@ static void BUG_if_skipped_connectivity_check(struct command *commands,
>  	BUG_if_bug("connectivity check skipped???");
>  }
>  
> +static void ref_transaction_rejection_handler(const char *refname,
> +					      const struct object_id *old_oid UNUSED,
> +					      const struct object_id *new_oid UNUSED,
> +					      const char *old_target UNUSED,
> +					      const char *new_target UNUSED,
> +					      enum ref_transaction_error err,
> +					      void *cb_data)
> +{
> +	struct strmap *failed_refs = (struct strmap *)cb_data;

> +	const char *reason = "";
> +
> +	switch (err) {
> +	case REF_TRANSACTION_ERROR_NAME_CONFLICT:
> +		reason = "refname conflict";
> +		break;
> +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
> +		reason = "reference already exists";
> +		break;
> +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
> +		reason = "reference does not exist";
> +		break;
> +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
> +		reason = "incorrect old value provided";
> +		break;
> +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
> +		reason = "invalid new value provided";
> +		break;
> +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
> +		reason = "expected symref but found regular ref";
> +		break;
> +	default:
> +		reason = "unkown failure";
> +	}
> +
> +	strmap_put(failed_refs, refname, xstrdup(reason));
> +}

>  static void execute_commands_non_atomic(struct command *commands,
>  					struct shallow_info *si)
>  {
>  	struct command *cmd;
>  	struct strbuf err = STRBUF_INIT;
> +	const char *reported_error = "";
> +	struct strmap failed_refs = STRMAP_INIT;
> +
> +	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +						  REF_TRANSACTION_ALLOW_FAILURE, &err);
> +	if (!transaction) {
> +		rp_error("%s", err.buf);
> +		strbuf_reset(&err);
> +		reported_error = "transaction failed to start";
> +		goto failure;
> +	}

>  	for (cmd = commands; cmd; cmd = cmd->next) {
>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>  			continue;
>  
> -		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> -							  0, &err);
> -		if (!transaction) {
> -			rp_error("%s", err.buf);
> -			strbuf_reset(&err);
> -			cmd->error_string = "transaction failed to start";
> -			continue;
> -		}
> -
>  		cmd->error_string = update(cmd, si);
> +	}

> -		if (!cmd->error_string
> -		    && ref_transaction_commit(transaction, &err)) {
> -			rp_error("%s", err.buf);
> -			strbuf_reset(&err);
> -			cmd->error_string = "failed to update ref";
> -		}
> -		ref_transaction_free(transaction);
> +	if (ref_transaction_commit(transaction, &err)) {
> +		rp_error("%s", err.buf);
> +		reported_error = "failed to update refs";
> +		goto failure;
>  	}
> +
> +	ref_transaction_for_each_rejected_update(transaction,
> +						 ref_transaction_rejection_handler,
> +						 &failed_refs);
> +
> +	if (strmap_empty(&failed_refs))
> +		goto cleanup;
> +
> +failure:
> +	for (cmd = commands; cmd; cmd = cmd->next) {
> +		if (strmap_empty(&failed_refs))
> +			cmd->error_string = reported_error;

>> +	switch (err) {
>> +	case REF_TRANSACTION_ERROR_NAME_CONFLICT:
>> +		reason = "refname conflict";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
>> +		reason = "reference already exists";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
>> +		reason = "reference does not exist";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
>> +		reason = "incorrect old value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
>> +		reason = "invalid new value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
>> +		reason = "expected symref but found regular ref";
>> +		break;
>> +	default:
>> +		reason = "unkown failure";
>> +	}
>> +
>> +	strmap_put(failed_refs, refname, xstrdup(reason));
>> +}
>
> I'd have expected something like this for git-fetch(1), as well, so that
> we don't silently swallow failed ref updates. Would it make sense to
> maybe provide an array of reasons by enum so that we can reuse those
> messages?

>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index be314879e8..b4fceb3837 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1843,35 +1843,91 @@ static void BUG_if_skipped_connectivity_check(struct command *commands,
>>  	BUG_if_bug("connectivity check skipped???");
>>  }
>>
>> +static void ref_transaction_rejection_handler(const char *refname,
>> +					      const struct object_id *old_oid UNUSED,
>> +					      const struct object_id *new_oid UNUSED,
>> +					      const char *old_target UNUSED,
>> +					      const char *new_target UNUSED,
>> +					      enum ref_transaction_error err,
>> +					      void *cb_data)
>> +{
>> +	struct strmap *failed_refs = (struct strmap *)cb_data;
>
> This cast is unnecessary.
>

>> +	const char *reason = "";
>> +
>> +	switch (err) {
>> +	case REF_TRANSACTION_ERROR_NAME_CONFLICT:
>> +		reason = "refname conflict";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
>> +		reason = "reference already exists";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
>> +		reason = "reference does not exist";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
>> +		reason = "incorrect old value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
>> +		reason = "invalid new value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
>> +		reason = "expected symref but found regular ref";
>> +		break;
>> +	default:
>> +		reason = "unkown failure";
>> +	}
>> +
>> +	strmap_put(failed_refs, refname, xstrdup(reason));
>> +}
>
> I'd have expected something like this for git-fetch(1), as well, so that
> we don't silently swallow failed ref updates. Would it make sense to
> maybe provide an array of reasons by enum so that we can reuse those
> messages?
>

>>  static void execute_commands_non_atomic(struct command *commands,
>>  					struct shallow_info *si)
>>  {
>>  	struct command *cmd;
>>  	struct strbuf err = STRBUF_INIT;
>> +	const char *reported_error = "";
>> +	struct strmap failed_refs = STRMAP_INIT;
>> +
>> +	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> +						  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> +	if (!transaction) {
>> +		rp_error("%s", err.buf);
>> +		strbuf_reset(&err);
>> +		reported_error = "transaction failed to start";
>> +		goto failure;
>> +	}
>
> Okay. We now create a single transaction with failures being allowed.
>
>>  	for (cmd = commands; cmd; cmd = cmd->next) {
>>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>>  			continue;
>>
>> -		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> -							  0, &err);
>> -		if (!transaction) {
>> -			rp_error("%s", err.buf);
>> -			strbuf_reset(&err);
>> -			cmd->error_string = "transaction failed to start";
>> -			continue;
>> -		}
>> -
>>  		cmd->error_string = update(cmd, si);
>> +	}
>
> So here we only need to queue each update.
>
>> -		if (!cmd->error_string
>> -		    && ref_transaction_commit(transaction, &err)) {
>> -			rp_error("%s", err.buf);
>> -			strbuf_reset(&err);
>> -			cmd->error_string = "failed to update ref";
>> -		}
>> -		ref_transaction_free(transaction);
>> +	if (ref_transaction_commit(transaction, &err)) {
>> +		rp_error("%s", err.buf);
>> +		reported_error = "failed to update refs";
>> +		goto failure;
>>  	}
>> +
>> +	ref_transaction_for_each_rejected_update(transaction,
>> +						 ref_transaction_rejection_handler,
>> +						 &failed_refs);
>> +
>> +	if (strmap_empty(&failed_refs))
>> +		goto cleanup;
>> +
>> +failure:
>> +	for (cmd = commands; cmd; cmd = cmd->next) {
>> +		if (strmap_empty(&failed_refs))
>> +			cmd->error_string = reported_error;
>
> The reported error may have one of there values now:
>
>   - The empty string. Is it correct to set the error string to that
>     value? Shouldn't it rather be a `NULL` pointer?
>
>   - "transaction failed to start". It makes sense to update every
>     command accordingly, as we wouldn't have updated any of them.
>
>   - "failed to update refs", in case the commit failed. Does the commit
>     fail only in cases where we couldn't update _any_ reference, or does
>     it also retrun an error when only one of the updates failed? If the
>     latter, we shouldn't update all the others to "failed", should we?
>
> In any case, it feels weird that we use `strmap_empty()` to check for
> this condition. I'd have expected that we rather check for
> `reported_error` to be a non-empty string directly to figure out whether
> the transaction itself has failed as a whole. Like that, we'd know that
> we only ever do this if we hit a `goto failure` branch.
>

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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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 of doesn't accommodate this behavior as a particular
>
> -ECANNOTPARSE around "the list of doesn't accommodate".
>

>> 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.
>
> OK.  A natural question is what happens when A successfully updates
> and B fails, or A fails but B successfully updates (failing both is
> much less interesting).  What should happen in such cases is unclear
> but my gut feeling is that the last-one wins (which you implemented)
> is probably just as OK as the first-one gets retained (which might
> be less work at runtime), and perhaps keeping-the-more-severe-one
> might be more useful than either of these two, but I didn't think
> it through.
>

> +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;
> +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
> +		reason = "reference already exists";
> +		break;
> +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
> +		reason = "reference does not exist";
> +		break;
> +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
> +		reason = "incorrect old value provided";
> +		break;
> +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
> +		reason = "invalid new value provided";
> +		break;
> +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
> +		reason = "expected symref but found regular ref";
> +		break;
> +	default:
> +		reason = "unkown failure";
> +	}
> +
> +	return xstrdup(reason);
> +}

> On Thu, May 15, 2025 at 04:07:25PM +0200, Karthik Nayak wrote:
>
>> +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;
>> +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
>> +		reason = "reference already exists";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
>> +		reason = "reference does not exist";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
>> +		reason = "incorrect old value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
>> +		reason = "invalid new value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
>> +		reason = "expected symref but found regular ref";
>> +		break;
>> +	default:
>> +		reason = "unkown failure";
>> +	}
>> +
>> +	return xstrdup(reason);
>> +}
>
> The assignment of "" is dead code, I think? We will always assign
> "unknown failure" as a last resort. Not a big deal, but just something I
> noticed while reading this related to what's going on in patch 4.
>

> (I guess the dead store of "" was present in the original, too, for that
> matter).
>
> -Peff
>
> PS Sorry for all the nit-picky comments. I was just going down the
>    Coverity rabbit hole and didn't really review the rest of the series.
>    But I wanted to say that the numbers you are seeing are very cool!

> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 2b1e336ba1..09b99143bf 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -575,30 +575,7 @@ static void print_rejected_refs(const char *refname,
>  				void *cb_data UNUSED)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> -	const char *reason = "";
> -
> -	switch (err) {
> -	case REF_TRANSACTION_ERROR_NAME_CONFLICT:
> -		reason = "refname conflict";
> -		break;
> -	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
> -		reason = "reference already exists";
> -		break;
> -	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
> -		reason = "reference does not exist";
> -		break;
> -	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
> -		reason = "incorrect old value provided";
> -		break;
> -	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
> -		reason = "invalid new value provided";
> -		break;
> -	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
> -		reason = "expected symref but found regular ref";
> -		break;
> -	default:
> -		reason = "unkown failure";
> -	}
> +	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,
> @@ -606,6 +583,7 @@ static void print_rejected_refs(const char *refname,
>  		    reason);
>  
>  	fwrite(sb.buf, sb.len, 1, stdout);
> +	free(reason);
>  	strbuf_release(&sb);
>  }

> +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;
> +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
> +		reason = "reference already exists";
> +		break;
> +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
> +		reason = "reference does not exist";
> +		break;
> +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
> +		reason = "incorrect old value provided";
> +		break;
> +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
> +		reason = "invalid new value provided";
> +		break;
> +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
> +		reason = "expected symref but found regular ref";
> +		break;
> +	default:
> +		reason = "unkown failure";
> +	}
> +
> +	return xstrdup(reason);
> +}

>> +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;
>> +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
>> +		reason = "reference already exists";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
>> +		reason = "reference does not exist";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
>> +		reason = "incorrect old value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
>> +		reason = "invalid new value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
>> +		reason = "expected symref but found regular ref";
>> +		break;
>> +	default:
>> +		reason = "unkown failure";
>> +	}
>> +
>> +	return xstrdup(reason);
>> +}
>
> Why can't this return "const char *", without xstrdup()?

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 5279997c96..15eac2b1c2 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>  	return result;
>  }
>  
> +struct ref_rejection_data {
> +	int *retcode;
> +	int conflict_msg_shown;
> +	const char *remote_name;
> +};
> +
> +static void ref_transaction_rejection_handler(const char *refname,
> +					      const struct object_id *old_oid UNUSED,
> +					      const struct object_id *new_oid UNUSED,
> +					      const char *old_target UNUSED,
> +					      const char *new_target UNUSED,
> +					      enum ref_transaction_error err,
> +					      void *cb_data)
> +{
> +	struct ref_rejection_data *data = (struct ref_rejection_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"
> +			" 'git remote prune %s' to remove any old, conflicting "
> +			"branches"), data->remote_name);
> +		data->conflict_msg_shown = 1;
> +	} else {
> +		char *reason = ref_transaction_error_msg(err);
> +
> +		error(_("fetching ref %s failed: %s"), refname, reason);
> +		free(reason);
> +	}
> +
> +	*data->retcode = 1;
> +}

> @@ -1808,6 +1795,24 @@ static int do_fetch(struct transport *transport,
>  			retcode = 1;
>  	}
>  
> +	/*
> +	 * If not atomic, we can still use batched updates, which would be much
> +	 * more performant. We don't initiate the transaction before pruning,
> +	 * since pruning must be an independent step, to avoid F/D conflicts.
> +	 *
> +	 * 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.

> On Thu, May 15, 2025 at 04:07:26PM +0200, Karthik Nayak wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 5279997c96..15eac2b1c2 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>>  	return result;
>>  }
>>
>> +struct ref_rejection_data {
>> +	int *retcode;
>> +	int conflict_msg_shown;
>> +	const char *remote_name;
>> +};
>> +
>> +static void ref_transaction_rejection_handler(const char *refname,
>> +					      const struct object_id *old_oid UNUSED,
>> +					      const struct object_id *new_oid UNUSED,
>> +					      const char *old_target UNUSED,
>> +					      const char *new_target UNUSED,
>> +					      enum ref_transaction_error err,
>> +					      void *cb_data)
>> +{
>> +	struct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;
>
> Nit: unnecessary cast.
>
>> +	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
>> +		error(_("some local refs could not be updated; try running\n"
>> +			" 'git remote prune %s' to remove any old, conflicting "
>> +			"branches"), data->remote_name);
>> +		data->conflict_msg_shown = 1;
>> +	} else {
>> +		char *reason = ref_transaction_error_msg(err);
>> +
>> +		error(_("fetching ref %s failed: %s"), refname, reason);
>> +		free(reason);
>> +	}
>> +
>> +	*data->retcode = 1;
>> +}
>
> Okay, we stopped ignoring generic errors now and will print them. What
> I'm still unclear about: which exact errors do we accept now that
> `REF_TRANSACTION_ALLOW_FAILURE` is specified? Most of the error codes we
> probably want to accept, but what about `REF_TRANSACTION_ERROR_GENERIC`?

>
> This makes me wonder a bit about the current layout of how we handle
> these errors. If the rejection handler was invoked while preparing the
> transaction for each reference as we go instead of afterwards we could
> decide on-the-fly whether a specific error should be ignored or not.
> That might lead to a design that is both more flexible and more obvious
> at the same time because error handling is now handled explicitly by the
> callsite that wants to ignore some errors.
>

> Last but not least, I think that it would also allow us to decide ahead
> of time whether we want to commit. Right now we basically say "just
> commit it, whatever happens". But if I'm not mistaken, all the errors
> that we care about and that callers may want to ignore are already
> detected at prepare time. So if we already bubbled up relevant info
> while calling `ref_transaction_prepare()` the caller may then decide to
> not commit at all based on some criteria.
>

>> @@ -1808,6 +1795,24 @@ static int do_fetch(struct transport *transport,
>>  			retcode = 1;
>>  	}
>>
>> +	/*
>> +	 * If not atomic, we can still use batched updates, which would be much
>> +	 * more performant. We don't initiate the transaction before pruning,
>> +	 * since pruning must be an independent step, to avoid F/D conflicts.
>> +	 *
>> +	 * 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.
>
> s/about/above/?
>

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Thu, May 15, 2025 at 04:07:26PM +0200, Karthik Nayak wrote:
> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
> >> index 5279997c96..15eac2b1c2 100644
> >> --- a/builtin/fetch.c
> >> +++ b/builtin/fetch.c
> >> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
> >>  	return result;
> >>  }
> >>
> >> +struct ref_rejection_data {
> >> +	int *retcode;
> >> +	int conflict_msg_shown;
> >> +	const char *remote_name;
> >> +};
> >> +
> >> +static void ref_transaction_rejection_handler(const char *refname,
> >> +					      const struct object_id *old_oid UNUSED,
> >> +					      const struct object_id *new_oid UNUSED,
> >> +					      const char *old_target UNUSED,
> >> +					      const char *new_target UNUSED,
> >> +					      enum ref_transaction_error err,
> >> +					      void *cb_data)
> >> +{
> >> +	struct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;
> >
> > Nit: unnecessary cast.
> >
> >> +	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
> >> +		error(_("some local refs could not be updated; try running\n"
> >> +			" 'git remote prune %s' to remove any old, conflicting "
> >> +			"branches"), data->remote_name);
> >> +		data->conflict_msg_shown = 1;
> >> +	} else {
> >> +		char *reason = ref_transaction_error_msg(err);
> >> +
> >> +		error(_("fetching ref %s failed: %s"), refname, reason);
> >> +		free(reason);
> >> +	}
> >> +
> >> +	*data->retcode = 1;
> >> +}
> >
> > Okay, we stopped ignoring generic errors now and will print them. What
> > I'm still unclear about: which exact errors do we accept now that
> > `REF_TRANSACTION_ALLOW_FAILURE` is specified? Most of the error codes we
> > probably want to accept, but what about `REF_TRANSACTION_ERROR_GENERIC`?
> 
> The current mechanism in `ref_transaction_maybe_set_rejected()` doesn't
> handle `REF_TRANSACTION_ERROR_GENERIC` errors. This was a design choice
> (more of a requirement of what this error represents), where
> `REF_TRANSACTION_ERROR_GENERIC` errors cannot be resolved on an
> individual reference level. It includes:
> 
>   - System errors such as I/O errors
>   - Duplicates present
> 
> Both of these represent issues which are bigger than a single ref
> update, so we have to propagate these errors up.

> > This makes me wonder a bit about the current layout of how we handle
> > these errors. If the rejection handler was invoked while preparing the
> > transaction for each reference as we go instead of afterwards we could
> > decide on-the-fly whether a specific error should be ignored or not.
> > That might lead to a design that is both more flexible and more obvious
> > at the same time because error handling is now handled explicitly by the
> > callsite that wants to ignore some errors.
> >
> 
> I did ponder on this while I was building the batched transaction
> mechanism. I decided to take it iteratively. We can, for instance,
> modify `ref_transaction_maybe_set_rejected()` to work with a callback
> function which would allow the users to accept/reject errors.
> 
> However, even if we go down that route, `REF_TRANSACTION_ERROR_GENERIC`
> errors still cannot be overlooked, these errors will abort the entire
> transaction.

> That said, I'm not trying to avoid going down that route. I do agree
> with the flexibility it does provide. Once we hit such a usecase, we
> should make that change.
> 
> For 'git-fetch(1)' and 'git-recieve-pack(1)', do you see a usecase?

> > Last but not least, I think that it would also allow us to decide ahead
> > of time whether we want to commit. Right now we basically say "just
> > commit it, whatever happens". But if I'm not mistaken, all the errors
> > that we care about and that callers may want to ignore are already
> > detected at prepare time. So if we already bubbled up relevant info
> > while calling `ref_transaction_prepare()` the caller may then decide to
> > not commit at all based on some criteria.
> >
> 
> Indeed, that is correct. I can confirm that even now all the calls to
> `ref_transaction_maybe_set_rejected()` are made in the prepare phase, so
> we could already do this, since `transaction->rejections` is already
> populated at this stage.

> On Fri, May 16, 2025 at 02:53:22AM -0700, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > On Thu, May 15, 2025 at 04:07:26PM +0200, Karthik Nayak wrote:
>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> >> index 5279997c96..15eac2b1c2 100644
>> >> --- a/builtin/fetch.c
>> >> +++ b/builtin/fetch.c
>> >> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>> >>  	return result;
>> >>  }
>> >>
>> >> +struct ref_rejection_data {
>> >> +	int *retcode;
>> >> +	int conflict_msg_shown;
>> >> +	const char *remote_name;
>> >> +};
>> >> +
>> >> +static void ref_transaction_rejection_handler(const char *refname,
>> >> +					      const struct object_id *old_oid UNUSED,
>> >> +					      const struct object_id *new_oid UNUSED,
>> >> +					      const char *old_target UNUSED,
>> >> +					      const char *new_target UNUSED,
>> >> +					      enum ref_transaction_error err,
>> >> +					      void *cb_data)
>> >> +{
>> >> +	struct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;
>> >
>> > Nit: unnecessary cast.
>> >
>> >> +	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
>> >> +		error(_("some local refs could not be updated; try running\n"
>> >> +			" 'git remote prune %s' to remove any old, conflicting "
>> >> +			"branches"), data->remote_name);
>> >> +		data->conflict_msg_shown = 1;
>> >> +	} else {
>> >> +		char *reason = ref_transaction_error_msg(err);
>> >> +
>> >> +		error(_("fetching ref %s failed: %s"), refname, reason);
>> >> +		free(reason);
>> >> +	}
>> >> +
>> >> +	*data->retcode = 1;
>> >> +}
>> >
>> > Okay, we stopped ignoring generic errors now and will print them. What
>> > I'm still unclear about: which exact errors do we accept now that
>> > `REF_TRANSACTION_ALLOW_FAILURE` is specified? Most of the error codes we
>> > probably want to accept, but what about `REF_TRANSACTION_ERROR_GENERIC`?
>>
>> The current mechanism in `ref_transaction_maybe_set_rejected()` doesn't
>> handle `REF_TRANSACTION_ERROR_GENERIC` errors. This was a design choice
>> (more of a requirement of what this error represents), where
>> `REF_TRANSACTION_ERROR_GENERIC` errors cannot be resolved on an
>> individual reference level. It includes:
>>
>>   - System errors such as I/O errors
>>   - Duplicates present
>>
>> Both of these represent issues which are bigger than a single ref
>> update, so we have to propagate these errors up.
>
> The second case is also why the behaviour changes now, right? If we were
> able to handle duplicates via the same mechanism then it would become
> possible to retain current behaviour for git-receive-pack(1)?
>

>> > This makes me wonder a bit about the current layout of how we handle
>> > these errors. If the rejection handler was invoked while preparing the
>> > transaction for each reference as we go instead of afterwards we could
>> > decide on-the-fly whether a specific error should be ignored or not.
>> > That might lead to a design that is both more flexible and more obvious
>> > at the same time because error handling is now handled explicitly by the
>> > callsite that wants to ignore some errors.
>> >
>>
>> I did ponder on this while I was building the batched transaction
>> mechanism. I decided to take it iteratively. We can, for instance,
>> modify `ref_transaction_maybe_set_rejected()` to work with a callback
>> function which would allow the users to accept/reject errors.
>>
>> However, even if we go down that route, `REF_TRANSACTION_ERROR_GENERIC`
>> errors still cannot be overlooked, these errors will abort the entire
>> transaction.
>
> Okay, good.
>
>> That said, I'm not trying to avoid going down that route. I do agree
>> with the flexibility it does provide. Once we hit such a usecase, we
>> should make that change.
>>
>> For 'git-fetch(1)' and 'git-recieve-pack(1)', do you see a usecase?
>
> No, I don't right now. I just want to avoid that we have to eventually
> refactor all of this to support an alternative API. But agreed, there
> isn't really much of a reason why we wouldn't be able to introduce such
> a mechanism retroactively while keeping existing callers intact.
>
> So let's stick with what we have and keep this in the back of our minds
> if we ever need such a mechanism going forward.
>

>> > Last but not least, I think that it would also allow us to decide ahead
>> > of time whether we want to commit. Right now we basically say "just
>> > commit it, whatever happens". But if I'm not mistaken, all the errors
>> > that we care about and that callers may want to ignore are already
>> > detected at prepare time. So if we already bubbled up relevant info
>> > while calling `ref_transaction_prepare()` the caller may then decide to
>> > not commit at all based on some criteria.
>> >
>>
>> Indeed, that is correct. I can confirm that even now all the calls to
>> `ref_transaction_maybe_set_rejected()` are made in the prepare phase, so
>> we could already do this, since `transaction->rejections` is already
>> populated at this stage.
>
> Good. After all, we shouldn't have to perform checks in the "commit"
> phase. Things are locked, things have been checked, so it should
> basically be a mere "let's move everything into place now". Which of
> course can still fail, but the only valid reason should be system
> failures.
>
> Patrick
>

> +failure:
> +	for (cmd = commands; cmd; cmd = cmd->next) {
> +		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));
>  	}

> On Thu, May 15, 2025 at 04:07:28PM +0200, Karthik Nayak wrote:
> 
> > +failure:
> > +	for (cmd = commands; cmd; cmd = cmd->next) {
> > +		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));
> >  	}

> On Thu, May 15, 2025 at 02:55:36PM -0400, Jeff King wrote:
>
>> On Thu, May 15, 2025 at 04:07:28PM +0200, Karthik Nayak wrote:
>>
>> > +failure:
>> > +	for (cmd = commands; cmd; cmd = cmd->next) {
>> > +		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));
>> >  	}
>
> BTW, one other funny thing about this code: we duplicate the strings
> that we assign to cmd->error_string. But no other call path does so. So
> now we have allocated memory, but nobody can ever free() it because
> often it is not allocated!
>
> I'm surprised LSan does not report a leak here, so I might be missing
> something.
>

> It looks like there is some magic in the struct here to handle this
> like:
>
>   cmd->error_string = cmd->error_string_owned = xstrdup(...);
>
> which would solve it. But I wonder: do these need to be allocated at
> all? We are pulling out strings owned by the strmap, which will free
> them. So as-is, yes, we need to make our own copies.
>
> But does the strmap need to own them in the first place? It is taking
> the values from ref_transaction_error_msg(). That returns an allocated
> string, but it doesn't need to. It could just return the string literals
> directly, which are valid for the life of the program. That would save
> other callers from having to call free(), though it does mean we have to
> cast away the constness when putting them into the strmap (since it
> accepts a non-const void pointer). Something like:
>

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 4a3c46eca7..cf0bcf1ad0 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1665,10 +1665,9 @@ static void ref_transaction_rejection_handler(const char *refname,
>  			"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;
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index bd0fb729ff..4cef2ddd3d 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1855,7 +1855,7 @@ static void ref_transaction_rejection_handler(const char *refname,
>  {
>  	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,
> @@ -1897,15 +1897,16 @@ static void execute_commands_non_atomic(struct command *commands,
>
>  failure:
>  	for (cmd = commands; cmd; cmd = cmd->next) {
> +		const char *reason;
>  		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));
> +		else if ((reason = strmap_get(&failed_refs, cmd->ref_name)))
> +			cmd->error_string = reason;
>  	}
>
>  cleanup:
>  	ref_transaction_free(transaction);
> -	strmap_clear(&failed_refs, 1);
> +	strmap_clear(&failed_refs, 0);
>  	strbuf_release(&err);
>  }
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 09b99143bf..1e6131e04a 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -575,15 +575,14 @@ static void print_rejected_refs(const char *refname,
>  				void *cb_data UNUSED)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> -	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,
>  		    old_oid ? oid_to_hex(old_oid) : old_target,
>  		    reason);
>
>  	fwrite(sb.buf, sb.len, 1, stdout);
> -	free(reason);
>  	strbuf_release(&sb);
>  }
>
> diff --git a/refs.c b/refs.c
> index 351ed52deb..953f83bb52 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3315,7 +3315,7 @@ 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 = "";
>
> @@ -3342,5 +3342,5 @@ char *ref_transaction_error_msg(enum ref_transaction_error err)
>  		reason = "unkown failure";
>  	}
>
> -	return xstrdup(reason);
> +	return reason;
>  }
> diff --git a/refs.h b/refs.h
> index a0b2e3c43d..2d58af3d88 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -910,7 +910,7 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio
>  /*
>   * 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.
>
> -Peff