From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 10 Nov 2025 13:11:14 GMT
Subject: Re: [PATCH v3 1/2] fetch: extract out reference committing logic
Message-ID: <CAOLa=ZQLWF_GBtYXN9F=+=BwqugYOH=Z9OuNV1n3VmnH=rbqpA@mail.gmail.com>
In-Reply-To: <aRGVhA4eXnAFxvqE@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> 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`?
>

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