From: Karthik Nayak Date: Fri, 16 May 2025 19:49:35 GMT Subject: Re: [PATCH v2 4/4] receive-pack: use batched reference updates Message-ID: In-Reply-To: <20250515190909.GA3320028@coredump.intra.peff.net> Jeff King 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