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

From: Jeff King <peff@peff.net>
Date: May 15, 2025, 19:09 UTC
Message-ID: <20250515190909.GA3320028@coredump.intra.peff.net>
In-Reply-To: <20250515185535.GA3309052@coredump.intra.peff.net>

On Thu, May 15, 2025 at 02:55:36PM -0400, Jeff King wrote:

Show 9 quoted lines

> 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