Re: [PATCH v2 08/11] refs/files: add a comment about refs_reflog_exists() call

[Jeff King <peff@peff.net>]

On Mon, Jul 19, 2021 at 09:43:14AM -0700, Junio C Hamano wrote:

> >> +	 * 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...:)

> 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