Re: [PATCH v2 08/11] refs/files: add a comment about refs_reflog_exists() call
- From
Jeff King <peff@peff.net>
- Date
- Jul 20, 2021, 07:22 UTC
- Message-ID
- <YPZ5mzknK/nzKNCh@coredump.intra.peff.net>
- In-Reply-To
- <xmqqa6mintjx.fsf@gitster.g>
On Mon, Jul 19, 2021 at 09:43:14AM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> >> + * such thing as a lock on the reflog, instead we always lock
> >> + * the "loose ref" (even if packet) above with
> >> + * lock_ref_oid_basic().
> >> + *
> >> + * If race happens we've got nothing more to do, we were asked
> >> + * to delete the reflog, and it's not there anymore. Great!
> >> + */
> >> if (!refs_reflog_exists(ref_store, refname)) {
> >> unlock_ref(lock);
> >> return 0;
> >
> > I think everything is accurate here, though I wouldn't have made the
> > distinction with "locking the loose ref". There is no such thing as
> > locking a packed ref; we always take the loose lock.
> >
> > s/packet/packed/, and maybe s/If race/If a race/.
>
> Meaning, there is no such thing as locking a packed ref or a loose
> ref, we just lock a "ref" and the way it is done in files backend is
> by creating a lockfile as if we are creating/updating a loose one?Yes, exactly. We do take a lock on the packed-refs file sometimes, but I generally think "lock the ref" always means to take refs/heads/foo.lock in the filesystem. Probably not all that important, but if we need a re-roll to clarify language anyway...:)
Show 13 quoted lines
> I do agree that the second sentence needs rewriting to make it less > confusing. lock_ref_oid_basic() being the way to lock "a ref" is > not limited to cases where we want to do something to do to the > reflog. > > Taking all together, perhaps: > > When refs are deleted, their reflog is deleted before the > ref itself is deleted. This is because there is no separate > lock for reflog---instead we take a lock on the ref with > lock_ref_oid_basic(). > > If a race happens we've got nothing more to do...
Yeah, that reads fine to me.
-Peff