| Message ID | patch-02.11-033c0cec33d-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:12:58PM +0200, Ævar Arnfjörð Bjarmason wrote: > The lock_ref_oid_basic() function has gradually been replaced by use > of the file transaction API, there are only 4 remaining callers of > it. Should this be "ref transaction API"? Ditto in the next patch. Other than that small nit, the both look like very nice cleanups. -Peff
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The lock_ref_oid_basic() function has gradually been replaced by use > of the file transaction API, there are only 4 remaining callers of > it. Peff mentioned "file, not ref?", and I wonder about the same thing. Inside the implementation of ref transaction (e.g. the codepath that calls lock_ref_for_update() from files_transaction_prepare()), we do end up calling the lockfile API and you could (but I'd prefer not to see you do so) call it "file transaction API", but I think you meant that most callers no longer perform low-level "acquire lock, update and release" and instead use the ref transaction. > None of those callers pass REF_DELETING, the last such caller went > away in 8df4e511387 (struct ref_update: move "have_old" into "flags", > 2015-02-17). This is the start of even more removal of unused code in > and around this function. While I agree that no existing calls to lock_ref_oid_basic() pass REF_DELETING to it (hence this patch is a benign no-op), inside ref_transaction_commit(), 8df4e511387 still used REF_DELETING, I think, like so: /* Acquire all locks while verifying old values */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; unsigned int flags = update->flags; if (is_null_sha1(update->new_sha1)) flags |= REF_DELETING; update->lock = lock_ref_sha1_basic( update->refname, ((update->flags & REF_HAVE_OLD) ? update->old_sha1 : NULL), NULL, flags, &update->type); > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > refs/files-backend.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 677b7e4cdd2..326f0224218 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -934,8 +934,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > > if (mustexist) > resolve_flags |= RESOLVE_REF_READING; > - if (flags & REF_DELETING) > - resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; > > files_ref_path(refs, &ref_file, refname); > resolved = !!refs_resolve_ref_unsafe(&refs->base,
On Mon, Jul 19, 2021 at 09:16:49AM -0700, Junio C Hamano wrote: > > None of those callers pass REF_DELETING, the last such caller went > > away in 8df4e511387 (struct ref_update: move "have_old" into "flags", > > 2015-02-17). This is the start of even more removal of unused code in > > and around this function. > > While I agree that no existing calls to lock_ref_oid_basic() pass > REF_DELETING to it (hence this patch is a benign no-op), inside > ref_transaction_commit(), 8df4e511387 still used REF_DELETING, I > think, like so: > > /* Acquire all locks while verifying old values */ > for (i = 0; i < n; i++) { > struct ref_update *update = updates[i]; > unsigned int flags = update->flags; > > if (is_null_sha1(update->new_sha1)) > flags |= REF_DELETING; > update->lock = lock_ref_sha1_basic( > update->refname, > ((update->flags & REF_HAVE_OLD) ? > update->old_sha1 : NULL), > NULL, > flags, > &update->type); Good catch. That code got moved to refs/files-backend.c in 7bd9bcf372 (refs: split filesystem-based refs code into a new file, 2015-11-09), and then pulled into a function in 165056b2fc (lock_ref_for_update(): new function, 2016-04-24). And then finally in 92b1551b1d (refs: resolve symbolic refs first, 2016-04-25), we replaced the call to lock_ref_sha1_basic() with lock_raw_ref(). So I think that final one is when the parameter actually became obsolete (so the patch is still good, just a minor history inaccuracy). -Peff
diff --git a/refs/files-backend.c b/refs/files-backend.c index 677b7e4cdd2..326f0224218 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -934,8 +934,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, if (mustexist) resolve_flags |= RESOLVE_REF_READING; - if (flags & REF_DELETING) - resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; files_ref_path(refs, &ref_file, refname); resolved = !!refs_resolve_ref_unsafe(&refs->base,
The lock_ref_oid_basic() function has gradually been replaced by use of the file transaction API, there are only 4 remaining callers of it. None of those callers pass REF_DELETING, the last such caller went away in 8df4e511387 (struct ref_update: move "have_old" into "flags", 2015-02-17). This is the start of even more removal of unused code in and around this function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- refs/files-backend.c | 2 -- 1 file changed, 2 deletions(-)