Re: [PATCH v3 1/2] fetch: extract out reference committing logic
From: Karthik Nayak <hidden>
Date: 2025-11-10 13:11:16
Patrick Steinhardt [off-list ref] writes:
On Sat, Nov 08, 2025 at 10:34:43PM +0100, Karthik Nayak wrote:quoted
diff --git a/builtin/fetch.c b/builtin/fetch.c index c7ff3480fb..49e195199e 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c@@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname, *data->retcode = 1; } +/* + * Commit the reference transaction. If it isn't an atomic transaction, handle + * rejected updates as part of using batched updates. + */ +static int commit_ref_transaction(struct ref_transaction **transaction, + bool is_atomic, const char *remote_name, + struct strbuf *err) +{ + int retcode = ref_transaction_commit(*transaction, err); + if (retcode) { + /* + * Explicitly handle transaction cleanup to avoid + * aborting an already closed transaction. + */ + ref_transaction_free(*transaction); + *transaction = NULL; + } + + if (*transaction && !is_atomic) {This condition is somewhat weird, as we know that it won't ever execute if `retcode` is non-zero. So wouldn't the function be way easier to follow if you turned the above conditional into a `goto out`?
I don't have any arguments here, I basically simply moved the code as is and didn't want to make changes to reduce the review load.
static int commit_ref_transaction(struct ref_transaction **transaction,
bool is_atomic, const char *remote_name,
struct strbuf *err)
{
int retcode;
retcode = ref_transaction_commit(*transaction, err);
if (retcode)
goto out;
if (!is_atomic) {
struct ref_rejection_data data = {
.conflict_msg_shown = 0,
.remote_name = remote_name,
.retcode = &retcode,
};
ref_transaction_for_each_rejected_update(*transaction,
ref_transaction_rejection_handler,
&data);
}
out:
ref_transaction_free(*transaction);
*transaction = NULL;
return retcode;
}
This feels significantly easier to read to me.
PatrickI do agree here, and since the code is small, I think it is worthwhile making this change. Will add in. Thanks
Attachments
- signature.asc [application/pgp-signature] 690 bytes