Re: [PATCH v3] refs: return conflict error when checking packed refs

From: Karthik Nayak <karthik.188@gmail.com>
Date: May 7, 2024, 13:37 UTC
Message-ID: <CAOLa=ZTu1to4HRc8sD2BcA+1ZzgcnXW9nHb1gg_VEG=nLOjHvg@mail.gmail.com>
In-Reply-To: <CA+WCWMyxBA90QRsxn-i0H1TRi7WrhRcD=Tgj07f_Y_=UVNkhbw@mail.gmail.com>

Hello Ivan,

Ivan Tse <ivan.tse1@gmail.com> writes:

Show 31 quoted lines

> On Mon, May 6, 2024 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>> >> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> >> index a098d14ea00..97473f377d1 100644
>> >> --- a/refs/files-backend.c
>> >> +++ b/refs/files-backend.c
>> >> @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs,
>> >>               */
>> >>              if (refs_verify_refname_available(
>> >>                                  refs->packed_ref_store, refname,
>> >> -                                extras, NULL, err))
>> >> +                                extras, NULL, err)) {
>> >> +                    ret = TRANSACTION_NAME_CONFLICT;
>> >>                      goto error_return;
>> >> +            }
>> >>      }
>> >>
>> >>      ret = 0;
>> >>
>> >
>> > Shouldn't we also do this change in `lock_ref_oid_basic` where we gather
>> > the same lock again for creating the reflog entry?
>>
>> An interesting question.
>
> Hi!
>
> Apologies but I'm not sure I follow - could you elaborate? I am not
> too familiar with the Git source code (or C language) but from looking
> at `lock_ref_oid_basic`, it looks like that function returns a lock
> struct and not an integer success/error code. I'm not sure how we
> would apply this change in that function as well?

That's correct, my question was merely that we also have a `refs_verify_refname_available` function call there and was wondering if we somehow need to catch and propagate the same error from there. Like you mentioned the way it is currently structured doesn't make that possible.