Re: [PATCH v2 4/4] receive-pack: use batched reference updates

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5563 bytes --]

Jeff King <peff@peff.net> writes:

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

Yeah, indeed that is a leak and surprising that this wasn't caught by
LSan.

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

Thanks Jeff, this does make a lot of sense, I think with Junio's
suggestion in the first patch, making `ref_transaction_error_msg()`
return a 'const char *' solves this issue too, we no longer will hold
allocated strings here and as such no longer have to worry about
allocation/freeing of memory. Much cleaner.

In the end it looks similar to you patch below!

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]