| Message ID | patch-06.12-295594fe8ae-20210720T102051Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix "git reflog expire" race & get rid of EISDIR in refs API | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > static void reflog_expiry_prepare(const char *refname, > - const struct object_id *oid, > + struct object_id *locked_oid, > void *cb_data) > { > struct expire_reflog_policy_cb *cb = cb_data; > @@ -361,7 +361,7 @@ static void reflog_expiry_prepare(const char *refname, > cb->unreachable_expire_kind = UE_HEAD; > } else { > cb->tip_commit = lookup_commit_reference_gently(the_repository, > - oid, 1); > + locked_oid, 1); > if (!cb->tip_commit) > cb->unreachable_expire_kind = UE_ALWAYS; > else > diff --git a/refs.h b/refs.h > index 48970dfc7e0..c009707438d 100644 > --- a/refs.h > +++ b/refs.h > @@ -796,7 +796,7 @@ enum expire_reflog_flags { > * expiration policy that is desired. > * > * reflog_expiry_prepare_fn -- Called once after the reference is > - * locked. > + * locked. Called with the OID of the locked reference. > * > * reflog_expiry_should_prune_fn -- Called once for each entry in the > * existing reflog. It should return true iff that entry should be > @@ -806,7 +806,7 @@ enum expire_reflog_flags { > * unlocked again. > */ > typedef void reflog_expiry_prepare_fn(const char *refname, > - const struct object_id *oid, > + struct object_id *lock_oid, > void *cb_data); The files_reflog_expire() helper still takes const oid but it can tolerate this change because it uses the lockfile API as its implementation detail, and passes &(lock->old_oid) (which is no longer a const). But do we expect that all backends to take an on-filesystem lock via the lockfile API and pass it down to prepare_fn? This obviously breaks the latest round of reftable topic, as it still wants this type to take const oid, and I do not think using on-filesystem lock as if we are using the files backend is not a good solution.
Junio C Hamano <gitster@pobox.com> writes: > This obviously breaks the latest round of reftable topic, as it > still wants this type to take const oid, and I do not think using > on-filesystem lock as if we are using the files backend is not a > good solution. Sorry for redundant negation. "I do not think it is a good solution to have everybody pretend as if they are files backend when they lock refs." was what I meant.
On Wed, Jul 21 2021, Han-Wen Nienhuys wrote: > On Wed, Jul 21, 2021 at 7:48 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > This obviously breaks the latest round of reftable topic, as it > > still wants this type to take const oid, and I do not think using > > on-filesystem lock as if we are using the files backend is not a > > good solution. > > Sorry for redundant negation. "I do not think it is a good solution > to have everybody pretend as if they are files backend when they > lock refs." was what I meant. > > Reftable could easily read the current OID for the reference, if necessary. (I'm replying to a mail of Han-Wen's that didn't make it on-list due to inline HTML, quoted here in its entirety sans signature, see https://lore.kernel.org/git/87eebptr7i.fsf@evledraar.gmail.com/) Junio: I can change the const around if desired. I thought we weren't particularly concerned about it in general except to avoid the verbosity of frequent casting, and in this case the lock API doesn't have "const". But as for the reftable incompatibility it seems to me irrespective of backend that a reflog API that supports expiry is going to want to have a callback for "give me a lock to expire this branch" and give you a reply of "OK, you have the lock, you can expire the log, and it's at this OID". Why would it be file-backend specific?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Jul 21 2021, Han-Wen Nienhuys wrote: > >> On Wed, Jul 21, 2021 at 7:48 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Junio C Hamano <gitster@pobox.com> writes: >> >> > This obviously breaks the latest round of reftable topic, as it >> > still wants this type to take const oid, and I do not think using >> > on-filesystem lock as if we are using the files backend is not a >> > good solution. >> >> Sorry for redundant negation. "I do not think it is a good solution >> to have everybody pretend as if they are files backend when they >> lock refs." was what I meant. >> >> Reftable could easily read the current OID for the reference, if necessary. > > (I'm replying to a mail of Han-Wen's that didn't make it on-list due to > inline HTML, quoted here in its entirety sans signature, see > https://lore.kernel.org/git/87eebptr7i.fsf@evledraar.gmail.com/) > > Junio: I can change the const around if desired. I thought we weren't > particularly concerned about it in general except to avoid the verbosity > of frequent casting, and in this case the lock API doesn't have "const". > > But as for the reftable incompatibility it seems to me irrespective of > backend that a reflog API that supports expiry is going to want to have > a callback for "give me a lock to expire this branch" and give you a > reply of "OK, you have the lock, you can expire the log, and it's at > this OID". > > Why would it be file-backend specific? If you feed const oid to *_reflog_expire() method of any backend (including the ones that that are *not* files backend), and if you expect they all will use lockfile API to copy it into lock->old_oid so that it can be fed safely to prepare_fn(), then the arrangement for constness you have set up in your series would work out OK for everybody. But for any backend that does not use one-file-per-ref filesystem mapping, it is rather strange to use lockfile API for locking refs, no? THat is what I meant by files-backend specific.
On Fri, Jul 23 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Wed, Jul 21 2021, Han-Wen Nienhuys wrote: >> >>> On Wed, Jul 21, 2021 at 7:48 PM Junio C Hamano <gitster@pobox.com> wrote: >>> >>> Junio C Hamano <gitster@pobox.com> writes: >>> >>> > This obviously breaks the latest round of reftable topic, as it >>> > still wants this type to take const oid, and I do not think using >>> > on-filesystem lock as if we are using the files backend is not a >>> > good solution. >>> >>> Sorry for redundant negation. "I do not think it is a good solution >>> to have everybody pretend as if they are files backend when they >>> lock refs." was what I meant. >>> >>> Reftable could easily read the current OID for the reference, if necessary. >> >> (I'm replying to a mail of Han-Wen's that didn't make it on-list due to >> inline HTML, quoted here in its entirety sans signature, see >> https://lore.kernel.org/git/87eebptr7i.fsf@evledraar.gmail.com/) >> >> Junio: I can change the const around if desired. I thought we weren't >> particularly concerned about it in general except to avoid the verbosity >> of frequent casting, and in this case the lock API doesn't have "const". >> >> But as for the reftable incompatibility it seems to me irrespective of >> backend that a reflog API that supports expiry is going to want to have >> a callback for "give me a lock to expire this branch" and give you a >> reply of "OK, you have the lock, you can expire the log, and it's at >> this OID". >> >> Why would it be file-backend specific? > > If you feed const oid to *_reflog_expire() method of any backend > (including the ones that that are *not* files backend), and if you > expect they all will use lockfile API to copy it into lock->old_oid > so that it can be fed safely to prepare_fn(), then the arrangement > for constness you have set up in your series would work out OK for > everybody. But for any backend that does not use one-file-per-ref > filesystem mapping, it is rather strange to use lockfile API for > locking refs, no? THat is what I meant by files-backend specific. It won't be using the lockfile API, but as an implementation detail the non-const old_oid is where the "struct object id *" you get passed comes from in the file API. Is that what you mean by the behavior being file-backend specific?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> If you feed const oid to *_reflog_expire() method of any backend >> (including the ones that that are *not* files backend), and if you >> expect they all will use lockfile API to copy it into lock->old_oid >> so that it can be fed safely to prepare_fn(), then the arrangement >> for constness you have set up in your series would work out OK for >> everybody. But for any backend that does not use one-file-per-ref >> filesystem mapping, it is rather strange to use lockfile API for >> locking refs, no? THat is what I meant by files-backend specific. > > It won't be using the lockfile API, but as an implementation detail the > non-const old_oid is where the "struct object id *" you get passed comes > from in the file API. > > Is that what you mean by the behavior being file-backend specific? It wasn't that deep. It was just - The constness of the oids _expire() method of backends and _prepare() callback take used to be the same; - The recent files backend update made them different, because the callback made to _prepare() by the files backend wants to lose constness because files backend wants to pass the oid embedded in the lock object now. But do we expect other backends, like reftable, to also use lockfile API so that they will have a writable oid handy to pass their calls to _prepare()? I somehow doubted it. If they want to obtain a writable place because _prepare() callback now wants to take a writable oid, they can of course create an on-stack copy and pass it, but that feels to be working around this "files-backend centric" change. In any case, we seem to be focusing on an irrelevant tangent in this discussion; what made me feel that this change was files-backend centric does not matter much in the larger picture. What matters more now is that this change does not work well with Han-Wen's latest round, and we need to know what the final shape of these APIs are. Thanks.
diff --git a/builtin/reflog.c b/builtin/reflog.c index 09541d1c804..9f9e6bceb03 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -351,7 +351,7 @@ static int is_head(const char *refname) } static void reflog_expiry_prepare(const char *refname, - const struct object_id *oid, + struct object_id *locked_oid, void *cb_data) { struct expire_reflog_policy_cb *cb = cb_data; @@ -361,7 +361,7 @@ static void reflog_expiry_prepare(const char *refname, cb->unreachable_expire_kind = UE_HEAD; } else { cb->tip_commit = lookup_commit_reference_gently(the_repository, - oid, 1); + locked_oid, 1); if (!cb->tip_commit) cb->unreachable_expire_kind = UE_ALWAYS; else diff --git a/refs.h b/refs.h index 48970dfc7e0..c009707438d 100644 --- a/refs.h +++ b/refs.h @@ -796,7 +796,7 @@ enum expire_reflog_flags { * expiration policy that is desired. * * reflog_expiry_prepare_fn -- Called once after the reference is - * locked. + * locked. Called with the OID of the locked reference. * * reflog_expiry_should_prune_fn -- Called once for each entry in the * existing reflog. It should return true iff that entry should be @@ -806,7 +806,7 @@ enum expire_reflog_flags { * unlocked again. */ typedef void reflog_expiry_prepare_fn(const char *refname, - const struct object_id *oid, + struct object_id *lock_oid, void *cb_data); typedef int reflog_expiry_should_prune_fn(struct object_id *ooid, struct object_id *noid, diff --git a/refs/debug.c b/refs/debug.c index 449ac3e6cc8..18fd9bca595 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -364,12 +364,14 @@ struct debug_reflog_expiry_should_prune { }; static void debug_reflog_expiry_prepare(const char *refname, - const struct object_id *oid, + struct object_id *locked_oid, void *cb_data) { struct debug_reflog_expiry_should_prune *prune = cb_data; - trace_printf_key(&trace_refs, "reflog_expire_prepare: %s\n", refname); - prune->prepare(refname, oid, prune->cb_data); + trace_printf_key(&trace_refs, "reflog_expire_prepare: %s locket at %s\n", + refname, + oid_to_hex(locked_oid)); + prune->prepare(refname, locked_oid, prune->cb_data); } static int debug_reflog_expiry_should_prune_fn(struct object_id *ooid, diff --git a/refs/files-backend.c b/refs/files-backend.c index af332fa8fe4..ce4b3fb1c7a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3098,7 +3098,8 @@ static int files_reflog_expire(struct ref_store *ref_store, } } - (*prepare_fn)(refname, oid, cb.policy_cb); + assert(oideq(&lock->old_oid, oid)); + (*prepare_fn)(refname, &lock->old_oid, cb.policy_cb); refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb); (*cleanup_fn)(cb.policy_cb);
Don't pass the object ID we pass into reflog_expire() back to the caller, but rather our locked OID. As the assert shows these two were the same thing in practice as we'd exit earlier in this function if we couldn't lock the desired OID. This is in preparation for a subsequent change of not having reflog_expire() pass in the OID at all, also remove the "const" from the parameter since the "struct ref_lock" does not have it on its "old_oid" member. As we'll see in a subsequent commit we don't actually want to assert that we locked a given OID, we want this API to do the locking and tell us what the OID is, but for now let's just setup the basic scaffolding for that. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/reflog.c | 4 ++-- refs.h | 4 ++-- refs/debug.c | 8 +++++--- refs/files-backend.c | 3 ++- 4 files changed, 11 insertions(+), 8 deletions(-)