Re: [PATCH v8 1/6] refs: atomically record overwritten ref in update_symref
- From
Bence Ferdinandy <bence@ferdinandy.com>
- Date
- Oct 15, 2024, 16:24 UTC
- Message-ID
- <D4WIP42OSINN.VGZI7YIMMK8S@ferdinandy.com>
- In-Reply-To
- <CAOLa=ZTMzz-9XE8AY8vk7Gpbm+U0T-5jWNJzKLyd621_URiDEA@mail.gmail.com>
On Tue Oct 15, 2024 at 09:41, karthik nayak <karthik.188@gmail.com> wrote:
Show 35 quoted lines
> Bence Ferdinandy <bence@ferdinandy.com> 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.
Show 13 quoted lines
>
>> +
>> + 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