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

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

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