Thread (43 messages) 43 messages, 4 authors, 2025-12-02

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.

Patrick
I do agree here, and since the code is small, I think it is worthwhile
making this change. Will add in. Thanks

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help