Re: [PATCH v3 1/2] fetch: extract out reference committing logic

[Patrick Steinhardt <ps@pks.im>]

On Sat, Nov 08, 2025 at 10:34:43PM +0100, Karthik Nayak wrote:
> 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`?

	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