| Message ID | patch-08.11-1e25b7c59c5-20210716T140631Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix "git reflog expire" race & get rid of EISDIR in refs API | expand |
On Fri, Jul 16, 2021 at 04:13:04PM +0200, Ævar Arnfjörð Bjarmason wrote: > Add a comment about why it is that we need to check for the the s/the the// > existence of a reflog we're deleting after we've successfully acquired > the lock in files_reflog_expire(). As noted in [1] the lock protocol > for reflogs is somewhat intuitive. Did you mean unintuitive here? > diff --git a/refs/files-backend.c b/refs/files-backend.c > index ec9c70d79cc..f73637fa087 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -3068,6 +3068,17 @@ static int files_reflog_expire(struct ref_store *ref_store, > strbuf_release(&err); > return -1; > } > + > + /* > + * When refs are deleted their reflog is deleted before the > + * ref itself deleted. This race happens because there's no > + * 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/. -Peff
Jeff King <peff@peff.net> writes: > On Fri, Jul 16, 2021 at 04:13:04PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Add a comment about why it is that we need to check for the the > > s/the the// > >> existence of a reflog we're deleting after we've successfully acquired >> the lock in files_reflog_expire(). As noted in [1] the lock protocol >> for reflogs is somewhat intuitive. > > Did you mean unintuitive here? > >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index ec9c70d79cc..f73637fa087 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -3068,6 +3068,17 @@ static int files_reflog_expire(struct ref_store *ref_store, >> strbuf_release(&err); >> return -1; >> } >> + >> + /* >> + * When refs are deleted their reflog is deleted before the >> + * ref itself deleted. This race happens because there's no "... before the ref itself gets deleted", or some verb there. A comma in "When refs are deleted, there reflog is ..." would help making it more readable, too. >> + * 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? 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... > > -Peff
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
diff --git a/refs/files-backend.c b/refs/files-backend.c index ec9c70d79cc..f73637fa087 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3068,6 +3068,17 @@ static int files_reflog_expire(struct ref_store *ref_store, strbuf_release(&err); return -1; } + + /* + * When refs are deleted their reflog is deleted before the + * ref itself deleted. This race happens because there's no + * 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;
Add a comment about why it is that we need to check for the the existence of a reflog we're deleting after we've successfully acquired the lock in files_reflog_expire(). As noted in [1] the lock protocol for reflogs is somewhat intuitive. This early exit code the comment applies to dates all the way back to 4264dc15e19 (git reflog expire, 2006-12-19). 1. https://lore.kernel.org/git/54DCDA42.2060800@alum.mit.edu/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- refs/files-backend.c | 11 +++++++++++ 1 file changed, 11 insertions(+)