[PATCH v2 00/11] fix "git reflog expire" race & get rid of EISDIR in refs API

This is a follow-up to the much smaller one-patch v1: https://lore.kernel.org/git/patch-1.1-de0838fe99-20210714T111351Z-avarab@gmail.com/

As noted in the discussion there there's a potential loose end with one of the 4 callers of lock_ref_oid_basic().

I'd forgotten that I fixed a bug in 2019 by removing the OID call to that one caller[1]. It didn't get integrated for no particularly good reason, had some "prep" series it depended on, it looked good to reviewers, but I just forgot to pursue it after the prep patches landed.

That patch has been running in a large production for a long time, and as far as I know still is. The version of it we end up with here is the same in all the important ways, I just clean up the immediate caller as well (and there's some prep patches for that).

There's still some loose ends left in builtin/reflog.c after that, but it's not important for correctness. I've got a 7-patch series post-this for that, hopefully I'll remember to submit it this time.

1. https://lore.kernel.org/git/875yxczbd6.fsf@evledraar.gmail.com/

Ævar Arnfjörð Bjarmason (11):
  refs/packet: add missing BUG() invocations to reflog callbacks
  refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
  refs/files: remove unused "extras/skip" in lock_ref_oid_basic()
  refs/files: remove unused "skip" in lock_raw_ref() too
  refs/debug: re-indent argument list for "prepare"
  refs API: pass the "lock OID" to reflog "prepare"
  refs: make repo_dwim_log() accept a NULL oid
  refs/files: add a comment about refs_reflog_exists() call
  reflog expire: don't lock reflogs using previously seen OID
  refs/files: remove unused "oid" in lock_ref_oid_basic()
  refs/files: remove unused "errno == EISDIR" code

 builtin/reflog.c      |  17 +++---
 reflog-walk.c         |   3 +-
 refs.c                |   5 +-
 refs.h                |   4 +-
 refs/debug.c          |  10 ++--
 refs/files-backend.c  | 122 ++++++++++--------------------------------
 refs/packed-backend.c |   5 ++
 7 files changed, 54 insertions(+), 112 deletions(-)

Range-diff against v1:
 -:  ----------- >  1:  30bd7679a5c refs/packet: add missing BUG() invocations to reflog callbacks
 -:  ----------- >  2:  033c0cec33d refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
 -:  ----------- >  3:  94ffcd8cfda refs/files: remove unused "extras/skip" in lock_ref_oid_basic()
 -:  ----------- >  4:  61f9e0fc864 refs/files: remove unused "skip" in lock_raw_ref() too
 -:  ----------- >  5:  95101c322b7 refs/debug: re-indent argument list for "prepare"
 -:  ----------- >  6:  e93465f4137 refs API: pass the "lock OID" to reflog "prepare"
 -:  ----------- >  7:  0fff2d32cfc refs: make repo_dwim_log() accept a NULL oid
 -:  ----------- >  8:  1e25b7c59c5 refs/files: add a comment about refs_reflog_exists() call
 -:  ----------- >  9:  60d6cf342fc reflog expire: don't lock reflogs using previously seen OID
 -:  ----------- > 10:  09dd8836437 refs/files: remove unused "oid" in lock_ref_oid_basic()
 1:  de0838fe996 ! 11:  96c3e5e9f79 refs file backend: remove dead "errno == EISDIR" code
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    refs file backend: remove dead "errno == EISDIR" code
    +    refs/files: remove unused "errno == EISDIR" code
     
         When we lock a reference like "foo" we need to handle the case where
         "foo" exists, but is an empty directory. That's what this code added
    @@ Commit message
         remove_empty_directories() and continue.
     
         Since a1c1d8170d (refs_resolve_ref_unsafe: handle d/f conflicts for
    -    writes, 2017-10-06) we don't, because our our callstack will look
    -    something like:
    +    writes, 2017-10-06) we don't. E.g. in the case of
    +    files_copy_or_rename_ref() our callstack will look something like:
     
    -        files_copy_or_rename_ref() -> lock_ref_oid_basic() -> refs_resolve_ref_unsafe()
    +            [...] ->
    +            files_copy_or_rename_ref() ->
    +            lock_ref_oid_basic() ->
    +            refs_resolve_ref_unsafe()
     
    -    And then the refs_resolve_ref_unsafe() call here will in turn (in the
    -    code added in a1c1d8170d) do the equivalent of this (via a call to
    -    refs_read_raw_ref()):
    +    At that point the first (now only) refs_resolve_ref_unsafe() call in
    +    lock_ref_oid_basic() would do the equivalent of this in the resulting
    +    call to refs_read_raw_ref() in refs_resolve_ref_unsafe():
     
                 /* Via refs_read_raw_ref() */
                 fd = open(path, O_RDONLY);
    @@ Commit message
         We then proceed with the entire ref update and don't call
         remove_empty_directories() until we call commit_ref_update(). See
         5387c0d883 (commit_ref(): if there is an empty dir in the way, delete
    -    it, 2016-05-05) for the addition of that code.
    +    it, 2016-05-05) for the addition of that code, and
    +    a1c1d8170db (refs_resolve_ref_unsafe: handle d/f conflicts for writes,
    +    2017-10-06) for the commit that changed the original codepath added in
    +    bc7127ef0f to use this "EISDIR" handling.
    +
    +    Further historical commentary:
    +
    +    Before the two preceding commits the caller in files_reflog_expire()
    +    was the only one out of our 4 callers that would pass non-NULL as an
    +    oid. We would then set a (now gone) "resolve_flags" to
    +    "RESOLVE_REF_READING" and just before that "errno != EISDIR" check do:
    +
    +            if (resolve_flags & RESOLVE_REF_READING)
    +                    return NULL;
    +
    +    There may have been some case where this ended up mattering and we
    +    couldn't safely make this change before we removed the "oid"
    +    parameter, but I don't think there was, see [1] for some discussion on
    +    that.
    +
    +    In any case, now that we've removed the "oid" parameter in a preceding
    +    commit we can be sure that this code is redundant, so let's remove it.
    +
    +    1. http://lore.kernel.org/git/871r801yp6.fsf@evledraar.gmail.com
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## refs/files-backend.c ##
     @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
    - 	resolved = !!refs_resolve_ref_unsafe(&refs->base,
    - 					     refname, resolve_flags,
    - 					     &lock->old_oid, type);
    + 	struct strbuf ref_file = STRBUF_INIT;
    + 	struct ref_lock *lock;
    + 	int last_errno = 0;
    +-	int resolved;
    + 
    + 	files_assert_main_repository(refs, "lock_ref_oid_basic");
    + 	assert(err);
    +@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
    + 	CALLOC_ARRAY(lock, 1);
    + 
    + 	files_ref_path(refs, &ref_file, refname);
    +-	resolved = !!refs_resolve_ref_unsafe(&refs->base, refname,
    +-					     RESOLVE_REF_NO_RECURSE,
    +-					     &lock->old_oid, type);
     -	if (!resolved && errno == EISDIR) {
     -		/*
     -		 * we are trying to lock foo but we used to
    @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_re
     -			last_errno = errno;
     -			if (!refs_verify_refname_available(
     -					    &refs->base,
    --					    refname, extras, skip, err))
    +-					    refname, NULL, NULL, err))
     -				strbuf_addf(err, "there are still refs under '%s'",
     -					    refname);
     -			goto error_return;
     -		}
    --		resolved = !!refs_resolve_ref_unsafe(&refs->base,
    --						     refname, resolve_flags,
    +-		resolved = !!refs_resolve_ref_unsafe(&refs->base, refname,
    +-						     RESOLVE_REF_NO_RECURSE,
     -						     &lock->old_oid, type);
     -	}
    - 	if (!resolved) {
    +-	if (!resolved) {
    ++	if (!refs_resolve_ref_unsafe(&refs->base, refname,
    ++				     RESOLVE_REF_NO_RECURSE,
    ++				     &lock->old_oid, type)) {
      		last_errno = errno;
      		if (last_errno != ENOTDIR ||
    + 		    !refs_verify_refname_available(&refs->base, refname,