[PATCH] refs file backend: remove dead "errno == EISDIR" code

[PATCH] refs file backend: remove dead "errno == EISDIR" code

[Ævar Arnfjörð Bjarmason]

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
in bc7127ef0f (ref locking: allow 'foo' when 'foo/bar' used to exist
but not anymore., 2006-09-30) seems like it should be dealing with.

Except it doesn't, and we never take this branch. The reason is that
when bc7127ef0f was written this looked like:

	ref = resolve_ref([...]);
	if (!ref && errno == EISDIR) {
	[...]

And in resolve_ref() we had this code:

	fd = open(path, O_RDONLY);
	if (fd < 0)
		return NULL;

I.e. we would attempt to read "foo" with open(), which would fail with
EISDIR and we'd return NULL. We'd then take this branch, call
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:

    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()):

	/* Via refs_read_raw_ref() */
	fd = open(path, O_RDONLY);
	if (fd < 0)
		/* get errno == EISDIR */
	/* later, in refs_resolve_ref_unsafe() */
	if ([...] && errno != EISDIR)
		return NULL;
	[...]
	/* returns the refs/heads/foo to the caller, even though it's a directory */
	return refname;

I.e. even though we got an "errno == EISDIR" we won't take this
branch, since in cases of EISDIR "resolved" is always
non-NULL. I.e. we pretend at this point as though everything's OK and
there is no "foo" directory.

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

While working on a re-roll of the review/helping out Han-Wen with the
refs backend at [1] I discovered that this codepath in
lock_ref_oid_basic() has been unused for some time. In that series I
added a BUG() to it[2], but it's even better to remove it entirely.

I'll submit any proposed re-roll of [1] on top of this, I thought it
was better to review this isolated and more easily understood change
on its own.

1. https://lore.kernel.org/git/cover-00.17-00000000000-20210711T162803Z-avarab@gmail.com
2. https://lore.kernel.org/git/patch-07.17-10a40c9244e-20210711T162803Z-avarab@gmail.com/

 refs/files-backend.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 677b7e4cdd..7e4963fd07 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -941,26 +941,6 @@ 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);
-	if (!resolved && errno == EISDIR) {
-		/*
-		 * we are trying to lock foo but we used to
-		 * have foo/bar which now does not exist;
-		 * it is normal for the empty directory 'foo'
-		 * to remain.
-		 */
-		if (remove_empty_directories(&ref_file)) {
-			last_errno = errno;
-			if (!refs_verify_refname_available(
-					    &refs->base,
-					    refname, extras, skip, err))
-				strbuf_addf(err, "there are still refs under '%s'",
-					    refname);
-			goto error_return;
-		}
-		resolved = !!refs_resolve_ref_unsafe(&refs->base,
-						     refname, resolve_flags,
-						     &lock->old_oid, type);
-	}
 	if (!resolved) {
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
-- 
2.32.0.851.g0fc62a9785

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Jeff King]

On Wed, Jul 14, 2021 at 01:17:14PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 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:
> 
>     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()):
> 
> 	/* Via refs_read_raw_ref() */
> 	fd = open(path, O_RDONLY);
> 	if (fd < 0)
> 		/* get errno == EISDIR */
> 	/* later, in refs_resolve_ref_unsafe() */
> 	if ([...] && errno != EISDIR)
> 		return NULL;
> 	[...]
> 	/* returns the refs/heads/foo to the caller, even though it's a directory */
> 	return refname;

Isn't that pseudo-code missing a conditional that's there in the real
code? In refs_resolve_ref_unsafe(), I see:

       if (refs_read_raw_ref(refs, refname,
                             oid, &sb_refname, &read_flags)) {
               *flags |= read_flags;

               /* In reading mode, refs must eventually resolve */
               if (resolve_flags & RESOLVE_REF_READING)
                       return NULL;

               /*
                * Otherwise a missing ref is OK. But the files backend
                * may show errors besides ENOENT if there are
                * similarly-named refs.
                */
               if (errno != ENOENT &&
                   errno != EISDIR &&
                   errno != ENOTDIR)
                       return NULL;

So if RESOLVE_REF_READING is set, we can return NULL immediately, with
errno set to EISDIR. Which contradicts this:

> I.e. even though we got an "errno == EISDIR" we won't take this
> branch, since in cases of EISDIR "resolved" is always
> non-NULL. I.e. we pretend at this point as though everything's OK and
> there is no "foo" directory.

So when is RESOLVE_REF_READING set? The resolve_flags parameter is
passed in by the caller. In lock_ref_oid_basic(), it comes from this:

    int mustexist = (old_oid && !is_null_oid(old_oid));
    [...]
    if (mustexist)
            resolve_flags |= RESOLVE_REF_READING;

So do any callers pass in old_oid? Surprisingly few. It used to be
called from other locking functions, but these days it looks like it is
only files_reflog_expire().

I'm not sure if this case is important or not. If we're expecting the
ref to exist, then an in-the-way directory is going to mean failure
either way. It could still exist within the packed-refs file, but then
refs_read_raw_ref() would not return failure.

So...I think it's fine? But the argument in your commit message seems to
have missed this case entirely.

-Peff

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 14 2021, Jeff King wrote:

> On Wed, Jul 14, 2021 at 01:17:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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:
>> 
>>     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()):
>> 
>> 	/* Via refs_read_raw_ref() */
>> 	fd = open(path, O_RDONLY);
>> 	if (fd < 0)
>> 		/* get errno == EISDIR */
>> 	/* later, in refs_resolve_ref_unsafe() */
>> 	if ([...] && errno != EISDIR)
>> 		return NULL;
>> 	[...]
>> 	/* returns the refs/heads/foo to the caller, even though it's a directory */
>> 	return refname;
>
> Isn't that pseudo-code missing a conditional that's there in the real
> code? In refs_resolve_ref_unsafe(), I see:
>
>        if (refs_read_raw_ref(refs, refname,
>                              oid, &sb_refname, &read_flags)) {
>                *flags |= read_flags;
>
>                /* In reading mode, refs must eventually resolve */
>                if (resolve_flags & RESOLVE_REF_READING)
>                        return NULL;
>
>                /*
>                 * Otherwise a missing ref is OK. But the files backend
>                 * may show errors besides ENOENT if there are
>                 * similarly-named refs.
>                 */
>                if (errno != ENOENT &&
>                    errno != EISDIR &&
>                    errno != ENOTDIR)
>                        return NULL;
>
> So if RESOLVE_REF_READING is set, we can return NULL immediately, with
> errno set to EISDIR. Which contradicts this:

I opted (perhaps unwisely) to elide that since as you note above we
don't take that path in relation to the removed code. I.e. I'm
describing the relevant codepath we take nowadays given the code & its
callers.

But will reword etc., thanks.

>> I.e. even though we got an "errno == EISDIR" we won't take this
>> branch, since in cases of EISDIR "resolved" is always
>> non-NULL. I.e. we pretend at this point as though everything's OK and
>> there is no "foo" directory.
>
> So when is RESOLVE_REF_READING set? The resolve_flags parameter is
> passed in by the caller. In lock_ref_oid_basic(), it comes from this:
>
>     int mustexist = (old_oid && !is_null_oid(old_oid));
>     [...]
>     if (mustexist)
>             resolve_flags |= RESOLVE_REF_READING;
>
> So do any callers pass in old_oid? Surprisingly few. It used to be
> called from other locking functions, but these days it looks like it is
> only files_reflog_expire().

In general (and not being too familiar with this area) and per:

    7521cc4611 (refs.c: make delete_ref use a transaction, 2014-04-30)
    92b1551b1d (refs: resolve symbolic refs first, 2016-04-25)
    029cdb4ab2 (refs.c: make prune_ref use a transaction to delete the ref, 2014-04-30)

And:

    https://lore.kernel.org/git/20140902205841.GA18279@google.com/    

I wonder if these remaining cases can be migrated over to lock_raw_ref()
or the transaction API, as many other similar callers have been already.

But that's a bigger change, I won't be doing that now, just wondering if
these are some #leftoverbits or if there's a good reason they were left.

> I'm not sure if this case is important or not. If we're expecting the
> ref to exist, then an in-the-way directory is going to mean failure
> either way. It could still exist within the packed-refs file, but then
> refs_read_raw_ref() would not return failure.
>
> So...I think it's fine? But the argument in your commit message seems to
> have missed this case entirely.

Perhaps more succinctly: If we have a directory in the way, it's going
to be impossible for the "old_oid" condition to be satisfied in any case
in the file backend.

Even if we still had a caller that did "care" about that what could they
hope to get from an "old_oid=<some-OID>" for a lock on "foo/bar" where
"foo" is an empty directory?

Except of course for the case where it's not a directory but packed, but
as you noted that's handled in another case.

Perhaps it's informative that the below diff-on-top also passes all
tests, i.e. that we have largely the same
"refs_read_raw_ref(refs->packed_ref_store" copy/pasted in
files_read_raw_ref() in two adjacent places, we're just changing what
errno we pass upwards.

It thoroughly tramples on Han-Wen's series, and it's easier to deal with
(if at all) once his lands, just thought it might be interesting:

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7e4963fd07..4a97cd48d9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -356,6 +356,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	int ret = -1;
 	int save_errno;
 	int remaining_retries = 3;
+	int lstat_bad_or_not_file = 0;
+	int lstat_errno = 0;
 
 	*type = 0;
 	strbuf_reset(&sb_path);
@@ -382,11 +384,28 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 		goto out;
 
 	if (lstat(path, &st) < 0) {
-		if (errno != ENOENT)
+		lstat_bad_or_not_file = 1;
+		lstat_errno = errno;
+	} else if (S_ISDIR(st.st_mode)) {
+		/*
+		 * Maybe it's an empty directory, maybe it's not, in
+		 * either case this ref does not exist in the files
+		 * backend (but may be packet), later code will handle
+		 * the "create and maybe remove_empty_directories()"
+		 * case if needed, or die otherwise.
+		 */
+		lstat_bad_or_not_file = 1;
+	}
+
+	if (lstat_bad_or_not_file) {
+		if (lstat_errno && lstat_errno != ENOENT)
 			goto out;
 		if (refs_read_raw_ref(refs->packed_ref_store, refname,
 				      oid, referent, type)) {
-			errno = ENOENT;
+			if (lstat_errno)
+				errno = ENOENT;
+			else
+				errno = EISDIR;
 			goto out;
 		}
 		ret = 0;
@@ -417,22 +436,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 		 */
 	}
 
-	/* Is it a directory? */
-	if (S_ISDIR(st.st_mode)) {
-		/*
-		 * Even though there is a directory where the loose
-		 * ref is supposed to be, there could still be a
-		 * packed ref:
-		 */
-		if (refs_read_raw_ref(refs->packed_ref_store, refname,
-				      oid, referent, type)) {
-			errno = EISDIR;
-			goto out;
-		}
-		ret = 0;
-		goto out;
-	}
-
 	/*
 	 * Anything else, just open it and try to use it as
 	 * a ref

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Jeff King]

On Wed, Jul 14, 2021 at 09:07:41PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Isn't that pseudo-code missing a conditional that's there in the real
> > code? In refs_resolve_ref_unsafe(), I see:
> >
> >        if (refs_read_raw_ref(refs, refname,
> >                              oid, &sb_refname, &read_flags)) {
> >                *flags |= read_flags;
> >
> >                /* In reading mode, refs must eventually resolve */
> >                if (resolve_flags & RESOLVE_REF_READING)
> >                        return NULL;
> >
> >                /*
> >                 * Otherwise a missing ref is OK. But the files backend
> >                 * may show errors besides ENOENT if there are
> >                 * similarly-named refs.
> >                 */
> >                if (errno != ENOENT &&
> >                    errno != EISDIR &&
> >                    errno != ENOTDIR)
> >                        return NULL;
> >
> > So if RESOLVE_REF_READING is set, we can return NULL immediately, with
> > errno set to EISDIR. Which contradicts this:
> 
> I opted (perhaps unwisely) to elide that since as you note above we
> don't take that path in relation to the removed code. I.e. I'm
> describing the relevant codepath we take nowadays given the code & its
> callers.

It's not clear to me that we don't take that path, though. The call in
files_reflog_expire() looks like it violates the assertion in your
commit message (that we would never return NULL with errno as EISDIR).

So I'm not entirely sure that the code is in fact dead (though I
couldn't find an easy way to trigger it from the command line). I do
think it probably can't do anything useful, and it is probably still OK
to delete. But in my mind that is quite a different argument.

Maybe that is splitting hairs, but I definitely try to err on the side
of caution and over-analysis when touching tricky code (and the
ref-backend code is in my experience one of the trickiest spots for
corner cases, races, etc).

> > So when is RESOLVE_REF_READING set? The resolve_flags parameter is
> > passed in by the caller. In lock_ref_oid_basic(), it comes from this:
> >
> >     int mustexist = (old_oid && !is_null_oid(old_oid));
> >     [...]
> >     if (mustexist)
> >             resolve_flags |= RESOLVE_REF_READING;
> >
> > So do any callers pass in old_oid? Surprisingly few. It used to be
> > called from other locking functions, but these days it looks like it is
> > only files_reflog_expire().
> 
> In general (and not being too familiar with this area) and per:
> 
>     7521cc4611 (refs.c: make delete_ref use a transaction, 2014-04-30)
>     92b1551b1d (refs: resolve symbolic refs first, 2016-04-25)
>     029cdb4ab2 (refs.c: make prune_ref use a transaction to delete the ref, 2014-04-30)
> 
> And:
> 
>     https://lore.kernel.org/git/20140902205841.GA18279@google.com/    
> 
> I wonder if these remaining cases can be migrated over to lock_raw_ref()
> or the transaction API, as many other similar callers have been already.
> 
> But that's a bigger change, I won't be doing that now, just wondering if
> these are some #leftoverbits or if there's a good reason they were left.

Quite possibly. It's been a while since I've looked this deep at the ref
code. It is weird that only one remaining caller passes old_oid. If even
that one could be converted, the whole lock_ref_oid_basic() could be
simplified a bit.

I agree that's a bigger change, so it might make sense to do smaller
cleanups in the interim.

> > So...I think it's fine? But the argument in your commit message seems to
> > have missed this case entirely.
> 
> Perhaps more succinctly: If we have a directory in the way, it's going
> to be impossible for the "old_oid" condition to be satisfied in any case
> in the file backend.
> 
> Even if we still had a caller that did "care" about that what could they
> hope to get from an "old_oid=<some-OID>" for a lock on "foo/bar" where
> "foo" is an empty directory?
> 
> Except of course for the case where it's not a directory but packed, but
> as you noted that's handled in another case.

Yeah, I think that's reasonably compelling. It's possible there are some
races unaccounted for here (like somebody else creating and deleting
shared-prefix loose refs at the same time), but it may be OK to just
accept those. The code is "we saw a failure, see if deleting stale
directories helps". And if the worst case is that this doesn't kick in
an obscure race (where we'd probably end up failing the whole operation
anyway), that's OK.

-Peff

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 14 2021, Jeff King wrote:

> On Wed, Jul 14, 2021 at 09:07:41PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > Isn't that pseudo-code missing a conditional that's there in the real
>> > code? In refs_resolve_ref_unsafe(), I see:
>> >
>> >        if (refs_read_raw_ref(refs, refname,
>> >                              oid, &sb_refname, &read_flags)) {
>> >                *flags |= read_flags;
>> >
>> >                /* In reading mode, refs must eventually resolve */
>> >                if (resolve_flags & RESOLVE_REF_READING)
>> >                        return NULL;
>> >
>> >                /*
>> >                 * Otherwise a missing ref is OK. But the files backend
>> >                 * may show errors besides ENOENT if there are
>> >                 * similarly-named refs.
>> >                 */
>> >                if (errno != ENOENT &&
>> >                    errno != EISDIR &&
>> >                    errno != ENOTDIR)
>> >                        return NULL;
>> >
>> > So if RESOLVE_REF_READING is set, we can return NULL immediately, with
>> > errno set to EISDIR. Which contradicts this:
>> 
>> I opted (perhaps unwisely) to elide that since as you note above we
>> don't take that path in relation to the removed code. I.e. I'm
>> describing the relevant codepath we take nowadays given the code & its
>> callers.
>
> It's not clear to me that we don't take that path, though. The call in
> files_reflog_expire() looks like it violates the assertion in your
> commit message (that we would never return NULL with errno as EISDIR).
>
> So I'm not entirely sure that the code is in fact dead (though I
> couldn't find an easy way to trigger it from the command line). I do
> think it probably can't do anything useful, and it is probably still OK
> to delete. But in my mind that is quite a different argument.
>
> Maybe that is splitting hairs, but I definitely try to err on the side
> of caution and over-analysis when touching tricky code (and the
> ref-backend code is in my experience one of the trickiest spots for
> corner cases, races, etc).

I'd entirely forgotten about this, but I had a patch to remove that
"oid" call entirely, as it's really an unrelated bug/undesired behavior

You also looked at it at the time & Michael Haggerty reviewed it:
https://lore.kernel.org/git/20190315155959.12390-9-avarab@gmail.com/

The state of that patch was that I needed to get to some minor issues
with it (commit message rewording, cleaning up some related callers),
but the fundamental approach seemed good.

I then split it off from the v4 of that series to get the non-tricky
changes in:
https://lore.kernel.org/git/20190328161434.19200-1-avarab@gmail.com/

I then just never got to picking it up again, I'll probably re-roll it &
make it a part of this series, then we can remove this whole OID != NULL
case and will be sure nothing fishy's going on.

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Jeff King]

On Thu, Jul 15, 2021 at 02:02:40AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Maybe that is splitting hairs, but I definitely try to err on the side
> > of caution and over-analysis when touching tricky code (and the
> > ref-backend code is in my experience one of the trickiest spots for
> > corner cases, races, etc).
> 
> I'd entirely forgotten about this, but I had a patch to remove that
> "oid" call entirely, as it's really an unrelated bug/undesired behavior
> 
> You also looked at it at the time & Michael Haggerty reviewed it:
> https://lore.kernel.org/git/20190315155959.12390-9-avarab@gmail.com/
> 
> The state of that patch was that I needed to get to some minor issues
> with it (commit message rewording, cleaning up some related callers),
> but the fundamental approach seemed good.
> 
> I then split it off from the v4 of that series to get the non-tricky
> changes in:
> https://lore.kernel.org/git/20190328161434.19200-1-avarab@gmail.com/
> 
> I then just never got to picking it up again, I'll probably re-roll it &
> make it a part of this series, then we can remove this whole OID != NULL
> case and will be sure nothing fishy's going on.

Yeah, that sounds like a good path forward. I do think the patch under
discussion here is probably the right thing to do. But it becomes all
the more obvious if lock_ref_oid_basic() ends up dropping that parameter
entirely.

-Peff

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jul 15, 2021 at 02:02:40AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > Maybe that is splitting hairs, but I definitely try to err on the side
>> > of caution and over-analysis when touching tricky code (and the
>> > ref-backend code is in my experience one of the trickiest spots for
>> > corner cases, races, etc).
>> 
>> I'd entirely forgotten about this, but I had a patch to remove that
>> "oid" call entirely, as it's really an unrelated bug/undesired behavior
>> 
>> You also looked at it at the time & Michael Haggerty reviewed it:
>> https://lore.kernel.org/git/20190315155959.12390-9-avarab@gmail.com/
>> 
>> The state of that patch was that I needed to get to some minor issues
>> with it (commit message rewording, cleaning up some related callers),
>> but the fundamental approach seemed good.
>> 
>> I then split it off from the v4 of that series to get the non-tricky
>> changes in:
>> https://lore.kernel.org/git/20190328161434.19200-1-avarab@gmail.com/
>> 
>> I then just never got to picking it up again, I'll probably re-roll it &
>> make it a part of this series, then we can remove this whole OID != NULL
>> case and will be sure nothing fishy's going on.
>
> Yeah, that sounds like a good path forward. I do think the patch under
> discussion here is probably the right thing to do. But it becomes all
> the more obvious if lock_ref_oid_basic() ends up dropping that parameter
> entirely.

OK, so what's the final verdict on this step?  It is unfortunate
that when Ævar took over a topic from Han-Wen, this patch has been
inserted as the very first step before the patches in the series, so
until we know we are happy with it, it takes several other patches
hostage.

Thanks.

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Jeff King]

On Fri, Jul 16, 2021 at 06:28:06PM -0700, Junio C Hamano wrote:

> >> I then just never got to picking it up again, I'll probably re-roll it &
> >> make it a part of this series, then we can remove this whole OID != NULL
> >> case and will be sure nothing fishy's going on.
> >
> > Yeah, that sounds like a good path forward. I do think the patch under
> > discussion here is probably the right thing to do. But it becomes all
> > the more obvious if lock_ref_oid_basic() ends up dropping that parameter
> > entirely.
> 
> OK, so what's the final verdict on this step?  It is unfortunate
> that when Ævar took over a topic from Han-Wen, this patch has been
> inserted as the very first step before the patches in the series, so
> until we know we are happy with it, it takes several other patches
> hostage.

I just read through v2. Modulo a few small nits (mostly typos, but a few
commit message suggestions), it looks good to me. I agree it's a lot to
stick in front of another set of patches, but I think in this case we
can proceed quickly enough to make it worth doing in the order Ævar
suggests.

-Peff

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 16, 2021 at 06:28:06PM -0700, Junio C Hamano wrote:
>
>> >> I then just never got to picking it up again, I'll probably re-roll it &
>> >> make it a part of this series, then we can remove this whole OID != NULL
>> >> case and will be sure nothing fishy's going on.
>> >
>> > Yeah, that sounds like a good path forward. I do think the patch under
>> > discussion here is probably the right thing to do. But it becomes all
>> > the more obvious if lock_ref_oid_basic() ends up dropping that parameter
>> > entirely.
>> 
>> OK, so what's the final verdict on this step?  It is unfortunate
>> that when Ævar took over a topic from Han-Wen, this patch has been
>> inserted as the very first step before the patches in the series, so
>> until we know we are happy with it, it takes several other patches
>> hostage.
>
> I just read through v2. Modulo a few small nits (mostly typos, but a few
> commit message suggestions), it looks good to me. I agree it's a lot to
> stick in front of another set of patches, but I think in this case we
> can proceed quickly enough to make it worth doing in the order Ævar
> suggests.

I did not check the v2 thouroughly myself, but read it enough to
convince me that it would be good preliminary clean-up steps to come
before the main series (modulo typoes and nits, as you pointed out).

Thanks.

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I did not check the v2 thouroughly myself, but read it enough to
> convince me that it would be good preliminary clean-up steps to come
> before the main series (modulo typoes and nits, as you pointed out).

I've came to the same conclusion as you did.  The patches themselves
are good, modulo typos and some inaccuracies in the comment string
and commit log messages, that would not hurt immediate execution of
the resulting code but they need updates to help future developers
who need to interact with these commits and its results.

Thanks.

Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 16 2021, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
>> On Thu, Jul 15, 2021 at 02:02:40AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>> > Maybe that is splitting hairs, but I definitely try to err on the side
>>> > of caution and over-analysis when touching tricky code (and the
>>> > ref-backend code is in my experience one of the trickiest spots for
>>> > corner cases, races, etc).
>>> 
>>> I'd entirely forgotten about this, but I had a patch to remove that
>>> "oid" call entirely, as it's really an unrelated bug/undesired behavior
>>> 
>>> You also looked at it at the time & Michael Haggerty reviewed it:
>>> https://lore.kernel.org/git/20190315155959.12390-9-avarab@gmail.com/
>>> 
>>> The state of that patch was that I needed to get to some minor issues
>>> with it (commit message rewording, cleaning up some related callers),
>>> but the fundamental approach seemed good.
>>> 
>>> I then split it off from the v4 of that series to get the non-tricky
>>> changes in:
>>> https://lore.kernel.org/git/20190328161434.19200-1-avarab@gmail.com/
>>> 
>>> I then just never got to picking it up again, I'll probably re-roll it &
>>> make it a part of this series, then we can remove this whole OID != NULL
>>> case and will be sure nothing fishy's going on.
>>
>> Yeah, that sounds like a good path forward. I do think the patch under
>> discussion here is probably the right thing to do. But it becomes all
>> the more obvious if lock_ref_oid_basic() ends up dropping that parameter
>> entirely.
>
> OK, so what's the final verdict on this step?  It is unfortunate
> that when Ævar took over a topic from Han-Wen, this patch has been
> inserted as the very first step before the patches in the series, so
> until we know we are happy with it, it takes several other patches
> hostage.

I'm just interested in the end result landing sooner than later, so if
you think this re-imagining of it hinders more than helps I'm happy to
discard it and just take the last version Han-Wen submitted, i.e. the
v5:
https://lore.kernel.org/git/pull.1012.v5.git.git.1625684869.gitgitgadget@gmail.com/

I can then re-roll whatever I've come up here that I still find useful
on that after it lands.

I just thought that given the complexity of the ref code tying any loose
ends up before those changes would help, but maybe not.

Anyway, you/Han-Wen let me know. I'm happy to re-roll what I have
outstanding here based on feedback, but also just to discard it for now
and go with his version. I'll hold on any re-rolls in that area pending
feedback on what you two would like to do.

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

[Ævar Arnfjörð Bjarmason]

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,
-- 
2.32.0.873.gb6f2f696497

[PATCH v2 01/11] refs/packet: add missing BUG() invocations to reflog callbacks

[Ævar Arnfjörð Bjarmason]

In e0cc8ac8202 (packed_ref_store: make class into a subclass of
`ref_store`, 2017-06-23) a die() was added to packed_create_reflog(),
but not to any of the other reflog callbacks, let's do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/packed-backend.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index dfecdbc1db6..53d1ec27b60 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1600,6 +1600,7 @@ static int packed_for_each_reflog_ent(struct ref_store *ref_store,
 				      const char *refname,
 				      each_reflog_ent_fn fn, void *cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1608,12 +1609,14 @@ static int packed_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 					      each_reflog_ent_fn fn,
 					      void *cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
 static int packed_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1627,6 +1630,7 @@ static int packed_create_reflog(struct ref_store *ref_store,
 static int packed_delete_reflog(struct ref_store *ref_store,
 			       const char *refname)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1638,6 +1642,7 @@ static int packed_reflog_expire(struct ref_store *ref_store,
 				reflog_expiry_cleanup_fn cleanup_fn,
 				void *policy_cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
-- 
2.32.0.873.gb6f2f696497

[PATCH v2 02/11] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

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(-)

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,
-- 
2.32.0.873.gb6f2f696497

Re: [PATCH v2 02/11] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

[Jeff King]

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

Re: [PATCH v2 02/11] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

[Junio C Hamano]

Æ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,

Re: [PATCH v2 02/11] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

[Jeff King]

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

[PATCH v2 03/11] refs/files: remove unused "extras/skip" in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

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 non-NULL "extras" and "skip" parameters,
the last such caller went away in 92b1551b1d4 (refs: resolve symbolic
refs first, 2016-04-25), so let's remove the parameters.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 326f0224218..a59823d667e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -915,8 +915,6 @@ static int create_reflock(const char *path, void *cb)
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
 					   const struct object_id *old_oid,
-					   const struct string_list *extras,
-					   const struct string_list *skip,
 					   unsigned int flags, int *type,
 					   struct strbuf *err)
 {
@@ -950,7 +948,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 			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;
@@ -963,7 +961,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
 		    !refs_verify_refname_available(&refs->base, refname,
-						   extras, skip, err))
+						   NULL, NULL, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
 				    refname, strerror(last_errno));
 
@@ -978,7 +976,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	 */
 	if (is_null_oid(&lock->old_oid) &&
 	    refs_verify_refname_available(refs->packed_ref_store, refname,
-					  extras, skip, err)) {
+					  NULL, NULL, err)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -1413,8 +1411,8 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
-	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL,
-				  REF_NO_DEREF, NULL, &err);
+	lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
+				  &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1436,7 +1434,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	goto out;
 
  rollback:
-	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL,
+	lock = lock_ref_oid_basic(refs, oldrefname, NULL,
 				  REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
@@ -1845,7 +1843,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	int ret;
 
 	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  NULL, NULL, REF_NO_DEREF, NULL,
+				  REF_NO_DEREF, NULL,
 				  &err);
 	if (!lock) {
 		error("%s", err.buf);
@@ -3064,7 +3062,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference if --updateref was specified:
 	 */
 	lock = lock_ref_oid_basic(refs, refname, oid,
-				  NULL, NULL, REF_NO_DEREF,
+				  REF_NO_DEREF,
 				  &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
-- 
2.32.0.873.gb6f2f696497

[PATCH v2 04/11] refs/files: remove unused "skip" in lock_raw_ref() too

[Ævar Arnfjörð Bjarmason]

Remove the unused "skip" parameter to lock_raw_ref(), it was never
used. We do use it when passing "skip" to the
refs_rename_ref_available() function in files_copy_or_rename_ref(),
but not here.

This is part of a larger series that modifies lock_ref_oid_basic()
extensively, there will be no more modifications of this function in
this series, but since the preceding commit removed this unused
parameter from lock_ref_oid_basic(), let's do it here too for
consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a59823d667e..af332fa8fe4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -531,7 +531,6 @@ static void unlock_ref(struct ref_lock *lock)
 static int lock_raw_ref(struct files_ref_store *refs,
 			const char *refname, int mustexist,
 			const struct string_list *extras,
-			const struct string_list *skip,
 			struct ref_lock **lock_p,
 			struct strbuf *referent,
 			unsigned int *type,
@@ -568,7 +567,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		 * reason to expect this error to be transitory.
 		 */
 		if (refs_verify_refname_available(&refs->base, refname,
-						  extras, skip, err)) {
+						  extras, NULL, err)) {
 			if (mustexist) {
 				/*
 				 * To the user the relevant error is
@@ -673,7 +672,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 							  REMOVE_DIR_EMPTY_ONLY)) {
 				if (refs_verify_refname_available(
 						    &refs->base, refname,
-						    extras, skip, err)) {
+						    extras, NULL, err)) {
 					/*
 					 * The error message set by
 					 * verify_refname_available() is OK.
@@ -710,7 +709,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		 */
 		if (refs_verify_refname_available(
 				    refs->packed_ref_store, refname,
-				    extras, skip, err))
+				    extras, NULL, err))
 			goto error_return;
 	}
 
@@ -2412,7 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	}
 
 	ret = lock_raw_ref(refs, update->refname, mustexist,
-			   affected_refnames, NULL,
+			   affected_refnames,
 			   &lock, &referent,
 			   &update->type, err);
 	if (ret) {
-- 
2.32.0.873.gb6f2f696497

[PATCH v2 05/11] refs/debug: re-indent argument list for "prepare"

[Ævar Arnfjörð Bjarmason]

Re-indent this argument list that's been mis-indented since it was
added in 34c319970d1 (refs/debug: trace into reflog expiry too,
2021-04-23). This makes a subsequent change smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index 7db4abccc34..449ac3e6cc8 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -364,8 +364,8 @@ struct debug_reflog_expiry_should_prune {
 };
 
 static void debug_reflog_expiry_prepare(const char *refname,
-				    const struct object_id *oid,
-				    void *cb_data)
+					const struct object_id *oid,
+					void *cb_data)
 {
 	struct debug_reflog_expiry_should_prune *prune = cb_data;
 	trace_printf_key(&trace_refs, "reflog_expire_prepare: %s\n", refname);
-- 
2.32.0.873.gb6f2f696497

[PATCH v2 06/11] refs API: pass the "lock OID" to reflog "prepare"

[Ævar Arnfjörð Bjarmason]

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, but
as part of removing the passing of the OID to other functions further
on I'm splitting up these concerns.

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 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 | 2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

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..ec9c70d79cc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3098,7 +3098,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		}
 	}
 
-	(*prepare_fn)(refname, oid, cb.policy_cb);
+	(*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);
 
-- 
2.32.0.873.gb6f2f696497

Re: [PATCH v2 06/11] refs API: pass the "lock OID" to reflog "prepare"

[Jeff King]

On Fri, Jul 16, 2021 at 04:13:02PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 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

This drops "const" from the parameter (also below). I guess this is part
of the preparation for making these functions do the locking. I'll read
on...

-Peff

Re: [PATCH v2 06/11] refs API: pass the "lock OID" to reflog "prepare"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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

It is unclear which assert you refer to, but a call to verify_lock()
near the end of lock_ref_oid_basic() ensures this, I presume?

> exit earlier in this function if we couldn't lock the desired OID, but
> as part of removing the passing of the OID to other functions further
> on I'm splitting up these concerns.
>
> 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 scaffolding
> for that.

OK.  That sounds like a sensible direction to go in.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index af332fa8fe4..ec9c70d79cc 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3098,7 +3098,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  		}
>  	}
>  
> -	(*prepare_fn)(refname, oid, cb.policy_cb);
> +	(*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);

Re: [PATCH v2 06/11] refs API: pass the "lock OID" to reflog "prepare"

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 19 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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
>
> It is unclear which assert you refer to, but a call to verify_lock()
> near the end of lock_ref_oid_basic() ensures this, I presume?

It's something I omitted by mistake, already fixed in the
soon-to-be-sent re-roll, sorry.

[PATCH v2 07/11] refs: make repo_dwim_log() accept a NULL oid

[Ævar Arnfjörð Bjarmason]

Change the repo_dwim_log() function initially added as dwim_log() in
eb3a48221fd (log --reflog: use dwim_log, 2007-02-09) to accept a NULL
oid parameter. The refs_resolve_ref_unsafe() function it invokes
already deals with it, but it didn't.

This allows for a bit more clarity in a reflog-walk.c codepath added
in f2eba66d4d1 (Enable HEAD@{...} and make it independent from the
current branch, 2007-02-03). We'll shortly use this in
builtin/reflog.c as well.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reflog-walk.c | 3 +--
 refs.c        | 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index e9cd3283694..8ac4b284b6b 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -158,10 +158,9 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 		}
 		reflogs = read_complete_reflog(branch);
 		if (!reflogs || reflogs->nr == 0) {
-			struct object_id oid;
 			char *b;
 			int ret = dwim_log(branch, strlen(branch),
-					   &oid, &b);
+					   NULL, &b);
 			if (ret > 1)
 				free(b);
 			else if (ret == 1) {
diff --git a/refs.c b/refs.c
index 8c9490235ea..5e2330bf78e 100644
--- a/refs.c
+++ b/refs.c
@@ -698,7 +698,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 		strbuf_addf(&path, *p, len, str);
 		ref = refs_resolve_ref_unsafe(refs, path.buf,
 					      RESOLVE_REF_READING,
-					      &hash, NULL);
+					      oid ? &hash : NULL, NULL);
 		if (!ref)
 			continue;
 		if (refs_reflog_exists(refs, path.buf))
@@ -710,7 +710,8 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 			continue;
 		if (!logs_found++) {
 			*log = xstrdup(it);
-			oidcpy(oid, &hash);
+			if (oid)
+				oidcpy(oid, &hash);
 		}
 		if (!warn_ambiguous_refs)
 			break;
-- 
2.32.0.873.gb6f2f696497

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

[Ævar Arnfjörð Bjarmason]

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(+)

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;
-- 
2.32.0.873.gb6f2f696497

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

[Jeff King]

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

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

[Junio C Hamano]

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

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

[Jeff King]

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

[PATCH v2 09/11] reflog expire: don't lock reflogs using previously seen OID

[Ævar Arnfjörð Bjarmason]

During reflog expiry, the cmd_reflog_expire() function first iterates
over all reflogs in logs/*, and then one-by-one acquires the lock for
each one and expires it. This behavior has been with us since this
command was implemented in 4264dc15e1 ("git reflog expire",
2006-12-19).

Change this to stop calling lock_ref_oid_basic() with the OID we saw
when we looped over the logs, instead have it pass the OID it managed
to lock.

This mostly mitigates a race condition where e.g. "git gc" will fail
in a concurrently updated repository because the branch moved since
"git reflog expire --all" was started. I.e. with:

    error: cannot lock ref '<refname>': ref '<refname>' is at <OID-A> but expected <OID-B>

This behavior of passing in an "oid" was needed for an edge-case that
I've untangled in this and preceding commits though, namely that we
needed this OID because we'd:

 1. Lookup the reflog name/OID via dwim_log()
 2. With that OID, lock the reflog
 3. Later in builtin/reflog.c we use the we looked as input to
    lookup_commit_reference_gently(), assured that it's equal to the
    OID we got from dwim_log().

What do I mean with "mostly" above? It mostly mitigates it because
we'll still run into cases where the ref is locked and being updated
as we want to expire it, and other git processes wanting to update the
refs will in turn race with us as we expire the reflog.

This remaining race can in turn be mitigated with the
core.filesRefLockTimeout setting, see 4ff0f01cb7 ("refs: retry
acquiring reference locks for 100ms", 2017-08-21). In practice if that
value is high enough we'll probably never have ref updates or reflog
expiry failing, since the clients involved will retry for far longer
than the time any of those operations could take.

See [1] for an initial report of how this impacted "git gc" and a
large discussion about this change in early 2019. In particular patch
looked good to Michael Haggerty his[2], but that seems to not have
made it to the ML archive, its content is quoted in full in my [3].

I'm leaving behind now-unused code the refs API etc. that takes the
now-NULL "oid" argument, and other code that can be simplified now
that we never have on OID in that context, that'll be cleaned up in
subsequent commits, but for now let's narrowly focus on fixing the
"git gc" issue

Unfortunately this sort of probabilistic contention is hard to turn
into a test. I've tested this by running the following three subshells
in concurrent terminals:

    (
        rm -rf /tmp/git &&
        git init /tmp/git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >/tmp/git/out &&
            git -C /tmp/git add out &&
            git -C /tmp/git commit -m"out"
        done
    )

    (
	rm -rf /tmp/git-clone &&
        git clone file:///tmp/git /tmp/git-clone &&
        while git -C /tmp/git-clone pull
        do
            date
        done
    )

    (
        while git -C /tmp/git-clone reflog expire --all
        do
            date
        done
    )

Before this change the "reflog expire" would fail really quickly with
a "but expected" error.

After this change both the "pull" and "reflog expire" will run for a
while, but eventually fail because I get unlucky with
core.filesRefLockTimeout (the "reflog expire" is in a really tight
loop). As noted above that can in turn be mitigated with higher values
of core.filesRefLockTimeout than the 100ms default.

As noted in the commentary added in the preceding commit there's also
the case of branches being racily deleted, that can be tested by
adding this to the above:

    (
        while git -C /tmp/git-clone branch topic master &&
	      git -C /tmp/git-clone branch -D topic
        do
            date
        done
    )

With core.filesRefLockTimeout set to 10 seconds (it can probably be a
lot lower) I managed to run all four of these concurrently for about
an hour, and accumulated ~125k commits, auto-gc's and all, and didn't
have a single failure.

1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/
2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu
3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c     | 13 ++++++-------
 refs/files-backend.c |  2 +-
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 9f9e6bceb03..4506852c471 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -629,8 +629,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
+
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, &e->oid, flags,
+			status |= reflog_expire(e->reflog, NULL, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
@@ -642,13 +643,12 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (; i < argc; i++) {
 		char *ref;
-		struct object_id oid;
-		if (!dwim_log(argv[i], strlen(argv[i]), &oid, &ref)) {
+		if (!dwim_log(argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
-		status |= reflog_expire(ref, &oid, flags,
+		status |= reflog_expire(ref, NULL, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
@@ -700,7 +700,6 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 	for ( ; i < argc; i++) {
 		const char *spec = strstr(argv[i], "@{");
-		struct object_id oid;
 		char *ep, *ref;
 		int recno;
 
@@ -709,7 +708,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!dwim_log(argv[i], spec - argv[i], &oid, &ref)) {
+		if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) {
 			status |= error(_("no reflog for '%s'"), argv[i]);
 			continue;
 		}
@@ -724,7 +723,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.cmd.expire_total = 0;
 		}
 
-		status |= reflog_expire(ref, &oid, flags,
+		status |= reflog_expire(ref, NULL, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f73637fa087..cb8c64cffb5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3060,7 +3060,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, oid,
+	lock = lock_ref_oid_basic(refs, refname, NULL,
 				  REF_NO_DEREF,
 				  &type, &err);
 	if (!lock) {
-- 
2.32.0.873.gb6f2f696497

Re: [PATCH v2 09/11] reflog expire: don't lock reflogs using previously seen OID

[Jeff King]

On Fri, Jul 16, 2021 at 04:13:05PM +0200, Ævar Arnfjörð Bjarmason wrote:

> During reflog expiry, the cmd_reflog_expire() function first iterates
> over all reflogs in logs/*, and then one-by-one acquires the lock for
> each one and expires it. This behavior has been with us since this
> command was implemented in 4264dc15e1 ("git reflog expire",
> 2006-12-19).
> 
> Change this to stop calling lock_ref_oid_basic() with the OID we saw
> when we looped over the logs, instead have it pass the OID it managed
> to lock.
> 
> This mostly mitigates a race condition where e.g. "git gc" will fail
> in a concurrently updated repository because the branch moved since
> "git reflog expire --all" was started. I.e. with:
> 
>     error: cannot lock ref '<refname>': ref '<refname>' is at <OID-A> but expected <OID-B>
> 
> This behavior of passing in an "oid" was needed for an edge-case that
> I've untangled in this and preceding commits though, namely that we
> needed this OID because we'd:
> 
>  1. Lookup the reflog name/OID via dwim_log()
>  2. With that OID, lock the reflog
>  3. Later in builtin/reflog.c we use the we looked as input to
>     lookup_commit_reference_gently(), assured that it's equal to the
>     OID we got from dwim_log().

s/we use the we/we use the oid we/ in point 3, I think?

This explains the race and why it mitigates it, which is good. Are we
sure that there is no value in matching the oid we found during the log
lookup with the current value of the ref? I.e., why is this safe to do?

I suspect it is OK. The only problem would be if the reflog.c is somehow
relying on the ref's value not having changed from the earlier call
(e.g., if it did some reachability computation based on that value).

But I don't think so. Between the dwim_log() call and the
reflog_expire() call in reflog_expire(), we do almost nothing. In
reflog_delete(), between the two we do count up the entries to find a
record number. We do use that in deciding whether to expire an entry,
but:

  - I believe it is counting up from the beginning of time, so if
    somebody appends a new entry in the meantime, we are OK.

  - Further updates are not strictly prevented by comparing the oids
    anyway (we could generate new reflog entries without changing the
    ref; either a noop, or a switch-and-revert).

I do suspect there's a latent bug there if somebody else tries to expire
the reflogs (because now they're deleting old entries which are part of
our "recno"). But it is neither helped nor hindered by your change here,
so let's ignore it for now.

> [...]

The patch itself looks correct to me.

I think it would be good to have a brief "why is this safe" argument in
the commit message along the lines of what I wrote above.

-Peff

Re: [PATCH v2 09/11] reflog expire: don't lock reflogs using previously seen OID

[Han-Wen Nienhuys]

On Fri, Jul 16, 2021 at 4:13 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> -                       status |= reflog_expire(e->reflog, &e->oid, flags,
> +                       status |= reflog_expire(e->reflog, NULL, flags,
>                                                 reflog_expiry_prepare,

this causes reflog_expiry_prepare() to be called with a NULL oid. I'm
seeing a crash in do_lookup_replace_object() because of this in
t0031-reftable.sh.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH v2 09/11] reflog expire: don't lock reflogs using previously seen OID

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> On Fri, Jul 16, 2021 at 4:13 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> -                       status |= reflog_expire(e->reflog, &e->oid, flags,
>> +                       status |= reflog_expire(e->reflog, NULL, flags,
>>                                                 reflog_expiry_prepare,
>
> this causes reflog_expiry_prepare() to be called with a NULL oid. I'm
> seeing a crash in do_lookup_replace_object() because of this in
> t0031-reftable.sh.

Yeah, given that reflog_expire() is documented to take "oid is the
old value of the reference", the change looks bogus to me too.

Ævar, what is going on here?

Thanks.

Re: [PATCH v2 09/11] reflog expire: don't lock reflogs using previously seen OID

[Carlo Marcelo Arenas Belón]

On Wed, Aug 18, 2021 at 02:05:12PM -0700, Junio C Hamano wrote:
> Han-Wen Nienhuys <hanwen@google.com> writes:
> 
> > On Fri, Jul 16, 2021 at 4:13 PM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >> -                       status |= reflog_expire(e->reflog, &e->oid, flags,
> >> +                       status |= reflog_expire(e->reflog, NULL, flags,
> >>                                                 reflog_expiry_prepare,
> >
> > this causes reflog_expiry_prepare() to be called with a NULL oid. I'm
> > seeing a crash in do_lookup_replace_object() because of this in
> > t0031-reftable.sh.
> 
> Yeah, given that reflog_expire() is documented to take "oid is the
> old value of the reference", the change looks bogus to me too.
> 
> Ævar, what is going on here?

FWIW the crude revert of this commit (cut below for easy access)  does
almost (except for the pedantic fixes[1] that are expected with the next
fsmonitor rollup) allow a CI run[2] for "seen" to go fully to green.

Carlo

[1] https://lore.kernel.org/git/20210809063004.73736-1-carenas@gmail.com/
[2] https://github.com/carenas/git/runs/3370161764

--- >8 ---
Subject: [PATCH] Revert "reflog expire: don't lock reflogs using previously
 seen OID"

This reverts commit 8bb2a971949c50787809f14ccf1d2a5d5324f4e4.
---
 builtin/reflog.c     | 13 +++++++------
 refs.h               |  2 +-
 refs/files-backend.c |  5 +----
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 61795f22d5..09541d1c80 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -629,9 +629,8 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
-
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, NULL, flags,
+			status |= reflog_expire(e->reflog, &e->oid, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
@@ -643,12 +642,13 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (; i < argc; i++) {
 		char *ref;
-		if (!dwim_log(argv[i], strlen(argv[i]), NULL, &ref)) {
+		struct object_id oid;
+		if (!dwim_log(argv[i], strlen(argv[i]), &oid, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
-		status |= reflog_expire(ref, NULL, flags,
+		status |= reflog_expire(ref, &oid, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
@@ -700,6 +700,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 	for ( ; i < argc; i++) {
 		const char *spec = strstr(argv[i], "@{");
+		struct object_id oid;
 		char *ep, *ref;
 		int recno;
 
@@ -708,7 +709,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) {
+		if (!dwim_log(argv[i], spec - argv[i], &oid, &ref)) {
 			status |= error(_("no reflog for '%s'"), argv[i]);
 			continue;
 		}
@@ -723,7 +724,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.cmd.expire_total = 0;
 		}
 
-		status |= reflog_expire(ref, NULL, flags,
+		status |= reflog_expire(ref, &oid, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
diff --git a/refs.h b/refs.h
index 3e4ee01f8f..38773f3229 100644
--- a/refs.h
+++ b/refs.h
@@ -810,7 +810,7 @@ enum expire_reflog_flags {
  * expiration policy that is desired.
  *
  * reflog_expiry_prepare_fn -- Called once after the reference is
- *     locked. Called with the OID of the locked reference.
+ *     locked.
  *
  * reflog_expiry_should_prune_fn -- Called once for each entry in the
  *     existing reflog. It should return true iff that entry should be
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f546cc3cc3..e72cb0e43f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3078,7 +3078,7 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 }
 
 static int files_reflog_expire(struct ref_store *ref_store,
-			       const char *refname, const struct object_id *unused_oid,
+			       const char *refname, const struct object_id *oid,
 			       unsigned int flags,
 			       reflog_expiry_prepare_fn prepare_fn,
 			       reflog_expiry_should_prune_fn should_prune_fn,
@@ -3095,7 +3095,6 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	int status = 0;
 	int type;
 	struct strbuf err = STRBUF_INIT;
-	const struct object_id *oid;
 
 	memset(&cb, 0, sizeof(cb));
 	cb.flags = flags;
@@ -3113,7 +3112,6 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		strbuf_release(&err);
 		return -1;
 	}
-	oid = &lock->old_oid;
 
 	/*
 	 * When refs are deleted, their reflog is deleted before the
@@ -3157,7 +3155,6 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		}
 	}
 
-	assert(!unused_oid);
 	(*prepare_fn)(refname, oid, cb.policy_cb);
 	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
 	(*cleanup_fn)(cb.policy_cb);
-- 
2.33.0.476.gf000ecbed9

Re: [PATCH v2 09/11] reflog expire: don't lock reflogs using previously seen OID

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> FWIW the crude revert of this commit (cut below for easy access)  does
> almost (except for the pedantic fixes[1] that are expected with the next
> fsmonitor rollup) allow a CI run[2] for "seen" to go fully to green.

Thanks.

I've decided to kick these topics out of 'next' and move them nearer
to the tip of 'seen':

    ab/refs-files-cleanup
    hn/refs-errno-cleanup
    jk/refs-files-cleanup-cleanup

Most importantly they are now queued after the hn/reftable topic, so
that it get its turn for more exposure and priority.  Of course, we
still have the same CI breakage at the tip of 'seen'.

The jk/refs-files-cleanup-cleanup is to clean up leftover cruft in
the ab/refs-files-cleanup topic, so when ab/refs-files-cleanup is
updated, hopefully its contents can be squashed into its commit(s).
hn/refs-errno-cleanup builds on top of it and hopefully rebasing
the whole thing on top of hn/reftable after it becomes stable would
get rid of the CI breakage.

[PATCH v2 10/11] refs/files: remove unused "oid" in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

In the preceding commit the last caller that passed a non-NULL OID was
changed to pass NULL to lock_ref_oid_basic(). As noted in preceding
commits use of this API has been going away (we should use ref
transactions, or lock_raw_rew()), so we're unlikely to gain new
callers that want to pass the "oid".

So let's remove it, doing so means we can remove the "mustexist"
condition, and therefore anything except the "flags =
RESOLVE_REF_NO_RECURSE" case. Furthermore, since the verify_lock()
function we called did most of its work when the "oid" was passed (as
"old_oid") we can inline the trivial part of it that remains in what's
now its only caller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 72 ++++++++------------------------------------
 1 file changed, 12 insertions(+), 60 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index cb8c64cffb5..3e7598cc86c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -852,42 +852,6 @@ static struct ref_iterator *files_ref_iterator_begin(
 	return ref_iterator;
 }
 
-/*
- * Verify that the reference locked by lock has the value old_oid
- * (unless it is NULL).  Fail if the reference doesn't exist and
- * mustexist is set. Return 0 on success. On error, write an error
- * message to err, set errno, and return a negative value.
- */
-static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock,
-		       const struct object_id *old_oid, int mustexist,
-		       struct strbuf *err)
-{
-	assert(err);
-
-	if (refs_read_ref_full(ref_store, lock->ref_name,
-			       mustexist ? RESOLVE_REF_READING : 0,
-			       &lock->old_oid, NULL)) {
-		if (old_oid) {
-			int save_errno = errno;
-			strbuf_addf(err, "can't verify ref '%s'", lock->ref_name);
-			errno = save_errno;
-			return -1;
-		} else {
-			oidclr(&lock->old_oid);
-			return 0;
-		}
-	}
-	if (old_oid && !oideq(&lock->old_oid, old_oid)) {
-		strbuf_addf(err, "ref '%s' is at %s but expected %s",
-			    lock->ref_name,
-			    oid_to_hex(&lock->old_oid),
-			    oid_to_hex(old_oid));
-		errno = EBUSY;
-		return -1;
-	}
-	return 0;
-}
-
 static int remove_empty_directories(struct strbuf *path)
 {
 	/*
@@ -913,15 +877,12 @@ static int create_reflock(const char *path, void *cb)
  */
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
-					   const struct object_id *old_oid,
 					   unsigned int flags, int *type,
 					   struct strbuf *err)
 {
 	struct strbuf ref_file = STRBUF_INIT;
 	struct ref_lock *lock;
 	int last_errno = 0;
-	int mustexist = (old_oid && !is_null_oid(old_oid));
-	int resolve_flags = RESOLVE_REF_NO_RECURSE;
 	int resolved;
 
 	files_assert_main_repository(refs, "lock_ref_oid_basic");
@@ -929,12 +890,9 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 
 	CALLOC_ARRAY(lock, 1);
 
-	if (mustexist)
-		resolve_flags |= RESOLVE_REF_READING;
-
 	files_ref_path(refs, &ref_file, refname);
-	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 && errno == EISDIR) {
 		/*
@@ -952,8 +910,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					    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) {
@@ -988,10 +946,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) {
-		last_errno = errno;
-		goto error_return;
-	}
+	if (refs_read_ref_full(&refs->base, lock->ref_name,
+			       0,
+			       &lock->old_oid, NULL))
+		oidclr(&lock->old_oid);
 	goto out;
 
  error_return:
@@ -1410,8 +1368,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
-	lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
-				  &err);
+	lock = lock_ref_oid_basic(refs, newrefname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1433,8 +1390,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	goto out;
 
  rollback:
-	lock = lock_ref_oid_basic(refs, oldrefname, NULL,
-				  REF_NO_DEREF, NULL, &err);
+	lock = lock_ref_oid_basic(refs, oldrefname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
@@ -1841,9 +1797,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	struct ref_lock *lock;
 	int ret;
 
-	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  REF_NO_DEREF, NULL,
-				  &err);
+	lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -3060,9 +3014,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  REF_NO_DEREF,
-				  &type, &err);
+	lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
-- 
2.32.0.873.gb6f2f696497

Re: [PATCH v2 10/11] refs/files: remove unused "oid" in lock_ref_oid_basic()

[Jeff King]

On Fri, Jul 16, 2021 at 04:13:06PM +0200, Ævar Arnfjörð Bjarmason wrote:

> In the preceding commit the last caller that passed a non-NULL OID was
> changed to pass NULL to lock_ref_oid_basic(). As noted in preceding
> commits use of this API has been going away (we should use ref
> transactions, or lock_raw_rew()), so we're unlikely to gain new
> callers that want to pass the "oid".

s/rew/ref/

> So let's remove it, doing so means we can remove the "mustexist"
> condition, and therefore anything except the "flags =
> RESOLVE_REF_NO_RECURSE" case. Furthermore, since the verify_lock()
> function we called did most of its work when the "oid" was passed (as
> "old_oid") we can inline the trivial part of it that remains in what's
> now its only caller.
> [...]
>  1 file changed, 12 insertions(+), 60 deletions(-)

Sounds good, and the diffstat is very pleasing. :)

The patch itself looks correct to me.

-Peff

[PATCH v2 11/11] refs/files: remove unused "errno == EISDIR" code

[Ævar Arnfjörð Bjarmason]

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
in bc7127ef0f (ref locking: allow 'foo' when 'foo/bar' used to exist
but not anymore., 2006-09-30) seems like it should be dealing with.

Except it doesn't, and we never take this branch. The reason is that
when bc7127ef0f was written this looked like:

	ref = resolve_ref([...]);
	if (!ref && errno == EISDIR) {
	[...]

And in resolve_ref() we had this code:

	fd = open(path, O_RDONLY);
	if (fd < 0)
		return NULL;

I.e. we would attempt to read "foo" with open(), which would fail with
EISDIR and we'd return NULL. We'd then take this branch, call
remove_empty_directories() and continue.

Since a1c1d8170d (refs_resolve_ref_unsafe: handle d/f conflicts for
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()

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);
	if (fd < 0)
		/* get errno == EISDIR */
	/* later, in refs_resolve_ref_unsafe() */
	if ([...] && errno != EISDIR)
		return NULL;
	[...]
	/* returns the refs/heads/foo to the caller, even though it's a directory */
	return refname;

I.e. even though we got an "errno == EISDIR" we won't take this
branch, since in cases of EISDIR "resolved" is always
non-NULL. I.e. we pretend at this point as though everything's OK and
there is no "foo" directory.

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, 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 | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3e7598cc86c..bd99bc570de 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -883,7 +883,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	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);
@@ -891,30 +890,9 @@ 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
-		 * have foo/bar which now does not exist;
-		 * it is normal for the empty directory 'foo'
-		 * to remain.
-		 */
-		if (remove_empty_directories(&ref_file)) {
-			last_errno = errno;
-			if (!refs_verify_refname_available(
-					    &refs->base,
-					    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_REF_NO_RECURSE,
-						     &lock->old_oid, type);
-	}
-	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,
-- 
2.32.0.873.gb6f2f696497

Re: [PATCH v2 11/11] refs/files: remove unused "errno == EISDIR" code

[Jeff King]

On Fri, Jul 16, 2021 at 04:13:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 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:
> [...]

Right, thanks for digging into this situation in the commit message.

> @@ -891,30 +890,9 @@ 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
> -		 * have foo/bar which now does not exist;
> -		 * it is normal for the empty directory 'foo'
> -		 * to remain.
> -		 */
> -		if (remove_empty_directories(&ref_file)) {
> -			last_errno = errno;
> -			if (!refs_verify_refname_available(
> -					    &refs->base,
> -					    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_REF_NO_RECURSE,
> -						     &lock->old_oid, type);
> -	}
> -	if (!resolved) {
> +	if (!refs_resolve_ref_unsafe(&refs->base, refname,
> +				     RESOLVE_REF_NO_RECURSE,
> +				     &lock->old_oid, type)) {

I agree that this is all trivially dead code now, and can be removed.

>  		last_errno = errno;
>  		if (last_errno != ENOTDIR ||
>  		    !refs_verify_refname_available(&refs->base, refname,

Not necessary, but I think "last_errno != ENOTDIR" will always be true
now, too. We treat it the same as EISDIR in refs_resolve_ref_unsafe().
It's not that big a cleanup, but it could easily go on top.

-Peff

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

[Jeff King]

On Fri, Jul 16, 2021 at 04:12:56PM +0200, Ævar Arnfjörð Bjarmason wrote:

>  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(-)

The diffstat certainly looks nice. :)

I left a few comments on specific patches, mostly just little things.
The ones I didn't comment on looked good to me. Overall, it seems like a
very nice cleanup. Thanks for putting it together.

-Peff

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

[Ævar Arnfjörð Bjarmason]

A re-roll of
https://lore.kernel.org/git/cover-00.11-00000000000-20210716T140631Z-avarab@gmail.com/;
hopefully addressing all the comments by Peff & Junio, thanks
both!

Changes:

 * Lots of commit message/comment changes, see range-diff

 * I referred to an assert() I forgot to add, the series now juggles
   an assert() of thei oid back and forth, for clarity of what we're
   actually doing.

   As noted in v2 I've got a series prepped for this one to finally
   clean up those loose ends/dead code, but let's focus on the
   behavior changes for now.

 * There's now a 12th patch to remove the "ENOTDIR" case Peff noted,

Ævar Arnfjörð Bjarmason (12):
  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
  refs/files: remove unused "errno != ENOTDIR" condition

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

Range-diff against v2:
 1:  30bd7679a5c =  1:  737d2d8c3d1 refs/packet: add missing BUG() invocations to reflog callbacks
 2:  033c0cec33d !  2:  dfb9e34076e refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
    @@ Metadata
      ## Commit message ##
         refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
     
    -    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.
    +    The lock_ref_oid_basic() function has gradually been by most callers
    +    no longer performing a low-level "acquire lock, update and release",
    +    and instead using the ref transaction API. So there are only 4
    +    remaining callers of lock_ref_oid_basic().
    +
    +    None of those callers pass REF_DELETING anymore, the last caller went
    +    away in 92b1551b1d (refs: resolve symbolic refs first,
    +    2016-04-25).
    +
    +    Before that we'd refactored and moved this code in:
    +
    +     - 8df4e511387 (struct ref_update: move "have_old" into "flags",
    +       2015-02-17)
    +
    +     - 7bd9bcf372d (refs: split filesystem-based refs code into a new
    +       file, 2015-11-09)
    +
    +     - 165056b2fc (lock_ref_for_update(): new function, 2016-04-24)
    +
    +    We then finally stopped using it in 92b1551b1d (noted above). So let's
    +    remove the handling of this parameter.
    +
    +    By itself this change doesn't benefit us much, but it's the start of
    +    even more removal of unused code in and around this function in
    +    subsequent commits.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
 3:  94ffcd8cfda =  3:  0f2262fec69 refs/files: remove unused "extras/skip" in lock_ref_oid_basic()
 4:  61f9e0fc864 =  4:  7fb7ff97491 refs/files: remove unused "skip" in lock_raw_ref() too
 5:  95101c322b7 =  5:  4e526c34aaa refs/debug: re-indent argument list for "prepare"
 6:  e93465f4137 !  6:  295594fe8ae refs API: pass the "lock OID" to reflog "prepare"
    @@ Commit message
         refs API: pass the "lock OID" to reflog "prepare"
     
         Don't pass the object ID we pass into reflog_expire() back to the
    -    caller, but rather our locked OID.
    +    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.
     
    -    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, but
    -    as part of removing the passing of the OID to other functions further
    -    on I'm splitting up these concerns.
    +    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 scaffolding
    -    for that.
    +    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>
     
    @@ refs/files-backend.c: 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);
 7:  0fff2d32cfc =  7:  e45ec439db0 refs: make repo_dwim_log() accept a NULL oid
 8:  1e25b7c59c5 !  8:  8ae8e5ac029 refs/files: add a comment about refs_reflog_exists() call
    @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
      	}
     +
     +	/*
    -+	 * 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
    ++	 * 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 race happens we've got nothing more to do, we were asked
    -+	 * to delete the reflog, and it's not there anymore. Great!
    ++	 * If a race happens and the reflog doesn't exist after we've
    ++	 * acquired the lock that's OK. We've got nothing more to do;
    ++	 * We were asked to delete the reflog, but someone else
    ++	 * deleted it! The caller doesn't care that we deleted it,
    ++	 * just that it is deleted. So we can return successfully.
     +	 */
      	if (!refs_reflog_exists(ref_store, refname)) {
      		unlock_ref(lock);
 9:  60d6cf342fc !  9:  1050743e27c reflog expire: don't lock reflogs using previously seen OID
    @@ Commit message
     
          1. Lookup the reflog name/OID via dwim_log()
          2. With that OID, lock the reflog
    -     3. Later in builtin/reflog.c we use the we looked as input to
    +     3. Later in builtin/reflog.c we use the OID we looked as input to
             lookup_commit_reference_gently(), assured that it's equal to the
             OID we got from dwim_log().
     
    -    What do I mean with "mostly" above? It mostly mitigates it because
    -    we'll still run into cases where the ref is locked and being updated
    -    as we want to expire it, and other git processes wanting to update the
    -    refs will in turn race with us as we expire the reflog.
    +    We can be sure that this change is safe to make because between
    +    dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other
    +    logic relevant to the OID or expiry run in the cmd_reflog_expire()
    +    caller.
     
    -    This remaining race can in turn be mitigated with the
    +    We can thus treat that code as a black box, before and after this
    +    change it would get an OID that's been locked, the only difference is
    +    that now we mostly won't be failing to get the lock due to the TOCTOU
    +    race[0]. That failure was purely an implementation detail in how the
    +    "current OID" was looked up, it was divorced from the locking
    +    mechanism.
    +
    +    What do we mean with "mostly"? It mostly mitigates it because we'll
    +    still run into cases where the ref is locked and being updated as we
    +    want to expire it, and other git processes wanting to update the refs
    +    will in turn race with us as we expire the reflog.
    +
    +    That remaining race can in turn be mitigated with the
         core.filesRefLockTimeout setting, see 4ff0f01cb7 ("refs: retry
         acquiring reference locks for 100ms", 2017-08-21). In practice if that
         value is high enough we'll probably never have ref updates or reflog
    @@ Commit message
     
         See [1] for an initial report of how this impacted "git gc" and a
         large discussion about this change in early 2019. In particular patch
    -    looked good to Michael Haggerty his[2], but that seems to not have
    -    made it to the ML archive, its content is quoted in full in my [3].
    +    looked good to Michael Haggerty, see his[2]. That message seems to not
    +    have made it to the ML archive, its content is quoted in full in my
    +    [3].
     
         I'm leaving behind now-unused code the refs API etc. that takes the
         now-NULL "oid" argument, and other code that can be simplified now
         that we never have on OID in that context, that'll be cleaned up in
         subsequent commits, but for now let's narrowly focus on fixing the
    -    "git gc" issue
    +    "git gc" issue. As the modified assert() shows we always pass a NULL
    +    oid to reflog_expire() now.
     
         Unfortunately this sort of probabilistic contention is hard to turn
         into a test. I've tested this by running the following three subshells
    @@ Commit message
             )
     
         Before this change the "reflog expire" would fail really quickly with
    -    a "but expected" error.
    +    the "but expected" error noted above.
     
         After this change both the "pull" and "reflog expire" will run for a
         while, but eventually fail because I get unlucky with
    @@ Commit message
         With core.filesRefLockTimeout set to 10 seconds (it can probably be a
         lot lower) I managed to run all four of these concurrently for about
         an hour, and accumulated ~125k commits, auto-gc's and all, and didn't
    -    have a single failure.
    +    have a single failure. The loops visibly stall while waiting for the
    +    lock, but that's expected and desired behavior.
     
    +    0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use
         1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/
         2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu
         3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/
    @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
      				  REF_NO_DEREF,
      				  &type, &err);
      	if (!lock) {
    +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
    + 		}
    + 	}
    + 
    +-	assert(oideq(&lock->old_oid, oid));
    ++	assert(!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);
10:  09dd8836437 ! 10:  753c20f89bf refs/files: remove unused "oid" in lock_ref_oid_basic()
    @@ Commit message
         In the preceding commit the last caller that passed a non-NULL OID was
         changed to pass NULL to lock_ref_oid_basic(). As noted in preceding
         commits use of this API has been going away (we should use ref
    -    transactions, or lock_raw_rew()), so we're unlikely to gain new
    +    transactions, or lock_raw_ref()), so we're unlikely to gain new
         callers that want to pass the "oid".
     
         So let's remove it, doing so means we can remove the "mustexist"
         condition, and therefore anything except the "flags =
    -    RESOLVE_REF_NO_RECURSE" case. Furthermore, since the verify_lock()
    -    function we called did most of its work when the "oid" was passed (as
    -    "old_oid") we can inline the trivial part of it that remains in what's
    -    now its only caller.
    +    RESOLVE_REF_NO_RECURSE" case.
    +
    +    Furthermore, since the verify_lock() function we called did most of
    +    its work when the "oid" was passed (as "old_oid") we can inline the
    +    trivial part of it that remains in its only remaining caller. Without
    +    a NULL "oid" passed it was equivalent to calling refs_read_ref_full()
    +    followed by oidclr().
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
11:  96c3e5e9f79 = 11:  8a71bbef972 refs/files: remove unused "errno == EISDIR" code
 -:  ----------- > 12:  452253d597d refs/files: remove unused "errno != ENOTDIR" condition
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 01/12] refs/packet: add missing BUG() invocations to reflog callbacks

[Ævar Arnfjörð Bjarmason]

In e0cc8ac8202 (packed_ref_store: make class into a subclass of
`ref_store`, 2017-06-23) a die() was added to packed_create_reflog(),
but not to any of the other reflog callbacks, let's do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/packed-backend.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f8aa97d7998..24a360b719f 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1600,6 +1600,7 @@ static int packed_for_each_reflog_ent(struct ref_store *ref_store,
 				      const char *refname,
 				      each_reflog_ent_fn fn, void *cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1608,12 +1609,14 @@ static int packed_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 					      each_reflog_ent_fn fn,
 					      void *cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
 static int packed_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1627,6 +1630,7 @@ static int packed_create_reflog(struct ref_store *ref_store,
 static int packed_delete_reflog(struct ref_store *ref_store,
 			       const char *refname)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1638,6 +1642,7 @@ static int packed_reflog_expire(struct ref_store *ref_store,
 				reflog_expiry_cleanup_fn cleanup_fn,
 				void *policy_cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 02/12] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

The lock_ref_oid_basic() function has gradually been by most callers
no longer performing a low-level "acquire lock, update and release",
and instead using the ref transaction API. So there are only 4
remaining callers of lock_ref_oid_basic().

None of those callers pass REF_DELETING anymore, the last caller went
away in 92b1551b1d (refs: resolve symbolic refs first,
2016-04-25).

Before that we'd refactored and moved this code in:

 - 8df4e511387 (struct ref_update: move "have_old" into "flags",
   2015-02-17)

 - 7bd9bcf372d (refs: split filesystem-based refs code into a new
   file, 2015-11-09)

 - 165056b2fc (lock_ref_for_update(): new function, 2016-04-24)

We then finally stopped using it in 92b1551b1d (noted above). So let's
remove the handling of this parameter.

By itself this change doesn't benefit us much, but it's the start of
even more removal of unused code in and around this function in
subsequent commits.

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,
-- 
2.32.0.874.ge7a9d58bfcf

Re: [PATCH v3 02/12] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The lock_ref_oid_basic() function has gradually been by most callers
> no longer performing a low-level "acquire lock, update and release",

Something is missing after "has gradually been", isn't it?
"replaced", perhaps?

[PATCH v3 03/12] refs/files: remove unused "extras/skip" in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

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 non-NULL "extras" and "skip" parameters,
the last such caller went away in 92b1551b1d4 (refs: resolve symbolic
refs first, 2016-04-25), so let's remove the parameters.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 326f0224218..a59823d667e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -915,8 +915,6 @@ static int create_reflock(const char *path, void *cb)
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
 					   const struct object_id *old_oid,
-					   const struct string_list *extras,
-					   const struct string_list *skip,
 					   unsigned int flags, int *type,
 					   struct strbuf *err)
 {
@@ -950,7 +948,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 			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;
@@ -963,7 +961,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
 		    !refs_verify_refname_available(&refs->base, refname,
-						   extras, skip, err))
+						   NULL, NULL, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
 				    refname, strerror(last_errno));
 
@@ -978,7 +976,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	 */
 	if (is_null_oid(&lock->old_oid) &&
 	    refs_verify_refname_available(refs->packed_ref_store, refname,
-					  extras, skip, err)) {
+					  NULL, NULL, err)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -1413,8 +1411,8 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
-	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL,
-				  REF_NO_DEREF, NULL, &err);
+	lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
+				  &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1436,7 +1434,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	goto out;
 
  rollback:
-	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL,
+	lock = lock_ref_oid_basic(refs, oldrefname, NULL,
 				  REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
@@ -1845,7 +1843,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	int ret;
 
 	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  NULL, NULL, REF_NO_DEREF, NULL,
+				  REF_NO_DEREF, NULL,
 				  &err);
 	if (!lock) {
 		error("%s", err.buf);
@@ -3064,7 +3062,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference if --updateref was specified:
 	 */
 	lock = lock_ref_oid_basic(refs, refname, oid,
-				  NULL, NULL, REF_NO_DEREF,
+				  REF_NO_DEREF,
 				  &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 04/12] refs/files: remove unused "skip" in lock_raw_ref() too

[Ævar Arnfjörð Bjarmason]

Remove the unused "skip" parameter to lock_raw_ref(), it was never
used. We do use it when passing "skip" to the
refs_rename_ref_available() function in files_copy_or_rename_ref(),
but not here.

This is part of a larger series that modifies lock_ref_oid_basic()
extensively, there will be no more modifications of this function in
this series, but since the preceding commit removed this unused
parameter from lock_ref_oid_basic(), let's do it here too for
consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a59823d667e..af332fa8fe4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -531,7 +531,6 @@ static void unlock_ref(struct ref_lock *lock)
 static int lock_raw_ref(struct files_ref_store *refs,
 			const char *refname, int mustexist,
 			const struct string_list *extras,
-			const struct string_list *skip,
 			struct ref_lock **lock_p,
 			struct strbuf *referent,
 			unsigned int *type,
@@ -568,7 +567,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		 * reason to expect this error to be transitory.
 		 */
 		if (refs_verify_refname_available(&refs->base, refname,
-						  extras, skip, err)) {
+						  extras, NULL, err)) {
 			if (mustexist) {
 				/*
 				 * To the user the relevant error is
@@ -673,7 +672,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 							  REMOVE_DIR_EMPTY_ONLY)) {
 				if (refs_verify_refname_available(
 						    &refs->base, refname,
-						    extras, skip, err)) {
+						    extras, NULL, err)) {
 					/*
 					 * The error message set by
 					 * verify_refname_available() is OK.
@@ -710,7 +709,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		 */
 		if (refs_verify_refname_available(
 				    refs->packed_ref_store, refname,
-				    extras, skip, err))
+				    extras, NULL, err))
 			goto error_return;
 	}
 
@@ -2412,7 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	}
 
 	ret = lock_raw_ref(refs, update->refname, mustexist,
-			   affected_refnames, NULL,
+			   affected_refnames,
 			   &lock, &referent,
 			   &update->type, err);
 	if (ret) {
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 05/12] refs/debug: re-indent argument list for "prepare"

[Ævar Arnfjörð Bjarmason]

Re-indent this argument list that's been mis-indented since it was
added in 34c319970d1 (refs/debug: trace into reflog expiry too,
2021-04-23). This makes a subsequent change smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index 7db4abccc34..449ac3e6cc8 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -364,8 +364,8 @@ struct debug_reflog_expiry_should_prune {
 };
 
 static void debug_reflog_expiry_prepare(const char *refname,
-				    const struct object_id *oid,
-				    void *cb_data)
+					const struct object_id *oid,
+					void *cb_data)
 {
 	struct debug_reflog_expiry_should_prune *prune = cb_data;
 	trace_printf_key(&trace_refs, "reflog_expire_prepare: %s\n", refname);
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 06/12] refs API: pass the "lock OID" to reflog "prepare"

[Ævar Arnfjörð Bjarmason]

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(-)

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);
 
-- 
2.32.0.874.ge7a9d58bfcf

Re: [PATCH v3 06/12] refs API: pass the "lock OID" to reflog "prepare"

[Junio C Hamano]

Æ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.

Re: [PATCH v3 06/12] refs API: pass the "lock OID" to reflog "prepare"

[Junio C Hamano]

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.

Re: [PATCH v3 06/12] refs API: pass the "lock OID" to reflog "prepare"

[Ævar Arnfjörð Bjarmason]

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?

Re: [PATCH v3 06/12] refs API: pass the "lock OID" to reflog "prepare"

[Junio C Hamano]

Æ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.

Re: [PATCH v3 06/12] refs API: pass the "lock OID" to reflog "prepare"

[Ævar Arnfjörð Bjarmason]

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?

Re: [PATCH v3 06/12] refs API: pass the "lock OID" to reflog "prepare"

[Junio C Hamano]

Æ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.

[PATCH v3 07/12] refs: make repo_dwim_log() accept a NULL oid

[Ævar Arnfjörð Bjarmason]

Change the repo_dwim_log() function initially added as dwim_log() in
eb3a48221fd (log --reflog: use dwim_log, 2007-02-09) to accept a NULL
oid parameter. The refs_resolve_ref_unsafe() function it invokes
already deals with it, but it didn't.

This allows for a bit more clarity in a reflog-walk.c codepath added
in f2eba66d4d1 (Enable HEAD@{...} and make it independent from the
current branch, 2007-02-03). We'll shortly use this in
builtin/reflog.c as well.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reflog-walk.c | 3 +--
 refs.c        | 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index e9cd3283694..8ac4b284b6b 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -158,10 +158,9 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 		}
 		reflogs = read_complete_reflog(branch);
 		if (!reflogs || reflogs->nr == 0) {
-			struct object_id oid;
 			char *b;
 			int ret = dwim_log(branch, strlen(branch),
-					   &oid, &b);
+					   NULL, &b);
 			if (ret > 1)
 				free(b);
 			else if (ret == 1) {
diff --git a/refs.c b/refs.c
index 8b9f7c3a80a..d9635436759 100644
--- a/refs.c
+++ b/refs.c
@@ -698,7 +698,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 		strbuf_addf(&path, *p, len, str);
 		ref = refs_resolve_ref_unsafe(refs, path.buf,
 					      RESOLVE_REF_READING,
-					      &hash, NULL);
+					      oid ? &hash : NULL, NULL);
 		if (!ref)
 			continue;
 		if (refs_reflog_exists(refs, path.buf))
@@ -710,7 +710,8 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 			continue;
 		if (!logs_found++) {
 			*log = xstrdup(it);
-			oidcpy(oid, &hash);
+			if (oid)
+				oidcpy(oid, &hash);
 		}
 		if (!warn_ambiguous_refs)
 			break;
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 08/12] refs/files: add a comment about refs_reflog_exists() call

[Ævar Arnfjörð Bjarmason]

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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ce4b3fb1c7a..92f49cfb7d4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3068,6 +3068,19 @@ 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 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 and the reflog doesn't exist after we've
+	 * acquired the lock that's OK. We've got nothing more to do;
+	 * We were asked to delete the reflog, but someone else
+	 * deleted it! The caller doesn't care that we deleted it,
+	 * just that it is deleted. So we can return successfully.
+	 */
 	if (!refs_reflog_exists(ref_store, refname)) {
 		unlock_ref(lock);
 		return 0;
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 09/12] reflog expire: don't lock reflogs using previously seen OID

[Ævar Arnfjörð Bjarmason]

During reflog expiry, the cmd_reflog_expire() function first iterates
over all reflogs in logs/*, and then one-by-one acquires the lock for
each one and expires it. This behavior has been with us since this
command was implemented in 4264dc15e1 ("git reflog expire",
2006-12-19).

Change this to stop calling lock_ref_oid_basic() with the OID we saw
when we looped over the logs, instead have it pass the OID it managed
to lock.

This mostly mitigates a race condition where e.g. "git gc" will fail
in a concurrently updated repository because the branch moved since
"git reflog expire --all" was started. I.e. with:

    error: cannot lock ref '<refname>': ref '<refname>' is at <OID-A> but expected <OID-B>

This behavior of passing in an "oid" was needed for an edge-case that
I've untangled in this and preceding commits though, namely that we
needed this OID because we'd:

 1. Lookup the reflog name/OID via dwim_log()
 2. With that OID, lock the reflog
 3. Later in builtin/reflog.c we use the OID we looked as input to
    lookup_commit_reference_gently(), assured that it's equal to the
    OID we got from dwim_log().

We can be sure that this change is safe to make because between
dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other
logic relevant to the OID or expiry run in the cmd_reflog_expire()
caller.

We can thus treat that code as a black box, before and after this
change it would get an OID that's been locked, the only difference is
that now we mostly won't be failing to get the lock due to the TOCTOU
race[0]. That failure was purely an implementation detail in how the
"current OID" was looked up, it was divorced from the locking
mechanism.

What do we mean with "mostly"? It mostly mitigates it because we'll
still run into cases where the ref is locked and being updated as we
want to expire it, and other git processes wanting to update the refs
will in turn race with us as we expire the reflog.

That remaining race can in turn be mitigated with the
core.filesRefLockTimeout setting, see 4ff0f01cb7 ("refs: retry
acquiring reference locks for 100ms", 2017-08-21). In practice if that
value is high enough we'll probably never have ref updates or reflog
expiry failing, since the clients involved will retry for far longer
than the time any of those operations could take.

See [1] for an initial report of how this impacted "git gc" and a
large discussion about this change in early 2019. In particular patch
looked good to Michael Haggerty, see his[2]. That message seems to not
have made it to the ML archive, its content is quoted in full in my
[3].

I'm leaving behind now-unused code the refs API etc. that takes the
now-NULL "oid" argument, and other code that can be simplified now
that we never have on OID in that context, that'll be cleaned up in
subsequent commits, but for now let's narrowly focus on fixing the
"git gc" issue. As the modified assert() shows we always pass a NULL
oid to reflog_expire() now.

Unfortunately this sort of probabilistic contention is hard to turn
into a test. I've tested this by running the following three subshells
in concurrent terminals:

    (
        rm -rf /tmp/git &&
        git init /tmp/git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >/tmp/git/out &&
            git -C /tmp/git add out &&
            git -C /tmp/git commit -m"out"
        done
    )

    (
	rm -rf /tmp/git-clone &&
        git clone file:///tmp/git /tmp/git-clone &&
        while git -C /tmp/git-clone pull
        do
            date
        done
    )

    (
        while git -C /tmp/git-clone reflog expire --all
        do
            date
        done
    )

Before this change the "reflog expire" would fail really quickly with
the "but expected" error noted above.

After this change both the "pull" and "reflog expire" will run for a
while, but eventually fail because I get unlucky with
core.filesRefLockTimeout (the "reflog expire" is in a really tight
loop). As noted above that can in turn be mitigated with higher values
of core.filesRefLockTimeout than the 100ms default.

As noted in the commentary added in the preceding commit there's also
the case of branches being racily deleted, that can be tested by
adding this to the above:

    (
        while git -C /tmp/git-clone branch topic master &&
	      git -C /tmp/git-clone branch -D topic
        do
            date
        done
    )

With core.filesRefLockTimeout set to 10 seconds (it can probably be a
lot lower) I managed to run all four of these concurrently for about
an hour, and accumulated ~125k commits, auto-gc's and all, and didn't
have a single failure. The loops visibly stall while waiting for the
lock, but that's expected and desired behavior.

0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use
1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/
2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu
3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c     | 13 ++++++-------
 refs/files-backend.c |  4 ++--
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 9f9e6bceb03..4506852c471 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -629,8 +629,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
+
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, &e->oid, flags,
+			status |= reflog_expire(e->reflog, NULL, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
@@ -642,13 +643,12 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (; i < argc; i++) {
 		char *ref;
-		struct object_id oid;
-		if (!dwim_log(argv[i], strlen(argv[i]), &oid, &ref)) {
+		if (!dwim_log(argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
-		status |= reflog_expire(ref, &oid, flags,
+		status |= reflog_expire(ref, NULL, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
@@ -700,7 +700,6 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 	for ( ; i < argc; i++) {
 		const char *spec = strstr(argv[i], "@{");
-		struct object_id oid;
 		char *ep, *ref;
 		int recno;
 
@@ -709,7 +708,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!dwim_log(argv[i], spec - argv[i], &oid, &ref)) {
+		if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) {
 			status |= error(_("no reflog for '%s'"), argv[i]);
 			continue;
 		}
@@ -724,7 +723,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.cmd.expire_total = 0;
 		}
 
-		status |= reflog_expire(ref, &oid, flags,
+		status |= reflog_expire(ref, NULL, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 92f49cfb7d4..819351c82fc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3060,7 +3060,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, oid,
+	lock = lock_ref_oid_basic(refs, refname, NULL,
 				  REF_NO_DEREF,
 				  &type, &err);
 	if (!lock) {
@@ -3111,7 +3111,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		}
 	}
 
-	assert(oideq(&lock->old_oid, oid));
+	assert(!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);
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 10/12] refs/files: remove unused "oid" in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

In the preceding commit the last caller that passed a non-NULL OID was
changed to pass NULL to lock_ref_oid_basic(). As noted in preceding
commits use of this API has been going away (we should use ref
transactions, or lock_raw_ref()), so we're unlikely to gain new
callers that want to pass the "oid".

So let's remove it, doing so means we can remove the "mustexist"
condition, and therefore anything except the "flags =
RESOLVE_REF_NO_RECURSE" case.

Furthermore, since the verify_lock() function we called did most of
its work when the "oid" was passed (as "old_oid") we can inline the
trivial part of it that remains in its only remaining caller. Without
a NULL "oid" passed it was equivalent to calling refs_read_ref_full()
followed by oidclr().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 72 ++++++++------------------------------------
 1 file changed, 12 insertions(+), 60 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 819351c82fc..8bbabc140b2 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -852,42 +852,6 @@ static struct ref_iterator *files_ref_iterator_begin(
 	return ref_iterator;
 }
 
-/*
- * Verify that the reference locked by lock has the value old_oid
- * (unless it is NULL).  Fail if the reference doesn't exist and
- * mustexist is set. Return 0 on success. On error, write an error
- * message to err, set errno, and return a negative value.
- */
-static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock,
-		       const struct object_id *old_oid, int mustexist,
-		       struct strbuf *err)
-{
-	assert(err);
-
-	if (refs_read_ref_full(ref_store, lock->ref_name,
-			       mustexist ? RESOLVE_REF_READING : 0,
-			       &lock->old_oid, NULL)) {
-		if (old_oid) {
-			int save_errno = errno;
-			strbuf_addf(err, "can't verify ref '%s'", lock->ref_name);
-			errno = save_errno;
-			return -1;
-		} else {
-			oidclr(&lock->old_oid);
-			return 0;
-		}
-	}
-	if (old_oid && !oideq(&lock->old_oid, old_oid)) {
-		strbuf_addf(err, "ref '%s' is at %s but expected %s",
-			    lock->ref_name,
-			    oid_to_hex(&lock->old_oid),
-			    oid_to_hex(old_oid));
-		errno = EBUSY;
-		return -1;
-	}
-	return 0;
-}
-
 static int remove_empty_directories(struct strbuf *path)
 {
 	/*
@@ -913,15 +877,12 @@ static int create_reflock(const char *path, void *cb)
  */
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
-					   const struct object_id *old_oid,
 					   unsigned int flags, int *type,
 					   struct strbuf *err)
 {
 	struct strbuf ref_file = STRBUF_INIT;
 	struct ref_lock *lock;
 	int last_errno = 0;
-	int mustexist = (old_oid && !is_null_oid(old_oid));
-	int resolve_flags = RESOLVE_REF_NO_RECURSE;
 	int resolved;
 
 	files_assert_main_repository(refs, "lock_ref_oid_basic");
@@ -929,12 +890,9 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 
 	CALLOC_ARRAY(lock, 1);
 
-	if (mustexist)
-		resolve_flags |= RESOLVE_REF_READING;
-
 	files_ref_path(refs, &ref_file, refname);
-	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 && errno == EISDIR) {
 		/*
@@ -952,8 +910,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					    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) {
@@ -988,10 +946,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) {
-		last_errno = errno;
-		goto error_return;
-	}
+	if (refs_read_ref_full(&refs->base, lock->ref_name,
+			       0,
+			       &lock->old_oid, NULL))
+		oidclr(&lock->old_oid);
 	goto out;
 
  error_return:
@@ -1410,8 +1368,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
-	lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
-				  &err);
+	lock = lock_ref_oid_basic(refs, newrefname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1433,8 +1390,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	goto out;
 
  rollback:
-	lock = lock_ref_oid_basic(refs, oldrefname, NULL,
-				  REF_NO_DEREF, NULL, &err);
+	lock = lock_ref_oid_basic(refs, oldrefname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
@@ -1841,9 +1797,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	struct ref_lock *lock;
 	int ret;
 
-	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  REF_NO_DEREF, NULL,
-				  &err);
+	lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -3060,9 +3014,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  REF_NO_DEREF,
-				  &type, &err);
+	lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 11/12] refs/files: remove unused "errno == EISDIR" code

[Ævar Arnfjörð Bjarmason]

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
in bc7127ef0f (ref locking: allow 'foo' when 'foo/bar' used to exist
but not anymore., 2006-09-30) seems like it should be dealing with.

Except it doesn't, and we never take this branch. The reason is that
when bc7127ef0f was written this looked like:

	ref = resolve_ref([...]);
	if (!ref && errno == EISDIR) {
	[...]

And in resolve_ref() we had this code:

	fd = open(path, O_RDONLY);
	if (fd < 0)
		return NULL;

I.e. we would attempt to read "foo" with open(), which would fail with
EISDIR and we'd return NULL. We'd then take this branch, call
remove_empty_directories() and continue.

Since a1c1d8170d (refs_resolve_ref_unsafe: handle d/f conflicts for
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()

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);
	if (fd < 0)
		/* get errno == EISDIR */
	/* later, in refs_resolve_ref_unsafe() */
	if ([...] && errno != EISDIR)
		return NULL;
	[...]
	/* returns the refs/heads/foo to the caller, even though it's a directory */
	return refname;

I.e. even though we got an "errno == EISDIR" we won't take this
branch, since in cases of EISDIR "resolved" is always
non-NULL. I.e. we pretend at this point as though everything's OK and
there is no "foo" directory.

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, 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 | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8bbabc140b2..f83aa1063f4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -883,7 +883,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	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);
@@ -891,30 +890,9 @@ 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
-		 * have foo/bar which now does not exist;
-		 * it is normal for the empty directory 'foo'
-		 * to remain.
-		 */
-		if (remove_empty_directories(&ref_file)) {
-			last_errno = errno;
-			if (!refs_verify_refname_available(
-					    &refs->base,
-					    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_REF_NO_RECURSE,
-						     &lock->old_oid, type);
-	}
-	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,
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH v3 12/12] refs/files: remove unused "errno != ENOTDIR" condition

[Ævar Arnfjörð Bjarmason]

As a follow-up to the preceding commit where we removed the adjacent
"errno == EISDIR" condition in the same function, remove the
"last_errno != ENOTDIR" condition here.

It's not possible for us to hit this condition added in
5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F
conflicts, 2015-05-11). Since a1c1d8170db (refs_resolve_ref_unsafe:
handle d/f conflicts for writes, 2017-10-06) we've explicitly caught
these in refs_resolve_ref_unsafe() before returning NULL:

	if (errno != ENOENT &&
	    errno != EISDIR &&
	    errno != ENOTDIR)
		return NULL;

We'd then always return the refname from refs_resolve_ref_unsafe()
even if we were in a broken state as explained in the preceding
commit. The elided context here is a call to refs_resolve_ref_unsafe().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f83aa1063f4..443182da102 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -894,8 +894,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 				     RESOLVE_REF_NO_RECURSE,
 				     &lock->old_oid, type)) {
 		last_errno = errno;
-		if (last_errno != ENOTDIR ||
-		    !refs_verify_refname_available(&refs->base, refname,
+		if (!refs_verify_refname_available(&refs->base, refname,
 						   NULL, NULL, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
 				    refname, strerror(last_errno));
-- 
2.32.0.874.ge7a9d58bfcf

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

[Ævar Arnfjörð Bjarmason]

This version hopefully addresses the weirdness of passing the
lock->old_oid as the "oid". We still do so, but the "refs API: pass
the "lock OID" to reflog "prepare"" and its change from s/const// is
gone, instead the relevant (and much smaller) part of that is squashed
into the "reflog expire: don't lock reflogs using previously seen OID"
change.

Æ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: 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
  refs/files: remove unused "errno != ENOTDIR" condition

 builtin/reflog.c      |  13 ++---
 reflog-walk.c         |   3 +-
 refs.c                |   5 +-
 refs.h                |   2 +-
 refs/debug.c          |   4 +-
 refs/files-backend.c  | 130 ++++++++++++------------------------------
 refs/packed-backend.c |   5 ++
 7 files changed, 53 insertions(+), 109 deletions(-)

Range-diff against v3:
 1:  737d2d8c3d =  1:  92fc3af072 refs/packet: add missing BUG() invocations to reflog callbacks
 2:  dfb9e34076 =  2:  67cd2331fb refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
 3:  0f2262fec6 =  3:  7d76514b55 refs/files: remove unused "extras/skip" in lock_ref_oid_basic()
 4:  7fb7ff9749 =  4:  852f86e666 refs/files: remove unused "skip" in lock_raw_ref() too
 5:  4e526c34aa =  5:  685b48328a refs/debug: re-indent argument list for "prepare"
 6:  295594fe8a <  -:  ---------- refs API: pass the "lock OID" to reflog "prepare"
 7:  e45ec439db =  6:  b75e7673d7 refs: make repo_dwim_log() accept a NULL oid
 8:  8ae8e5ac02 =  7:  7fe6c9bd92 refs/files: add a comment about refs_reflog_exists() call
 9:  1050743e27 !  8:  c9c2da3599 reflog expire: don't lock reflogs using previously seen OID
    @@ Commit message
         [3].
     
         I'm leaving behind now-unused code the refs API etc. that takes the
    -    now-NULL "oid" argument, and other code that can be simplified now
    +    now-NULL "unused_oid" argument, and other code that can be simplified now
         that we never have on OID in that context, that'll be cleaned up in
         subsequent commits, but for now let's narrowly focus on fixing the
         "git gc" issue. As the modified assert() shows we always pass a NULL
    @@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, cons
      					should_expire_reflog_ent,
      					reflog_expiry_cleanup,
     
    + ## refs.h ##
    +@@ refs.h: 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
    +
      ## refs/files-backend.c ##
    +@@ refs/files-backend.c: static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
    + }
    + 
    + static int files_reflog_expire(struct ref_store *ref_store,
    +-			       const char *refname, const struct object_id *oid,
    ++			       const char *refname, const struct object_id *unused_oid,
    + 			       unsigned int flags,
    + 			       reflog_expiry_prepare_fn prepare_fn,
    + 			       reflog_expiry_should_prune_fn should_prune_fn,
    +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
    + 	int status = 0;
    + 	int type;
    + 	struct strbuf err = STRBUF_INIT;
    ++	const struct object_id *oid;
    + 
    + 	memset(&cb, 0, sizeof(cb));
    + 	cb.flags = flags;
     @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
      	 * reference itself, plus we might need to update the
      	 * reference if --updateref was specified:
    @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
      				  REF_NO_DEREF,
      				  &type, &err);
      	if (!lock) {
    +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
    + 		strbuf_release(&err);
    + 		return -1;
    + 	}
    ++	oid = &lock->old_oid;
    + 
    + 	/*
    + 	 * When refs are deleted, their reflog is deleted before the
     @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
      		}
      	}
      
    --	assert(oideq(&lock->old_oid, oid));
    -+	assert(!oid);
    - 	(*prepare_fn)(refname, &lock->old_oid, cb.policy_cb);
    ++	assert(!unused_oid);
    + 	(*prepare_fn)(refname, oid, cb.policy_cb);
      	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
      	(*cleanup_fn)(cb.policy_cb);
10:  753c20f89b =  9:  b61c734cf5 refs/files: remove unused "oid" in lock_ref_oid_basic()
11:  8a71bbef97 = 10:  009abc9968 refs/files: remove unused "errno == EISDIR" code
12:  452253d597 = 11:  acb131cc1c refs/files: remove unused "errno != ENOTDIR" condition
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 01/11] refs/packet: add missing BUG() invocations to reflog callbacks

[Ævar Arnfjörð Bjarmason]

In e0cc8ac8202 (packed_ref_store: make class into a subclass of
`ref_store`, 2017-06-23) a die() was added to packed_create_reflog(),
but not to any of the other reflog callbacks, let's do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/packed-backend.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f8aa97d799..24a360b719 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1600,6 +1600,7 @@ static int packed_for_each_reflog_ent(struct ref_store *ref_store,
 				      const char *refname,
 				      each_reflog_ent_fn fn, void *cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1608,12 +1609,14 @@ static int packed_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 					      each_reflog_ent_fn fn,
 					      void *cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
 static int packed_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1627,6 +1630,7 @@ static int packed_create_reflog(struct ref_store *ref_store,
 static int packed_delete_reflog(struct ref_store *ref_store,
 			       const char *refname)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1638,6 +1642,7 @@ static int packed_reflog_expire(struct ref_store *ref_store,
 				reflog_expiry_cleanup_fn cleanup_fn,
 				void *policy_cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 02/11] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

The lock_ref_oid_basic() function has gradually been by most callers
no longer performing a low-level "acquire lock, update and release",
and instead using the ref transaction API. So there are only 4
remaining callers of lock_ref_oid_basic().

None of those callers pass REF_DELETING anymore, the last caller went
away in 92b1551b1d (refs: resolve symbolic refs first,
2016-04-25).

Before that we'd refactored and moved this code in:

 - 8df4e511387 (struct ref_update: move "have_old" into "flags",
   2015-02-17)

 - 7bd9bcf372d (refs: split filesystem-based refs code into a new
   file, 2015-11-09)

 - 165056b2fc (lock_ref_for_update(): new function, 2016-04-24)

We then finally stopped using it in 92b1551b1d (noted above). So let's
remove the handling of this parameter.

By itself this change doesn't benefit us much, but it's the start of
even more removal of unused code in and around this function in
subsequent commits.

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 677b7e4cdd..326f022421 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,
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 03/11] refs/files: remove unused "extras/skip" in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

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 non-NULL "extras" and "skip" parameters,
the last such caller went away in 92b1551b1d4 (refs: resolve symbolic
refs first, 2016-04-25), so let's remove the parameters.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 326f022421..a59823d667 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -915,8 +915,6 @@ static int create_reflock(const char *path, void *cb)
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
 					   const struct object_id *old_oid,
-					   const struct string_list *extras,
-					   const struct string_list *skip,
 					   unsigned int flags, int *type,
 					   struct strbuf *err)
 {
@@ -950,7 +948,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 			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;
@@ -963,7 +961,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
 		    !refs_verify_refname_available(&refs->base, refname,
-						   extras, skip, err))
+						   NULL, NULL, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
 				    refname, strerror(last_errno));
 
@@ -978,7 +976,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	 */
 	if (is_null_oid(&lock->old_oid) &&
 	    refs_verify_refname_available(refs->packed_ref_store, refname,
-					  extras, skip, err)) {
+					  NULL, NULL, err)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -1413,8 +1411,8 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
-	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL,
-				  REF_NO_DEREF, NULL, &err);
+	lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
+				  &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1436,7 +1434,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	goto out;
 
  rollback:
-	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL,
+	lock = lock_ref_oid_basic(refs, oldrefname, NULL,
 				  REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
@@ -1845,7 +1843,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	int ret;
 
 	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  NULL, NULL, REF_NO_DEREF, NULL,
+				  REF_NO_DEREF, NULL,
 				  &err);
 	if (!lock) {
 		error("%s", err.buf);
@@ -3064,7 +3062,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference if --updateref was specified:
 	 */
 	lock = lock_ref_oid_basic(refs, refname, oid,
-				  NULL, NULL, REF_NO_DEREF,
+				  REF_NO_DEREF,
 				  &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 04/11] refs/files: remove unused "skip" in lock_raw_ref() too

[Ævar Arnfjörð Bjarmason]

Remove the unused "skip" parameter to lock_raw_ref(), it was never
used. We do use it when passing "skip" to the
refs_rename_ref_available() function in files_copy_or_rename_ref(),
but not here.

This is part of a larger series that modifies lock_ref_oid_basic()
extensively, there will be no more modifications of this function in
this series, but since the preceding commit removed this unused
parameter from lock_ref_oid_basic(), let's do it here too for
consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a59823d667..af332fa8fe 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -531,7 +531,6 @@ static void unlock_ref(struct ref_lock *lock)
 static int lock_raw_ref(struct files_ref_store *refs,
 			const char *refname, int mustexist,
 			const struct string_list *extras,
-			const struct string_list *skip,
 			struct ref_lock **lock_p,
 			struct strbuf *referent,
 			unsigned int *type,
@@ -568,7 +567,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		 * reason to expect this error to be transitory.
 		 */
 		if (refs_verify_refname_available(&refs->base, refname,
-						  extras, skip, err)) {
+						  extras, NULL, err)) {
 			if (mustexist) {
 				/*
 				 * To the user the relevant error is
@@ -673,7 +672,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 							  REMOVE_DIR_EMPTY_ONLY)) {
 				if (refs_verify_refname_available(
 						    &refs->base, refname,
-						    extras, skip, err)) {
+						    extras, NULL, err)) {
 					/*
 					 * The error message set by
 					 * verify_refname_available() is OK.
@@ -710,7 +709,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		 */
 		if (refs_verify_refname_available(
 				    refs->packed_ref_store, refname,
-				    extras, skip, err))
+				    extras, NULL, err))
 			goto error_return;
 	}
 
@@ -2412,7 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	}
 
 	ret = lock_raw_ref(refs, update->refname, mustexist,
-			   affected_refnames, NULL,
+			   affected_refnames,
 			   &lock, &referent,
 			   &update->type, err);
 	if (ret) {
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 05/11] refs/debug: re-indent argument list for "prepare"

[Ævar Arnfjörð Bjarmason]

Re-indent this argument list that's been mis-indented since it was
added in 34c319970d1 (refs/debug: trace into reflog expiry too,
2021-04-23). This makes a subsequent change smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index 7db4abccc3..449ac3e6cc 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -364,8 +364,8 @@ struct debug_reflog_expiry_should_prune {
 };
 
 static void debug_reflog_expiry_prepare(const char *refname,
-				    const struct object_id *oid,
-				    void *cb_data)
+					const struct object_id *oid,
+					void *cb_data)
 {
 	struct debug_reflog_expiry_should_prune *prune = cb_data;
 	trace_printf_key(&trace_refs, "reflog_expire_prepare: %s\n", refname);
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 06/11] refs: make repo_dwim_log() accept a NULL oid

[Ævar Arnfjörð Bjarmason]

Change the repo_dwim_log() function initially added as dwim_log() in
eb3a48221fd (log --reflog: use dwim_log, 2007-02-09) to accept a NULL
oid parameter. The refs_resolve_ref_unsafe() function it invokes
already deals with it, but it didn't.

This allows for a bit more clarity in a reflog-walk.c codepath added
in f2eba66d4d1 (Enable HEAD@{...} and make it independent from the
current branch, 2007-02-03). We'll shortly use this in
builtin/reflog.c as well.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reflog-walk.c | 3 +--
 refs.c        | 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index e9cd328369..8ac4b284b6 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -158,10 +158,9 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 		}
 		reflogs = read_complete_reflog(branch);
 		if (!reflogs || reflogs->nr == 0) {
-			struct object_id oid;
 			char *b;
 			int ret = dwim_log(branch, strlen(branch),
-					   &oid, &b);
+					   NULL, &b);
 			if (ret > 1)
 				free(b);
 			else if (ret == 1) {
diff --git a/refs.c b/refs.c
index 8b9f7c3a80..d963543675 100644
--- a/refs.c
+++ b/refs.c
@@ -698,7 +698,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 		strbuf_addf(&path, *p, len, str);
 		ref = refs_resolve_ref_unsafe(refs, path.buf,
 					      RESOLVE_REF_READING,
-					      &hash, NULL);
+					      oid ? &hash : NULL, NULL);
 		if (!ref)
 			continue;
 		if (refs_reflog_exists(refs, path.buf))
@@ -710,7 +710,8 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 			continue;
 		if (!logs_found++) {
 			*log = xstrdup(it);
-			oidcpy(oid, &hash);
+			if (oid)
+				oidcpy(oid, &hash);
 		}
 		if (!warn_ambiguous_refs)
 			break;
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 07/11] refs/files: add a comment about refs_reflog_exists() call

[Ævar Arnfjörð Bjarmason]

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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index af332fa8fe..5415306416 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3068,6 +3068,19 @@ 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 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 and the reflog doesn't exist after we've
+	 * acquired the lock that's OK. We've got nothing more to do;
+	 * We were asked to delete the reflog, but someone else
+	 * deleted it! The caller doesn't care that we deleted it,
+	 * just that it is deleted. So we can return successfully.
+	 */
 	if (!refs_reflog_exists(ref_store, refname)) {
 		unlock_ref(lock);
 		return 0;
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 08/11] reflog expire: don't lock reflogs using previously seen OID

[Ævar Arnfjörð Bjarmason]

During reflog expiry, the cmd_reflog_expire() function first iterates
over all reflogs in logs/*, and then one-by-one acquires the lock for
each one and expires it. This behavior has been with us since this
command was implemented in 4264dc15e1 ("git reflog expire",
2006-12-19).

Change this to stop calling lock_ref_oid_basic() with the OID we saw
when we looped over the logs, instead have it pass the OID it managed
to lock.

This mostly mitigates a race condition where e.g. "git gc" will fail
in a concurrently updated repository because the branch moved since
"git reflog expire --all" was started. I.e. with:

    error: cannot lock ref '<refname>': ref '<refname>' is at <OID-A> but expected <OID-B>

This behavior of passing in an "oid" was needed for an edge-case that
I've untangled in this and preceding commits though, namely that we
needed this OID because we'd:

 1. Lookup the reflog name/OID via dwim_log()
 2. With that OID, lock the reflog
 3. Later in builtin/reflog.c we use the OID we looked as input to
    lookup_commit_reference_gently(), assured that it's equal to the
    OID we got from dwim_log().

We can be sure that this change is safe to make because between
dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other
logic relevant to the OID or expiry run in the cmd_reflog_expire()
caller.

We can thus treat that code as a black box, before and after this
change it would get an OID that's been locked, the only difference is
that now we mostly won't be failing to get the lock due to the TOCTOU
race[0]. That failure was purely an implementation detail in how the
"current OID" was looked up, it was divorced from the locking
mechanism.

What do we mean with "mostly"? It mostly mitigates it because we'll
still run into cases where the ref is locked and being updated as we
want to expire it, and other git processes wanting to update the refs
will in turn race with us as we expire the reflog.

That remaining race can in turn be mitigated with the
core.filesRefLockTimeout setting, see 4ff0f01cb7 ("refs: retry
acquiring reference locks for 100ms", 2017-08-21). In practice if that
value is high enough we'll probably never have ref updates or reflog
expiry failing, since the clients involved will retry for far longer
than the time any of those operations could take.

See [1] for an initial report of how this impacted "git gc" and a
large discussion about this change in early 2019. In particular patch
looked good to Michael Haggerty, see his[2]. That message seems to not
have made it to the ML archive, its content is quoted in full in my
[3].

I'm leaving behind now-unused code the refs API etc. that takes the
now-NULL "unused_oid" argument, and other code that can be simplified now
that we never have on OID in that context, that'll be cleaned up in
subsequent commits, but for now let's narrowly focus on fixing the
"git gc" issue. As the modified assert() shows we always pass a NULL
oid to reflog_expire() now.

Unfortunately this sort of probabilistic contention is hard to turn
into a test. I've tested this by running the following three subshells
in concurrent terminals:

    (
        rm -rf /tmp/git &&
        git init /tmp/git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >/tmp/git/out &&
            git -C /tmp/git add out &&
            git -C /tmp/git commit -m"out"
        done
    )

    (
	rm -rf /tmp/git-clone &&
        git clone file:///tmp/git /tmp/git-clone &&
        while git -C /tmp/git-clone pull
        do
            date
        done
    )

    (
        while git -C /tmp/git-clone reflog expire --all
        do
            date
        done
    )

Before this change the "reflog expire" would fail really quickly with
the "but expected" error noted above.

After this change both the "pull" and "reflog expire" will run for a
while, but eventually fail because I get unlucky with
core.filesRefLockTimeout (the "reflog expire" is in a really tight
loop). As noted above that can in turn be mitigated with higher values
of core.filesRefLockTimeout than the 100ms default.

As noted in the commentary added in the preceding commit there's also
the case of branches being racily deleted, that can be tested by
adding this to the above:

    (
        while git -C /tmp/git-clone branch topic master &&
	      git -C /tmp/git-clone branch -D topic
        do
            date
        done
    )

With core.filesRefLockTimeout set to 10 seconds (it can probably be a
lot lower) I managed to run all four of these concurrently for about
an hour, and accumulated ~125k commits, auto-gc's and all, and didn't
have a single failure. The loops visibly stall while waiting for the
lock, but that's expected and desired behavior.

0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use
1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/
2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu
3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c     | 13 ++++++-------
 refs.h               |  2 +-
 refs/files-backend.c |  7 +++++--
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 09541d1c80..61795f22d5 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -629,8 +629,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
+
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, &e->oid, flags,
+			status |= reflog_expire(e->reflog, NULL, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
@@ -642,13 +643,12 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (; i < argc; i++) {
 		char *ref;
-		struct object_id oid;
-		if (!dwim_log(argv[i], strlen(argv[i]), &oid, &ref)) {
+		if (!dwim_log(argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
-		status |= reflog_expire(ref, &oid, flags,
+		status |= reflog_expire(ref, NULL, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
@@ -700,7 +700,6 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 	for ( ; i < argc; i++) {
 		const char *spec = strstr(argv[i], "@{");
-		struct object_id oid;
 		char *ep, *ref;
 		int recno;
 
@@ -709,7 +708,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!dwim_log(argv[i], spec - argv[i], &oid, &ref)) {
+		if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) {
 			status |= error(_("no reflog for '%s'"), argv[i]);
 			continue;
 		}
@@ -724,7 +723,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.cmd.expire_total = 0;
 		}
 
-		status |= reflog_expire(ref, &oid, flags,
+		status |= reflog_expire(ref, NULL, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
diff --git a/refs.h b/refs.h
index 48970dfc7e..ddbf15f1c2 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
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5415306416..ccdf455049 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3032,7 +3032,7 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 }
 
 static int files_reflog_expire(struct ref_store *ref_store,
-			       const char *refname, const struct object_id *oid,
+			       const char *refname, const struct object_id *unused_oid,
 			       unsigned int flags,
 			       reflog_expiry_prepare_fn prepare_fn,
 			       reflog_expiry_should_prune_fn should_prune_fn,
@@ -3049,6 +3049,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	int status = 0;
 	int type;
 	struct strbuf err = STRBUF_INIT;
+	const struct object_id *oid;
 
 	memset(&cb, 0, sizeof(cb));
 	cb.flags = flags;
@@ -3060,7 +3061,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, oid,
+	lock = lock_ref_oid_basic(refs, refname, NULL,
 				  REF_NO_DEREF,
 				  &type, &err);
 	if (!lock) {
@@ -3068,6 +3069,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		strbuf_release(&err);
 		return -1;
 	}
+	oid = &lock->old_oid;
 
 	/*
 	 * When refs are deleted, their reflog is deleted before the
@@ -3111,6 +3113,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		}
 	}
 
+	assert(!unused_oid);
 	(*prepare_fn)(refname, oid, cb.policy_cb);
 	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
 	(*cleanup_fn)(cb.policy_cb);
-- 
2.32.0.956.g6b0c84ceda8

Re: [PATCH v4 08/11] reflog expire: don't lock reflogs using previously seen OID

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> During reflog expiry, the cmd_reflog_expire() function first iterates
> ...
> I'm leaving behind now-unused code the refs API etc. that takes the
> now-NULL "unused_oid" argument, and other code that can be simplified now
> that we never have on OID in that context, that'll be cleaned up in
> subsequent commits, but for now let's narrowly focus on fixing the
> "git gc" issue. As the modified assert() shows we always pass a NULL
> oid to reflog_expire() now.

OK.  Nicely described.

>  static int files_reflog_expire(struct ref_store *ref_store,
> -			       const char *refname, const struct object_id *oid,
> +			       const char *refname, const struct object_id *unused_oid,
>  			       unsigned int flags,
>  			       reflog_expiry_prepare_fn prepare_fn,
>  			       reflog_expiry_should_prune_fn should_prune_fn,
> @@ -3049,6 +3049,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  	int status = 0;
>  	int type;
>  	struct strbuf err = STRBUF_INIT;
> +	const struct object_id *oid;
>  
>  	memset(&cb, 0, sizeof(cb));
>  	cb.flags = flags;
> @@ -3060,7 +3061,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  	 * reference itself, plus we might need to update the
>  	 * reference if --updateref was specified:
>  	 */
> -	lock = lock_ref_oid_basic(refs, refname, oid,
> +	lock = lock_ref_oid_basic(refs, refname, NULL,
>  				  REF_NO_DEREF,
>  				  &type, &err);
>  	if (!lock) {
> @@ -3068,6 +3069,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  		strbuf_release(&err);
>  		return -1;
>  	}
> +	oid = &lock->old_oid;

OK.  That makes it more clear that the object name the locking code
read is what gets used.

> @@ -3111,6 +3113,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  		}
>  	}
>  
> +	assert(!unused_oid);
>  	(*prepare_fn)(refname, oid, cb.policy_cb);
>  	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
>  	(*cleanup_fn)(cb.policy_cb);

The preference in this codebase is

	ptr_to_function(params);

over

	(*ptr_to_function)(params);

Once it is written and committed, it is not worth changing, but just
for the record...

THanks.

Re: [PATCH v4 08/11] reflog expire: don't lock reflogs using previously seen OID

[Ævar Arnfjörð Bjarmason]

On Mon, Aug 02 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> [...]
>> @@ -3111,6 +3113,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>>  		}
>>  	}
>>  
>> +	assert(!unused_oid);
>>  	(*prepare_fn)(refname, oid, cb.policy_cb);
>>  	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
>>  	(*cleanup_fn)(cb.policy_cb);
>
> The preference in this codebase is
>
> 	ptr_to_function(params);
>
> over
>
> 	(*ptr_to_function)(params);
>
> Once it is written and committed, it is not worth changing, but just
> for the record...

Indeed, in this series I don't touch that (except items on the argument
list), so refactoring that just for changing the syntax here didn't seem
worth it.

[PATCH v4 09/11] refs/files: remove unused "oid" in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

In the preceding commit the last caller that passed a non-NULL OID was
changed to pass NULL to lock_ref_oid_basic(). As noted in preceding
commits use of this API has been going away (we should use ref
transactions, or lock_raw_ref()), so we're unlikely to gain new
callers that want to pass the "oid".

So let's remove it, doing so means we can remove the "mustexist"
condition, and therefore anything except the "flags =
RESOLVE_REF_NO_RECURSE" case.

Furthermore, since the verify_lock() function we called did most of
its work when the "oid" was passed (as "old_oid") we can inline the
trivial part of it that remains in its only remaining caller. Without
a NULL "oid" passed it was equivalent to calling refs_read_ref_full()
followed by oidclr().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 72 ++++++++------------------------------------
 1 file changed, 12 insertions(+), 60 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ccdf455049..98d9aa1601 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -852,42 +852,6 @@ static struct ref_iterator *files_ref_iterator_begin(
 	return ref_iterator;
 }
 
-/*
- * Verify that the reference locked by lock has the value old_oid
- * (unless it is NULL).  Fail if the reference doesn't exist and
- * mustexist is set. Return 0 on success. On error, write an error
- * message to err, set errno, and return a negative value.
- */
-static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock,
-		       const struct object_id *old_oid, int mustexist,
-		       struct strbuf *err)
-{
-	assert(err);
-
-	if (refs_read_ref_full(ref_store, lock->ref_name,
-			       mustexist ? RESOLVE_REF_READING : 0,
-			       &lock->old_oid, NULL)) {
-		if (old_oid) {
-			int save_errno = errno;
-			strbuf_addf(err, "can't verify ref '%s'", lock->ref_name);
-			errno = save_errno;
-			return -1;
-		} else {
-			oidclr(&lock->old_oid);
-			return 0;
-		}
-	}
-	if (old_oid && !oideq(&lock->old_oid, old_oid)) {
-		strbuf_addf(err, "ref '%s' is at %s but expected %s",
-			    lock->ref_name,
-			    oid_to_hex(&lock->old_oid),
-			    oid_to_hex(old_oid));
-		errno = EBUSY;
-		return -1;
-	}
-	return 0;
-}
-
 static int remove_empty_directories(struct strbuf *path)
 {
 	/*
@@ -913,15 +877,12 @@ static int create_reflock(const char *path, void *cb)
  */
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
-					   const struct object_id *old_oid,
 					   unsigned int flags, int *type,
 					   struct strbuf *err)
 {
 	struct strbuf ref_file = STRBUF_INIT;
 	struct ref_lock *lock;
 	int last_errno = 0;
-	int mustexist = (old_oid && !is_null_oid(old_oid));
-	int resolve_flags = RESOLVE_REF_NO_RECURSE;
 	int resolved;
 
 	files_assert_main_repository(refs, "lock_ref_oid_basic");
@@ -929,12 +890,9 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 
 	CALLOC_ARRAY(lock, 1);
 
-	if (mustexist)
-		resolve_flags |= RESOLVE_REF_READING;
-
 	files_ref_path(refs, &ref_file, refname);
-	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 && errno == EISDIR) {
 		/*
@@ -952,8 +910,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					    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) {
@@ -988,10 +946,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) {
-		last_errno = errno;
-		goto error_return;
-	}
+	if (refs_read_ref_full(&refs->base, lock->ref_name,
+			       0,
+			       &lock->old_oid, NULL))
+		oidclr(&lock->old_oid);
 	goto out;
 
  error_return:
@@ -1410,8 +1368,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
-	lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
-				  &err);
+	lock = lock_ref_oid_basic(refs, newrefname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1433,8 +1390,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	goto out;
 
  rollback:
-	lock = lock_ref_oid_basic(refs, oldrefname, NULL,
-				  REF_NO_DEREF, NULL, &err);
+	lock = lock_ref_oid_basic(refs, oldrefname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
@@ -1841,9 +1797,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	struct ref_lock *lock;
 	int ret;
 
-	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  REF_NO_DEREF, NULL,
-				  &err);
+	lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -3061,9 +3015,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  REF_NO_DEREF,
-				  &type, &err);
+	lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 10/11] refs/files: remove unused "errno == EISDIR" code

[Ævar Arnfjörð Bjarmason]

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
in bc7127ef0f (ref locking: allow 'foo' when 'foo/bar' used to exist
but not anymore., 2006-09-30) seems like it should be dealing with.

Except it doesn't, and we never take this branch. The reason is that
when bc7127ef0f was written this looked like:

	ref = resolve_ref([...]);
	if (!ref && errno == EISDIR) {
	[...]

And in resolve_ref() we had this code:

	fd = open(path, O_RDONLY);
	if (fd < 0)
		return NULL;

I.e. we would attempt to read "foo" with open(), which would fail with
EISDIR and we'd return NULL. We'd then take this branch, call
remove_empty_directories() and continue.

Since a1c1d8170d (refs_resolve_ref_unsafe: handle d/f conflicts for
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()

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);
	if (fd < 0)
		/* get errno == EISDIR */
	/* later, in refs_resolve_ref_unsafe() */
	if ([...] && errno != EISDIR)
		return NULL;
	[...]
	/* returns the refs/heads/foo to the caller, even though it's a directory */
	return refname;

I.e. even though we got an "errno == EISDIR" we won't take this
branch, since in cases of EISDIR "resolved" is always
non-NULL. I.e. we pretend at this point as though everything's OK and
there is no "foo" directory.

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, 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 | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 98d9aa1601..28cd8853f5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -883,7 +883,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	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);
@@ -891,30 +890,9 @@ 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
-		 * have foo/bar which now does not exist;
-		 * it is normal for the empty directory 'foo'
-		 * to remain.
-		 */
-		if (remove_empty_directories(&ref_file)) {
-			last_errno = errno;
-			if (!refs_verify_refname_available(
-					    &refs->base,
-					    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_REF_NO_RECURSE,
-						     &lock->old_oid, type);
-	}
-	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,
-- 
2.32.0.956.g6b0c84ceda8

[PATCH v4 11/11] refs/files: remove unused "errno != ENOTDIR" condition

[Ævar Arnfjörð Bjarmason]

As a follow-up to the preceding commit where we removed the adjacent
"errno == EISDIR" condition in the same function, remove the
"last_errno != ENOTDIR" condition here.

It's not possible for us to hit this condition added in
5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F
conflicts, 2015-05-11). Since a1c1d8170db (refs_resolve_ref_unsafe:
handle d/f conflicts for writes, 2017-10-06) we've explicitly caught
these in refs_resolve_ref_unsafe() before returning NULL:

	if (errno != ENOENT &&
	    errno != EISDIR &&
	    errno != ENOTDIR)
		return NULL;

We'd then always return the refname from refs_resolve_ref_unsafe()
even if we were in a broken state as explained in the preceding
commit. The elided context here is a call to refs_resolve_ref_unsafe().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 28cd8853f5..5d12003471 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -894,8 +894,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 				     RESOLVE_REF_NO_RECURSE,
 				     &lock->old_oid, type)) {
 		last_errno = errno;
-		if (last_errno != ENOTDIR ||
-		    !refs_verify_refname_available(&refs->base, refname,
+		if (!refs_verify_refname_available(&refs->base, refname,
 						   NULL, NULL, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
 				    refname, strerror(last_errno));
-- 
2.32.0.956.g6b0c84ceda8

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

[Ævar Arnfjörð Bjarmason]

A re-roll of
https://lore.kernel.org/git/cover-00.11-0000000000-20210726T234237Z-avarab@gmail.com

This incorporates Jeff King's
https://lore.kernel.org/git/YQyGe9qOPRA0nf5v@coredump.intra.peff.net;
so this replaces ab/refs-files-cleanup and
jk/refs-files-cleanup-cleanup.

There's also a trivial commit message grammar fix here that Junio
already squashed into his version of v4. The rest of the changes are
all due to the rebase on Jeff's commit to get rid of the "flags"
parameter.

But most importantly there's a new "refs API: remove OID argument to
reflog_expire()" patch here, to change the function signature of
reflog_expire() to not pass the OID. Leaving it in place caused a
segfault in the reftable topic, now it just won't compile in
combination with this. I'm submitting update(s) to those other topics
on top to make these all work nicely together.

Jeff King (1):
  refs: drop unused "flags" parameter to lock_ref_oid_basic()

Ævar Arnfjörð Bjarmason (12):
  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: 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 API: remove OID argument to reflog_expire()
  refs/files: remove unused "oid" in lock_ref_oid_basic()
  refs/files: remove unused "errno == EISDIR" code
  refs/files: remove unused "errno != ENOTDIR" condition

 builtin/reflog.c      |  13 ++---
 reflog-walk.c         |   3 +-
 refs.c                |  13 +++--
 refs.h                |   9 ++-
 refs/debug.c          |   8 +--
 refs/files-backend.c  | 132 +++++++++++-------------------------------
 refs/packed-backend.c |   7 ++-
 refs/refs-internal.h  |   2 +-
 8 files changed, 64 insertions(+), 123 deletions(-)

Range-diff against v4:
 1:  92fc3af0727 =  1:  61cf49b9582 refs/packet: add missing BUG() invocations to reflog callbacks
 2:  67cd2331fb4 !  2:  a20548c1a4d refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
    @@ Metadata
      ## Commit message ##
         refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
     
    -    The lock_ref_oid_basic() function has gradually been by most callers
    -    no longer performing a low-level "acquire lock, update and release",
    -    and instead using the ref transaction API. So there are only 4
    -    remaining callers of lock_ref_oid_basic().
    +    The lock_ref_oid_basic() function has gradually been replaced by
    +    most callers no longer performing a low-level "acquire lock,
    +    update and release", and instead using the ref transaction API.
    +    So there are only 4 remaining callers of lock_ref_oid_basic().
     
         None of those callers pass REF_DELETING anymore, the last caller went
         away in 92b1551b1d (refs: resolve symbolic refs first,
 -:  ----------- >  3:  d3216a6b1d8 refs: drop unused "flags" parameter to lock_ref_oid_basic()
 3:  7d76514b559 !  4:  3e538eb3008 refs/files: remove unused "extras/skip" in lock_ref_oid_basic()
    @@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)
      					   const struct object_id *old_oid,
     -					   const struct string_list *extras,
     -					   const struct string_list *skip,
    - 					   unsigned int flags, int *type,
    - 					   struct strbuf *err)
    + 					   int *type, struct strbuf *err)
      {
    + 	struct strbuf ref_file = STRBUF_INIT;
     @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
      			last_errno = errno;
      			if (!refs_verify_refname_available(
    @@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_
      
      	logmoved = log;
      
    +-
     -	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL,
    --				  REF_NO_DEREF, NULL, &err);
    -+	lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
    -+				  &err);
    +-				  NULL, &err);
    ++	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, &err);
      	if (!lock) {
      		if (copy)
      			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
    @@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_
      
       rollback:
     -	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL,
    -+	lock = lock_ref_oid_basic(refs, oldrefname, NULL,
    - 				  REF_NO_DEREF, NULL, &err);
    +-				  NULL, &err);
    ++	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, &err);
      	if (!lock) {
      		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
    + 		strbuf_release(&err);
     @@ refs/files-backend.c: static int files_create_symref(struct ref_store *ref_store,
    + 	struct ref_lock *lock;
      	int ret;
      
    - 	lock = lock_ref_oid_basic(refs, refname, NULL,
    --				  NULL, NULL, REF_NO_DEREF, NULL,
    -+				  REF_NO_DEREF, NULL,
    - 				  &err);
    +-	lock = lock_ref_oid_basic(refs, refname, NULL,
    +-				  NULL, NULL, NULL,
    +-				  &err);
    ++	lock = lock_ref_oid_basic(refs, refname, NULL, NULL, &err);
      	if (!lock) {
      		error("%s", err.buf);
    + 		strbuf_release(&err);
     @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
    + 	 * reference itself, plus we might need to update the
      	 * reference if --updateref was specified:
      	 */
    - 	lock = lock_ref_oid_basic(refs, refname, oid,
    --				  NULL, NULL, REF_NO_DEREF,
    -+				  REF_NO_DEREF,
    - 				  &type, &err);
    +-	lock = lock_ref_oid_basic(refs, refname, oid,
    +-				  NULL, NULL, &type, &err);
    ++	lock = lock_ref_oid_basic(refs, refname, oid, &type, &err);
      	if (!lock) {
      		error("cannot lock ref '%s': %s", refname, err.buf);
    + 		strbuf_release(&err);
 4:  852f86e666f =  5:  b7335e79f8b refs/files: remove unused "skip" in lock_raw_ref() too
 5:  685b48328af =  6:  24449766060 refs/debug: re-indent argument list for "prepare"
 6:  b75e7673d70 =  7:  3b7daf03e5a refs: make repo_dwim_log() accept a NULL oid
 7:  7fe6c9bd921 =  8:  51abe459e70 refs/files: add a comment about refs_reflog_exists() call
 8:  c9c2da35997 !  9:  aba12606cea reflog expire: don't lock reflogs using previously seen OID
    @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
      	 * reference itself, plus we might need to update the
      	 * reference if --updateref was specified:
      	 */
    --	lock = lock_ref_oid_basic(refs, refname, oid,
    -+	lock = lock_ref_oid_basic(refs, refname, NULL,
    - 				  REF_NO_DEREF,
    - 				  &type, &err);
    +-	lock = lock_ref_oid_basic(refs, refname, oid, &type, &err);
    ++	lock = lock_ref_oid_basic(refs, refname, NULL, &type, &err);
      	if (!lock) {
    -@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
    + 		error("cannot lock ref '%s': %s", refname, err.buf);
      		strbuf_release(&err);
      		return -1;
      	}
 -:  ----------- > 10:  5afa8f1be29 refs API: remove OID argument to reflog_expire()
 9:  b61c734cf5c ! 11:  7712e29abe6 refs/files: remove unused "oid" in lock_ref_oid_basic()
    @@ refs/files-backend.c: static struct ref_iterator *files_ref_iterator_begin(
      {
      	/*
     @@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)
    +  * On failure errno is set to something meaningful.
       */
      static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
    - 					   const char *refname,
    +-					   const char *refname,
     -					   const struct object_id *old_oid,
    - 					   unsigned int flags, int *type,
    - 					   struct strbuf *err)
    +-					   int *type, struct strbuf *err)
    ++					   const char *refname, int *type,
    ++					   struct strbuf *err)
      {
      	struct strbuf ref_file = STRBUF_INIT;
      	struct ref_lock *lock;
    @@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_
      
      	logmoved = log;
      
    --	lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
    --				  &err);
    -+	lock = lock_ref_oid_basic(refs, newrefname, REF_NO_DEREF, NULL, &err);
    +-	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, &err);
    ++	lock = lock_ref_oid_basic(refs, newrefname, NULL, &err);
      	if (!lock) {
      		if (copy)
      			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
    @@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_
      	goto out;
      
       rollback:
    --	lock = lock_ref_oid_basic(refs, oldrefname, NULL,
    --				  REF_NO_DEREF, NULL, &err);
    -+	lock = lock_ref_oid_basic(refs, oldrefname, REF_NO_DEREF, NULL, &err);
    +-	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, &err);
    ++	lock = lock_ref_oid_basic(refs, oldrefname, NULL, &err);
      	if (!lock) {
      		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
      		strbuf_release(&err);
    @@ refs/files-backend.c: static int files_create_symref(struct ref_store *ref_store
      	struct ref_lock *lock;
      	int ret;
      
    --	lock = lock_ref_oid_basic(refs, refname, NULL,
    --				  REF_NO_DEREF, NULL,
    --				  &err);
    -+	lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, NULL, &err);
    +-	lock = lock_ref_oid_basic(refs, refname, NULL, NULL, &err);
    ++	lock = lock_ref_oid_basic(refs, refname, NULL, &err);
      	if (!lock) {
      		error("%s", err.buf);
      		strbuf_release(&err);
    @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
      	 * reference itself, plus we might need to update the
      	 * reference if --updateref was specified:
      	 */
    --	lock = lock_ref_oid_basic(refs, refname, NULL,
    --				  REF_NO_DEREF,
    --				  &type, &err);
    -+	lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, &type, &err);
    +-	lock = lock_ref_oid_basic(refs, refname, NULL, &type, &err);
    ++	lock = lock_ref_oid_basic(refs, refname, &type, &err);
      	if (!lock) {
      		error("cannot lock ref '%s': %s", refname, err.buf);
      		strbuf_release(&err);
10:  009abc99688 = 12:  f746939a27a refs/files: remove unused "errno == EISDIR" code
11:  acb131cc1c5 = 13:  2e30ee04edb refs/files: remove unused "errno != ENOTDIR" condition
-- 
2.33.0.662.g438caf9576d

[PATCH v5 01/13] refs/packet: add missing BUG() invocations to reflog callbacks

[Ævar Arnfjörð Bjarmason]

In e0cc8ac8202 (packed_ref_store: make class into a subclass of
`ref_store`, 2017-06-23) a die() was added to packed_create_reflog(),
but not to any of the other reflog callbacks, let's do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/packed-backend.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f8aa97d7998..24a360b719f 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1600,6 +1600,7 @@ static int packed_for_each_reflog_ent(struct ref_store *ref_store,
 				      const char *refname,
 				      each_reflog_ent_fn fn, void *cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1608,12 +1609,14 @@ static int packed_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 					      each_reflog_ent_fn fn,
 					      void *cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
 static int packed_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1627,6 +1630,7 @@ static int packed_create_reflog(struct ref_store *ref_store,
 static int packed_delete_reflog(struct ref_store *ref_store,
 			       const char *refname)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
@@ -1638,6 +1642,7 @@ static int packed_reflog_expire(struct ref_store *ref_store,
 				reflog_expiry_cleanup_fn cleanup_fn,
 				void *policy_cb_data)
 {
+	BUG("packed reference store does not support reflogs");
 	return 0;
 }
 
-- 
2.33.0.662.g438caf9576d

[PATCH v5 02/13] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

The lock_ref_oid_basic() function has gradually been replaced by
most callers no longer performing a low-level "acquire lock,
update and release", and instead using the ref transaction API.
So there are only 4 remaining callers of lock_ref_oid_basic().

None of those callers pass REF_DELETING anymore, the last caller went
away in 92b1551b1d (refs: resolve symbolic refs first,
2016-04-25).

Before that we'd refactored and moved this code in:

 - 8df4e511387 (struct ref_update: move "have_old" into "flags",
   2015-02-17)

 - 7bd9bcf372d (refs: split filesystem-based refs code into a new
   file, 2015-11-09)

 - 165056b2fc (lock_ref_for_update(): new function, 2016-04-24)

We then finally stopped using it in 92b1551b1d (noted above). So let's
remove the handling of this parameter.

By itself this change doesn't benefit us much, but it's the start of
even more removal of unused code in and around this function in
subsequent commits.

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,
-- 
2.33.0.662.g438caf9576d

[PATCH v5 03/13] refs: drop unused "flags" parameter to lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

From: Jeff King <peff@peff.net>

In the last commit we removed the REF_DELETING flag from
lock_ref_oid_basic(). Since then all of the remaining callers do pass
REF_NO_DEREF, but that has been ignored completely since
7a418f3a17 (lock_ref_sha1_basic(): only handle REF_NODEREF mode,
2016-04-22).

So we can simply get rid of the parameter entirely.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 326f0224218..e73458e2571 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -917,8 +917,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const struct object_id *old_oid,
 					   const struct string_list *extras,
 					   const struct string_list *skip,
-					   unsigned int flags, int *type,
-					   struct strbuf *err)
+					   int *type, struct strbuf *err)
 {
 	struct strbuf ref_file = STRBUF_INIT;
 	struct ref_lock *lock;
@@ -1413,8 +1412,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
+
 	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL,
-				  REF_NO_DEREF, NULL, &err);
+				  NULL, &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1437,7 +1437,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
  rollback:
 	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL,
-				  REF_NO_DEREF, NULL, &err);
+				  NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
@@ -1845,7 +1845,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	int ret;
 
 	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  NULL, NULL, REF_NO_DEREF, NULL,
+				  NULL, NULL, NULL,
 				  &err);
 	if (!lock) {
 		error("%s", err.buf);
@@ -3064,8 +3064,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference if --updateref was specified:
 	 */
 	lock = lock_ref_oid_basic(refs, refname, oid,
-				  NULL, NULL, REF_NO_DEREF,
-				  &type, &err);
+				  NULL, NULL, &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
-- 
2.33.0.662.g438caf9576d

[PATCH v5 04/13] refs/files: remove unused "extras/skip" in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

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 non-NULL "extras" and "skip" parameters,
the last such caller went away in 92b1551b1d4 (refs: resolve symbolic
refs first, 2016-04-25), so let's remove the parameters.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e73458e2571..69f7f54e03b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -915,8 +915,6 @@ static int create_reflock(const char *path, void *cb)
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
 					   const struct object_id *old_oid,
-					   const struct string_list *extras,
-					   const struct string_list *skip,
 					   int *type, struct strbuf *err)
 {
 	struct strbuf ref_file = STRBUF_INIT;
@@ -949,7 +947,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 			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;
@@ -962,7 +960,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
 		    !refs_verify_refname_available(&refs->base, refname,
-						   extras, skip, err))
+						   NULL, NULL, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
 				    refname, strerror(last_errno));
 
@@ -977,7 +975,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	 */
 	if (is_null_oid(&lock->old_oid) &&
 	    refs_verify_refname_available(refs->packed_ref_store, refname,
-					  extras, skip, err)) {
+					  NULL, NULL, err)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -1412,9 +1410,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
-
-	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL,
-				  NULL, &err);
+	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1436,8 +1432,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	goto out;
 
  rollback:
-	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL,
-				  NULL, &err);
+	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
@@ -1844,9 +1839,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	struct ref_lock *lock;
 	int ret;
 
-	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  NULL, NULL, NULL,
-				  &err);
+	lock = lock_ref_oid_basic(refs, refname, NULL, NULL, &err);
 	if (!lock) {
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -3063,8 +3056,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, oid,
-				  NULL, NULL, &type, &err);
+	lock = lock_ref_oid_basic(refs, refname, oid, &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
-- 
2.33.0.662.g438caf9576d

[PATCH v5 05/13] refs/files: remove unused "skip" in lock_raw_ref() too

[Ævar Arnfjörð Bjarmason]

Remove the unused "skip" parameter to lock_raw_ref(), it was never
used. We do use it when passing "skip" to the
refs_rename_ref_available() function in files_copy_or_rename_ref(),
but not here.

This is part of a larger series that modifies lock_ref_oid_basic()
extensively, there will be no more modifications of this function in
this series, but since the preceding commit removed this unused
parameter from lock_ref_oid_basic(), let's do it here too for
consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 69f7f54e03b..997e021c1c4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -531,7 +531,6 @@ static void unlock_ref(struct ref_lock *lock)
 static int lock_raw_ref(struct files_ref_store *refs,
 			const char *refname, int mustexist,
 			const struct string_list *extras,
-			const struct string_list *skip,
 			struct ref_lock **lock_p,
 			struct strbuf *referent,
 			unsigned int *type,
@@ -568,7 +567,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		 * reason to expect this error to be transitory.
 		 */
 		if (refs_verify_refname_available(&refs->base, refname,
-						  extras, skip, err)) {
+						  extras, NULL, err)) {
 			if (mustexist) {
 				/*
 				 * To the user the relevant error is
@@ -673,7 +672,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 							  REMOVE_DIR_EMPTY_ONLY)) {
 				if (refs_verify_refname_available(
 						    &refs->base, refname,
-						    extras, skip, err)) {
+						    extras, NULL, err)) {
 					/*
 					 * The error message set by
 					 * verify_refname_available() is OK.
@@ -710,7 +709,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		 */
 		if (refs_verify_refname_available(
 				    refs->packed_ref_store, refname,
-				    extras, skip, err))
+				    extras, NULL, err))
 			goto error_return;
 	}
 
@@ -2407,7 +2406,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	}
 
 	ret = lock_raw_ref(refs, update->refname, mustexist,
-			   affected_refnames, NULL,
+			   affected_refnames,
 			   &lock, &referent,
 			   &update->type, err);
 	if (ret) {
-- 
2.33.0.662.g438caf9576d

[PATCH v5 06/13] refs/debug: re-indent argument list for "prepare"

[Ævar Arnfjörð Bjarmason]

Re-indent this argument list that's been mis-indented since it was
added in 34c319970d1 (refs/debug: trace into reflog expiry too,
2021-04-23). This makes a subsequent change smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index 1a7a9e11cfa..3f1455db2df 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -365,8 +365,8 @@ struct debug_reflog_expiry_should_prune {
 };
 
 static void debug_reflog_expiry_prepare(const char *refname,
-				    const struct object_id *oid,
-				    void *cb_data)
+					const struct object_id *oid,
+					void *cb_data)
 {
 	struct debug_reflog_expiry_should_prune *prune = cb_data;
 	trace_printf_key(&trace_refs, "reflog_expire_prepare: %s\n", refname);
-- 
2.33.0.662.g438caf9576d

[PATCH v5 07/13] refs: make repo_dwim_log() accept a NULL oid

[Ævar Arnfjörð Bjarmason]

Change the repo_dwim_log() function initially added as dwim_log() in
eb3a48221fd (log --reflog: use dwim_log, 2007-02-09) to accept a NULL
oid parameter. The refs_resolve_ref_unsafe() function it invokes
already deals with it, but it didn't.

This allows for a bit more clarity in a reflog-walk.c codepath added
in f2eba66d4d1 (Enable HEAD@{...} and make it independent from the
current branch, 2007-02-03). We'll shortly use this in
builtin/reflog.c as well.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reflog-walk.c | 3 +--
 refs.c        | 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index e9cd3283694..8ac4b284b6b 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -158,10 +158,9 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 		}
 		reflogs = read_complete_reflog(branch);
 		if (!reflogs || reflogs->nr == 0) {
-			struct object_id oid;
 			char *b;
 			int ret = dwim_log(branch, strlen(branch),
-					   &oid, &b);
+					   NULL, &b);
 			if (ret > 1)
 				free(b);
 			else if (ret == 1) {
diff --git a/refs.c b/refs.c
index 8b9f7c3a80a..d9635436759 100644
--- a/refs.c
+++ b/refs.c
@@ -698,7 +698,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 		strbuf_addf(&path, *p, len, str);
 		ref = refs_resolve_ref_unsafe(refs, path.buf,
 					      RESOLVE_REF_READING,
-					      &hash, NULL);
+					      oid ? &hash : NULL, NULL);
 		if (!ref)
 			continue;
 		if (refs_reflog_exists(refs, path.buf))
@@ -710,7 +710,8 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
 			continue;
 		if (!logs_found++) {
 			*log = xstrdup(it);
-			oidcpy(oid, &hash);
+			if (oid)
+				oidcpy(oid, &hash);
 		}
 		if (!warn_ambiguous_refs)
 			break;
-- 
2.33.0.662.g438caf9576d

[PATCH v5 08/13] refs/files: add a comment about refs_reflog_exists() call

[Ævar Arnfjörð Bjarmason]

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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 997e021c1c4..fbcd0c790bc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3061,6 +3061,19 @@ 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 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 and the reflog doesn't exist after we've
+	 * acquired the lock that's OK. We've got nothing more to do;
+	 * We were asked to delete the reflog, but someone else
+	 * deleted it! The caller doesn't care that we deleted it,
+	 * just that it is deleted. So we can return successfully.
+	 */
 	if (!refs_reflog_exists(ref_store, refname)) {
 		unlock_ref(lock);
 		return 0;
-- 
2.33.0.662.g438caf9576d

[PATCH v5 09/13] reflog expire: don't lock reflogs using previously seen OID

[Ævar Arnfjörð Bjarmason]

During reflog expiry, the cmd_reflog_expire() function first iterates
over all reflogs in logs/*, and then one-by-one acquires the lock for
each one and expires it. This behavior has been with us since this
command was implemented in 4264dc15e1 ("git reflog expire",
2006-12-19).

Change this to stop calling lock_ref_oid_basic() with the OID we saw
when we looped over the logs, instead have it pass the OID it managed
to lock.

This mostly mitigates a race condition where e.g. "git gc" will fail
in a concurrently updated repository because the branch moved since
"git reflog expire --all" was started. I.e. with:

    error: cannot lock ref '<refname>': ref '<refname>' is at <OID-A> but expected <OID-B>

This behavior of passing in an "oid" was needed for an edge-case that
I've untangled in this and preceding commits though, namely that we
needed this OID because we'd:

 1. Lookup the reflog name/OID via dwim_log()
 2. With that OID, lock the reflog
 3. Later in builtin/reflog.c we use the OID we looked as input to
    lookup_commit_reference_gently(), assured that it's equal to the
    OID we got from dwim_log().

We can be sure that this change is safe to make because between
dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other
logic relevant to the OID or expiry run in the cmd_reflog_expire()
caller.

We can thus treat that code as a black box, before and after this
change it would get an OID that's been locked, the only difference is
that now we mostly won't be failing to get the lock due to the TOCTOU
race[0]. That failure was purely an implementation detail in how the
"current OID" was looked up, it was divorced from the locking
mechanism.

What do we mean with "mostly"? It mostly mitigates it because we'll
still run into cases where the ref is locked and being updated as we
want to expire it, and other git processes wanting to update the refs
will in turn race with us as we expire the reflog.

That remaining race can in turn be mitigated with the
core.filesRefLockTimeout setting, see 4ff0f01cb7 ("refs: retry
acquiring reference locks for 100ms", 2017-08-21). In practice if that
value is high enough we'll probably never have ref updates or reflog
expiry failing, since the clients involved will retry for far longer
than the time any of those operations could take.

See [1] for an initial report of how this impacted "git gc" and a
large discussion about this change in early 2019. In particular patch
looked good to Michael Haggerty, see his[2]. That message seems to not
have made it to the ML archive, its content is quoted in full in my
[3].

I'm leaving behind now-unused code the refs API etc. that takes the
now-NULL "unused_oid" argument, and other code that can be simplified now
that we never have on OID in that context, that'll be cleaned up in
subsequent commits, but for now let's narrowly focus on fixing the
"git gc" issue. As the modified assert() shows we always pass a NULL
oid to reflog_expire() now.

Unfortunately this sort of probabilistic contention is hard to turn
into a test. I've tested this by running the following three subshells
in concurrent terminals:

    (
        rm -rf /tmp/git &&
        git init /tmp/git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >/tmp/git/out &&
            git -C /tmp/git add out &&
            git -C /tmp/git commit -m"out"
        done
    )

    (
	rm -rf /tmp/git-clone &&
        git clone file:///tmp/git /tmp/git-clone &&
        while git -C /tmp/git-clone pull
        do
            date
        done
    )

    (
        while git -C /tmp/git-clone reflog expire --all
        do
            date
        done
    )

Before this change the "reflog expire" would fail really quickly with
the "but expected" error noted above.

After this change both the "pull" and "reflog expire" will run for a
while, but eventually fail because I get unlucky with
core.filesRefLockTimeout (the "reflog expire" is in a really tight
loop). As noted above that can in turn be mitigated with higher values
of core.filesRefLockTimeout than the 100ms default.

As noted in the commentary added in the preceding commit there's also
the case of branches being racily deleted, that can be tested by
adding this to the above:

    (
        while git -C /tmp/git-clone branch topic master &&
	      git -C /tmp/git-clone branch -D topic
        do
            date
        done
    )

With core.filesRefLockTimeout set to 10 seconds (it can probably be a
lot lower) I managed to run all four of these concurrently for about
an hour, and accumulated ~125k commits, auto-gc's and all, and didn't
have a single failure. The loops visibly stall while waiting for the
lock, but that's expected and desired behavior.

0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use
1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/
2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu
3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c     | 13 ++++++-------
 refs.h               |  2 +-
 refs/files-backend.c |  7 +++++--
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 09541d1c804..61795f22d53 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -629,8 +629,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free_worktrees(worktrees);
 		for (i = 0; i < collected.nr; i++) {
 			struct collected_reflog *e = collected.e[i];
+
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, &e->oid, flags,
+			status |= reflog_expire(e->reflog, NULL, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
@@ -642,13 +643,12 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (; i < argc; i++) {
 		char *ref;
-		struct object_id oid;
-		if (!dwim_log(argv[i], strlen(argv[i]), &oid, &ref)) {
+		if (!dwim_log(argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
-		status |= reflog_expire(ref, &oid, flags,
+		status |= reflog_expire(ref, NULL, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
@@ -700,7 +700,6 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 	for ( ; i < argc; i++) {
 		const char *spec = strstr(argv[i], "@{");
-		struct object_id oid;
 		char *ep, *ref;
 		int recno;
 
@@ -709,7 +708,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!dwim_log(argv[i], spec - argv[i], &oid, &ref)) {
+		if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) {
 			status |= error(_("no reflog for '%s'"), argv[i]);
 			continue;
 		}
@@ -724,7 +723,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.cmd.expire_total = 0;
 		}
 
-		status |= reflog_expire(ref, &oid, flags,
+		status |= reflog_expire(ref, NULL, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
diff --git a/refs.h b/refs.h
index 48970dfc7e0..ddbf15f1c21 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
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fbcd0c790bc..d81bda8bc25 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3027,7 +3027,7 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 }
 
 static int files_reflog_expire(struct ref_store *ref_store,
-			       const char *refname, const struct object_id *oid,
+			       const char *refname, const struct object_id *unused_oid,
 			       unsigned int flags,
 			       reflog_expiry_prepare_fn prepare_fn,
 			       reflog_expiry_should_prune_fn should_prune_fn,
@@ -3044,6 +3044,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	int status = 0;
 	int type;
 	struct strbuf err = STRBUF_INIT;
+	const struct object_id *oid;
 
 	memset(&cb, 0, sizeof(cb));
 	cb.flags = flags;
@@ -3055,12 +3056,13 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, oid, &type, &err);
+	lock = lock_ref_oid_basic(refs, refname, NULL, &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
 		return -1;
 	}
+	oid = &lock->old_oid;
 
 	/*
 	 * When refs are deleted, their reflog is deleted before the
@@ -3104,6 +3106,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		}
 	}
 
+	assert(!unused_oid);
 	(*prepare_fn)(refname, oid, cb.policy_cb);
 	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
 	(*cleanup_fn)(cb.policy_cb);
-- 
2.33.0.662.g438caf9576d

[PATCH v5 10/13] refs API: remove OID argument to reflog_expire()

[Ævar Arnfjörð Bjarmason]

Since the the preceding commit the "oid" parameter to reflog_expire()
is always NULL, but it was not cleaned up to reduce the size of the
diff. Let's do that subsequent API and documentation cleanup now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c      | 6 +++---
 refs.c                | 8 ++++----
 refs.h                | 7 +++----
 refs/debug.c          | 4 ++--
 refs/files-backend.c  | 3 +--
 refs/packed-backend.c | 2 +-
 refs/refs-internal.h  | 2 +-
 7 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 61795f22d53..bd4c669918d 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -631,7 +631,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			struct collected_reflog *e = collected.e[i];
 
 			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
-			status |= reflog_expire(e->reflog, NULL, flags,
+			status |= reflog_expire(e->reflog, flags,
 						reflog_expiry_prepare,
 						should_expire_reflog_ent,
 						reflog_expiry_cleanup,
@@ -648,7 +648,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
-		status |= reflog_expire(ref, NULL, flags,
+		status |= reflog_expire(ref, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
@@ -723,7 +723,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			cb.cmd.expire_total = 0;
 		}
 
-		status |= reflog_expire(ref, NULL, flags,
+		status |= reflog_expire(ref, flags,
 					reflog_expiry_prepare,
 					should_expire_reflog_ent,
 					reflog_expiry_cleanup,
diff --git a/refs.c b/refs.c
index d9635436759..05944d8e725 100644
--- a/refs.c
+++ b/refs.c
@@ -2371,19 +2371,19 @@ int delete_reflog(const char *refname)
 }
 
 int refs_reflog_expire(struct ref_store *refs,
-		       const char *refname, const struct object_id *oid,
+		       const char *refname,
 		       unsigned int flags,
 		       reflog_expiry_prepare_fn prepare_fn,
 		       reflog_expiry_should_prune_fn should_prune_fn,
 		       reflog_expiry_cleanup_fn cleanup_fn,
 		       void *policy_cb_data)
 {
-	return refs->be->reflog_expire(refs, refname, oid, flags,
+	return refs->be->reflog_expire(refs, refname, flags,
 				       prepare_fn, should_prune_fn,
 				       cleanup_fn, policy_cb_data);
 }
 
-int reflog_expire(const char *refname, const struct object_id *oid,
+int reflog_expire(const char *refname,
 		  unsigned int flags,
 		  reflog_expiry_prepare_fn prepare_fn,
 		  reflog_expiry_should_prune_fn should_prune_fn,
@@ -2391,7 +2391,7 @@ int reflog_expire(const char *refname, const struct object_id *oid,
 		  void *policy_cb_data)
 {
 	return refs_reflog_expire(get_main_ref_store(the_repository),
-				  refname, oid, flags,
+				  refname, flags,
 				  prepare_fn, should_prune_fn,
 				  cleanup_fn, policy_cb_data);
 }
diff --git a/refs.h b/refs.h
index ddbf15f1c21..fda8aef1547 100644
--- a/refs.h
+++ b/refs.h
@@ -816,20 +816,19 @@ typedef int reflog_expiry_should_prune_fn(struct object_id *ooid,
 typedef void reflog_expiry_cleanup_fn(void *cb_data);
 
 /*
- * Expire reflog entries for the specified reference. oid is the old
- * value of the reference. flags is a combination of the constants in
+ * Expire reflog entries for the specified reference.
+ * flags is a combination of the constants in
  * enum expire_reflog_flags. The three function pointers are described
  * above. On success, return zero.
  */
 int refs_reflog_expire(struct ref_store *refs,
 		       const char *refname,
-		       const struct object_id *oid,
 		       unsigned int flags,
 		       reflog_expiry_prepare_fn prepare_fn,
 		       reflog_expiry_should_prune_fn should_prune_fn,
 		       reflog_expiry_cleanup_fn cleanup_fn,
 		       void *policy_cb_data);
-int reflog_expire(const char *refname, const struct object_id *oid,
+int reflog_expire(const char *refname,
 		  unsigned int flags,
 		  reflog_expiry_prepare_fn prepare_fn,
 		  reflog_expiry_should_prune_fn should_prune_fn,
diff --git a/refs/debug.c b/refs/debug.c
index 3f1455db2df..bf4a82bccb6 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -392,7 +392,7 @@ static void debug_reflog_expiry_cleanup(void *cb_data)
 }
 
 static int debug_reflog_expire(struct ref_store *ref_store, const char *refname,
-			       const struct object_id *oid, unsigned int flags,
+			       unsigned int flags,
 			       reflog_expiry_prepare_fn prepare_fn,
 			       reflog_expiry_should_prune_fn should_prune_fn,
 			       reflog_expiry_cleanup_fn cleanup_fn,
@@ -405,7 +405,7 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname,
 		.should_prune = should_prune_fn,
 		.cb_data = policy_cb_data,
 	};
-	int res = drefs->refs->be->reflog_expire(drefs->refs, refname, oid,
+	int res = drefs->refs->be->reflog_expire(drefs->refs, refname,
 						 flags, &debug_reflog_expiry_prepare,
 						 &debug_reflog_expiry_should_prune_fn,
 						 &debug_reflog_expiry_cleanup,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index d81bda8bc25..a4adac46443 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3027,7 +3027,7 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 }
 
 static int files_reflog_expire(struct ref_store *ref_store,
-			       const char *refname, const struct object_id *unused_oid,
+			       const char *refname,
 			       unsigned int flags,
 			       reflog_expiry_prepare_fn prepare_fn,
 			       reflog_expiry_should_prune_fn should_prune_fn,
@@ -3106,7 +3106,6 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		}
 	}
 
-	assert(!unused_oid);
 	(*prepare_fn)(refname, oid, cb.policy_cb);
 	refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
 	(*cleanup_fn)(cb.policy_cb);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 24a360b719f..65ba4214b8d 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1635,7 +1635,7 @@ static int packed_delete_reflog(struct ref_store *ref_store,
 }
 
 static int packed_reflog_expire(struct ref_store *ref_store,
-				const char *refname, const struct object_id *oid,
+				const char *refname,
 				unsigned int flags,
 				reflog_expiry_prepare_fn prepare_fn,
 				reflog_expiry_should_prune_fn should_prune_fn,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 3155708345f..d496c5ed52d 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -593,7 +593,7 @@ typedef int create_reflog_fn(struct ref_store *ref_store, const char *refname,
 			     int force_create, struct strbuf *err);
 typedef int delete_reflog_fn(struct ref_store *ref_store, const char *refname);
 typedef int reflog_expire_fn(struct ref_store *ref_store,
-			     const char *refname, const struct object_id *oid,
+			     const char *refname,
 			     unsigned int flags,
 			     reflog_expiry_prepare_fn prepare_fn,
 			     reflog_expiry_should_prune_fn should_prune_fn,
-- 
2.33.0.662.g438caf9576d

[PATCH v5 11/13] refs/files: remove unused "oid" in lock_ref_oid_basic()

[Ævar Arnfjörð Bjarmason]

In the preceding commit the last caller that passed a non-NULL OID was
changed to pass NULL to lock_ref_oid_basic(). As noted in preceding
commits use of this API has been going away (we should use ref
transactions, or lock_raw_ref()), so we're unlikely to gain new
callers that want to pass the "oid".

So let's remove it, doing so means we can remove the "mustexist"
condition, and therefore anything except the "flags =
RESOLVE_REF_NO_RECURSE" case.

Furthermore, since the verify_lock() function we called did most of
its work when the "oid" was passed (as "old_oid") we can inline the
trivial part of it that remains in its only remaining caller. Without
a NULL "oid" passed it was equivalent to calling refs_read_ref_full()
followed by oidclr().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 70 +++++++++-----------------------------------
 1 file changed, 14 insertions(+), 56 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a4adac46443..4f2d907597a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -852,42 +852,6 @@ static struct ref_iterator *files_ref_iterator_begin(
 	return ref_iterator;
 }
 
-/*
- * Verify that the reference locked by lock has the value old_oid
- * (unless it is NULL).  Fail if the reference doesn't exist and
- * mustexist is set. Return 0 on success. On error, write an error
- * message to err, set errno, and return a negative value.
- */
-static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock,
-		       const struct object_id *old_oid, int mustexist,
-		       struct strbuf *err)
-{
-	assert(err);
-
-	if (refs_read_ref_full(ref_store, lock->ref_name,
-			       mustexist ? RESOLVE_REF_READING : 0,
-			       &lock->old_oid, NULL)) {
-		if (old_oid) {
-			int save_errno = errno;
-			strbuf_addf(err, "can't verify ref '%s'", lock->ref_name);
-			errno = save_errno;
-			return -1;
-		} else {
-			oidclr(&lock->old_oid);
-			return 0;
-		}
-	}
-	if (old_oid && !oideq(&lock->old_oid, old_oid)) {
-		strbuf_addf(err, "ref '%s' is at %s but expected %s",
-			    lock->ref_name,
-			    oid_to_hex(&lock->old_oid),
-			    oid_to_hex(old_oid));
-		errno = EBUSY;
-		return -1;
-	}
-	return 0;
-}
-
 static int remove_empty_directories(struct strbuf *path)
 {
 	/*
@@ -912,15 +876,12 @@ static int create_reflock(const char *path, void *cb)
  * On failure errno is set to something meaningful.
  */
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
-					   const char *refname,
-					   const struct object_id *old_oid,
-					   int *type, struct strbuf *err)
+					   const char *refname, int *type,
+					   struct strbuf *err)
 {
 	struct strbuf ref_file = STRBUF_INIT;
 	struct ref_lock *lock;
 	int last_errno = 0;
-	int mustexist = (old_oid && !is_null_oid(old_oid));
-	int resolve_flags = RESOLVE_REF_NO_RECURSE;
 	int resolved;
 
 	files_assert_main_repository(refs, "lock_ref_oid_basic");
@@ -928,12 +889,9 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 
 	CALLOC_ARRAY(lock, 1);
 
-	if (mustexist)
-		resolve_flags |= RESOLVE_REF_READING;
-
 	files_ref_path(refs, &ref_file, refname);
-	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 && errno == EISDIR) {
 		/*
@@ -951,8 +909,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					    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) {
@@ -987,10 +945,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) {
-		last_errno = errno;
-		goto error_return;
-	}
+	if (refs_read_ref_full(&refs->base, lock->ref_name,
+			       0,
+			       &lock->old_oid, NULL))
+		oidclr(&lock->old_oid);
 	goto out;
 
  error_return:
@@ -1409,7 +1367,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
 	logmoved = log;
 
-	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, &err);
+	lock = lock_ref_oid_basic(refs, newrefname, NULL, &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1431,7 +1389,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	goto out;
 
  rollback:
-	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, &err);
+	lock = lock_ref_oid_basic(refs, oldrefname, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
@@ -1838,7 +1796,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	struct ref_lock *lock;
 	int ret;
 
-	lock = lock_ref_oid_basic(refs, refname, NULL, NULL, &err);
+	lock = lock_ref_oid_basic(refs, refname, NULL, &err);
 	if (!lock) {
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -3056,7 +3014,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_oid_basic(refs, refname, NULL, &type, &err);
+	lock = lock_ref_oid_basic(refs, refname, &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
-- 
2.33.0.662.g438caf9576d

[PATCH v5 12/13] refs/files: remove unused "errno == EISDIR" code

[Ævar Arnfjörð Bjarmason]

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
in bc7127ef0f (ref locking: allow 'foo' when 'foo/bar' used to exist
but not anymore., 2006-09-30) seems like it should be dealing with.

Except it doesn't, and we never take this branch. The reason is that
when bc7127ef0f was written this looked like:

	ref = resolve_ref([...]);
	if (!ref && errno == EISDIR) {
	[...]

And in resolve_ref() we had this code:

	fd = open(path, O_RDONLY);
	if (fd < 0)
		return NULL;

I.e. we would attempt to read "foo" with open(), which would fail with
EISDIR and we'd return NULL. We'd then take this branch, call
remove_empty_directories() and continue.

Since a1c1d8170d (refs_resolve_ref_unsafe: handle d/f conflicts for
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()

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);
	if (fd < 0)
		/* get errno == EISDIR */
	/* later, in refs_resolve_ref_unsafe() */
	if ([...] && errno != EISDIR)
		return NULL;
	[...]
	/* returns the refs/heads/foo to the caller, even though it's a directory */
	return refname;

I.e. even though we got an "errno == EISDIR" we won't take this
branch, since in cases of EISDIR "resolved" is always
non-NULL. I.e. we pretend at this point as though everything's OK and
there is no "foo" directory.

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, 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 | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4f2d907597a..bed2ab25c3a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -882,7 +882,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	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);
@@ -890,30 +889,9 @@ 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
-		 * have foo/bar which now does not exist;
-		 * it is normal for the empty directory 'foo'
-		 * to remain.
-		 */
-		if (remove_empty_directories(&ref_file)) {
-			last_errno = errno;
-			if (!refs_verify_refname_available(
-					    &refs->base,
-					    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_REF_NO_RECURSE,
-						     &lock->old_oid, type);
-	}
-	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,
-- 
2.33.0.662.g438caf9576d

[PATCH v5 13/13] refs/files: remove unused "errno != ENOTDIR" condition

[Ævar Arnfjörð Bjarmason]

As a follow-up to the preceding commit where we removed the adjacent
"errno == EISDIR" condition in the same function, remove the
"last_errno != ENOTDIR" condition here.

It's not possible for us to hit this condition added in
5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F
conflicts, 2015-05-11). Since a1c1d8170db (refs_resolve_ref_unsafe:
handle d/f conflicts for writes, 2017-10-06) we've explicitly caught
these in refs_resolve_ref_unsafe() before returning NULL:

	if (errno != ENOENT &&
	    errno != EISDIR &&
	    errno != ENOTDIR)
		return NULL;

We'd then always return the refname from refs_resolve_ref_unsafe()
even if we were in a broken state as explained in the preceding
commit. The elided context here is a call to refs_resolve_ref_unsafe().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bed2ab25c3a..ab666af4b75 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -893,8 +893,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 				     RESOLVE_REF_NO_RECURSE,
 				     &lock->old_oid, type)) {
 		last_errno = errno;
-		if (last_errno != ENOTDIR ||
-		    !refs_verify_refname_available(&refs->base, refname,
+		if (!refs_verify_refname_available(&refs->base, refname,
 						   NULL, NULL, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
 				    refname, strerror(last_errno));
-- 
2.33.0.662.g438caf9576d