Subject: Re: [PATCH v8 1/6] refs: atomically record overwritten ref in update_symref

From: "Bence Ferdinandy" <[email-masked]>
Date: Tue, 15 Oct 2024 18:24:10 +0200

On Tue Oct 15, 2024 at 09:41, karthik nayak <[email-masked]> wrote:
> Bence Ferdinandy <[email-masked]> writes:
>
> [snip]
>
>> diff --git a/refs.c b/refs.c
>> index 5f729ed412..b964ac44d0 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2114,7 +2114,8 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct
>>  }
>>
>>  int refs_update_symref(struct ref_store *refs, const char *ref,
>> -		       const char *target, const char *logmsg)
>> +		       const char *target, const char *logmsg,
>> +		       struct strbuf *referent)
>>  {
>>  	struct ref_transaction *transaction;
>>  	struct strbuf err = STRBUF_INIT;
>> @@ -2122,17 +2123,23 @@ int refs_update_symref(struct ref_store *refs, const char *ref,
>>
>>  	transaction = ref_store_transaction_begin(refs, &err);
>>  	if (!transaction ||
>> -	    ref_transaction_update(transaction, ref, NULL, NULL,
>> +		ref_transaction_update(transaction, ref, NULL, NULL,
>>  				   target, NULL, REF_NO_DEREF,
>>  				   logmsg, &err) ||
>> -	    ref_transaction_commit(transaction, &err)) {
>> +		ref_transaction_prepare(transaction, &err)) {
>>  		ret = error("%s", err.buf);
>> +		goto cleanup;
>>  	}
>> +	if (referent)
>> +		refs_read_symbolic_ref(refs, ref, referent);
>
> Shouldn't we also check the return value here?

My reasoning was that if this fails referent will just look like as if it did
not exist. Since this is an addition to set-head and fetch failing to set the
HEAD in a case which would have previously worked I did not think it prudent to
now fail on this for any reason.

>
>> +
>> +	if (ref_transaction_commit(transaction, &err))
>> +		ret = error("%s", err.buf);
>>
>> +cleanup:
>>  	strbuf_release(&err);
>>  	if (transaction)
>>  		ref_transaction_free(transaction);
>> -
>>
>
> Why remove this whiteline?

Looks like a mistake made during rebase.

-- 
bence.ferdinandy.com