[v7,03/28] refs/debug: trace into reflog expiry too

Commit Message

Han-Wen Nienhuys April 19, 2021, 11:37 a.m. UTC

From: Han-Wen Nienhuys <[e-mail]>

Signed-off-by: Han-Wen Nienhuys <[e-mail]>
---
 refs/debug.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

Comments

Junio C Hamano April 20, 2021, 7:41 p.m. UTC | #1

"Han-Wen Nienhuys via GitGitGadget" <[e-mail]> writes:

> From: Han-Wen Nienhuys <[e-mail]>
>
> Signed-off-by: Han-Wen Nienhuys <[e-mail]>
> ---
>  refs/debug.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)

Nicely done.  

I as a reader of this patch do have to wonder, with the above very
limited log message material, how useful did "debug_reflog_expire()"
machinery used to be without any tracing.

It just reported the fact that expire method was called and what the
backend did as a whole, instead of reporting what the machinery
decided for each reflog entry to be pruned (or not pruned)?

Not a problem with this patch at all, and certainly it does not have
to be part of this series, but it feels very backwards, at least to
me, to have the method should_prune in ref backends.  As a function
to make a policy decision (e.g. "this has a timestamp older than X,
so needs to be pruned", "the author of this change I do not like, so
let's prune it ;-)"), it is more natural to have it as independent
as possible from the individual backends, no?

Han-Wen Nienhuys April 22, 2021, 5:27 p.m. UTC | #2

On Tue, Apr 20, 2021 at 9:42 PM Junio C Hamano <[e-mail]> wrote:
> Nicely done.
>
> I as a reader of this patch do have to wonder, with the above very
> limited log message material, how useful did "debug_reflog_expire()"
> machinery used to be without any tracing.

It wasn't; that's why I'm changing it :-)

I noticed a test failure with reftable, and adding this function
showed me I was expiring the entries in the reverse order, so that

  git reflog delete branch@{1}

would remove one but oldest entry in the reflog.

> Not a problem with this patch at all, and certainly it does not have
> to be part of this series, but it feels very backwards, at least to
> me, to have the method should_prune in ref backends.  As a function
> to make a policy decision (e.g. "this has a timestamp older than X,
> so needs to be pruned", "the author of this change I do not like, so
> let's prune it ;-)"), it is more natural to have it as independent
> as possible from the individual backends, no?

the should_prune function is passed in into the ref backend as an
argument of type reflog_expiry_should_prune_fn to the
refs_reflog_expire() function.

Patch

diff --git a/refs/debug.c b/refs/debug.c
index 922e64fa6ad9..3b25e3aeb1ba 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -353,6 +353,40 @@  static int debug_delete_reflog(struct ref_store *ref_store, const char *refname)
 	return res;
 }
 
+struct debug_reflog_expiry_should_prune {
+	reflog_expiry_prepare_fn *prepare;
+	reflog_expiry_should_prune_fn *should_prune;
+	reflog_expiry_cleanup_fn *cleanup;
+	void *cb_data;
+};
+
+static void debug_reflog_expiry_prepare(const char *refname,
+				    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);
+	prune->prepare(refname, oid, prune->cb_data);
+}
+
+static int debug_reflog_expiry_should_prune_fn(struct object_id *ooid,
+					       struct object_id *noid,
+					       const char *email,
+					       timestamp_t timestamp, int tz,
+					       const char *message, void *cb_data) {
+	struct debug_reflog_expiry_should_prune *prune = cb_data;
+
+	int result = prune->should_prune(ooid, noid, email, timestamp, tz, message, prune->cb_data);
+	trace_printf_key(&trace_refs, "reflog_expire_should_prune: %s %ld: %d\n", message, (long int) timestamp, result);
+	return result;
+}
+
+static void debug_reflog_expiry_cleanup(void *cb_data)
+{
+	struct debug_reflog_expiry_should_prune *prune = cb_data;
+	prune->cleanup(prune->cb_data);
+}
+
 static int debug_reflog_expire(struct ref_store *ref_store, const char *refname,
 			       const struct object_id *oid, unsigned int flags,
 			       reflog_expiry_prepare_fn prepare_fn,
@@ -361,10 +395,17 @@  static int debug_reflog_expire(struct ref_store *ref_store, const char *refname,
 			       void *policy_cb_data)
 {
 	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog_expiry_should_prune prune = {
+		.prepare = prepare_fn,
+		.cleanup = cleanup_fn,
+		.should_prune = should_prune_fn,
+		.cb_data = policy_cb_data,
+	};
 	int res = drefs->refs->be->reflog_expire(drefs->refs, refname, oid,
-						 flags, prepare_fn,
-						 should_prune_fn, cleanup_fn,
-						 policy_cb_data);
+						 flags, &debug_reflog_expiry_prepare,
+						 &debug_reflog_expiry_should_prune_fn,
+						 &debug_reflog_expiry_cleanup,
+						 &prune);
 	trace_printf_key(&trace_refs, "reflog_expire: %s: %d\n", refname, res);
 	return res;
 }