git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] RFC: refs: add GIT_DEBUG_REFS debugging mechanism
@ 2020-08-28 15:28 Han-Wen Nienhuys via GitGitGadget
  2020-09-02 17:49 ` Jonathan Tan
  2020-09-07 15:24 ` [PATCH v2] refs: add GIT_TRACE_REFS " Han-Wen Nienhuys via GitGitGadget
  0 siblings, 2 replies; 10+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2020-08-28 15:28 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

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

When set in the environment, GIT_DEBUG_REFS makes git print operations and
results as they flow through the ref storage backend. This helps debug
discrepancies between different ref backends.

This should be integrated with the trace2 sub-system, and I would appreciate
pointers on how to start.

Example:

    $ GIT_DEBUG_REFS=1 ./git branch
    ref_store for .git
    read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
    read_raw_ref: refs/heads/ref-debug: b0728cf8cb9dfd395bea41c646f48108c99230c0 (=> refs/heads/ref-debug) type 0: 0
    ref_iterator_begin: refs/heads/ (0x1)
    iterator_advance before: refs/heads/b4
    iterator_advance before: refs/heads/bla
    ...

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
    RFC: refs: add GIT_DEBUG_REFS debugging mechanism
    
    When set in the environment, GIT_DEBUG_REFS makes git print operations
    and results as they flow through the ref storage backend. This helps
    debug discrepancies between different ref backends.
    
    This should be integrated with the trace2 sub-system, and I would
    appreciate pointers on how to start.
    
    Example:
    
    $ GIT_DEBUG_REFS=1 ./git branch
    ref_store for .git
    read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
    read_raw_ref: refs/heads/ref-debug: b0728cf8cb9dfd395bea41c646f48108c99230c0 (=> refs/heads/ref-debug) type 0: 0
    ref_iterator_begin: refs/heads/ (0x1)
    iterator_advance before: refs/heads/b4
    iterator_advance before: refs/heads/bla
    ...
    
    Signed-off-by: Han-Wen Nienhuys hanwen@google.com [hanwen@google.com]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-713%2Fhanwen%2Fdebug-refs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-713/hanwen/debug-refs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/713

 Makefile             |   1 +
 refs.c               |   4 +
 refs/debug.c         | 387 +++++++++++++++++++++++++++++++++++++++++++
 refs/refs-internal.h |   6 +
 4 files changed, 398 insertions(+)
 create mode 100644 refs/debug.c

diff --git a/Makefile b/Makefile
index 65f8cfb236..51408fe453 100644
--- a/Makefile
+++ b/Makefile
@@ -957,6 +957,7 @@ LIB_OBJS += rebase.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
+LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
diff --git a/refs.c b/refs.c
index cf91711968..06726f7544 100644
--- a/refs.c
+++ b/refs.c
@@ -1748,6 +1748,10 @@ struct ref_store *get_main_ref_store(struct repository *r)
 		BUG("attempting to get main_ref_store outside of repository");
 
 	r->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
+	if (getenv("GIT_DEBUG_REFS")) {
+		// XXX - how to hook this up with the trace2 framework?
+		r->refs_private = debug_wrap(r->gitdir, r->refs_private);
+	}
 	return r->refs_private;
 }
 
diff --git a/refs/debug.c b/refs/debug.c
new file mode 100644
index 0000000000..1482c5bd5f
--- /dev/null
+++ b/refs/debug.c
@@ -0,0 +1,387 @@
+
+#include "refs-internal.h"
+
+struct debug_ref_store {
+	struct ref_store base;
+	struct ref_store *refs;
+};
+
+extern struct ref_storage_be refs_be_debug;
+struct ref_store *debug_wrap(const char *gitdir, struct ref_store *store);
+
+struct ref_store *debug_wrap(const char *gitdir, struct ref_store *store)
+{
+	struct debug_ref_store *res = malloc(sizeof(struct debug_ref_store));
+	struct ref_storage_be *be_copy = malloc(sizeof(*be_copy));
+	*be_copy = refs_be_debug;
+	be_copy->name = store->be->name;
+	fprintf(stderr, "ref_store for %s\n", gitdir);
+	res->refs = store;
+	base_ref_store_init((struct ref_store *)res, be_copy);
+	return (struct ref_store *)res;
+}
+
+static int debug_init_db(struct ref_store *refs, struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res = drefs->refs->be->init_db(drefs->refs, err);
+	fprintf(stderr, "init_db: %d\n", res);
+	return res;
+}
+
+static int debug_transaction_prepare(struct ref_store *refs,
+				     struct ref_transaction *transaction,
+				     struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_prepare(drefs->refs, transaction,
+						   err);
+	fprintf(stderr, "transaction_prepare: %d\n", res);
+	return res;
+}
+
+static void print_update(int i, const char *refname,
+			 const struct object_id *old_oid,
+			 const struct object_id *new_oid, unsigned int flags,
+			 unsigned int type, const char *msg)
+{
+	char o[200] = "null";
+	char n[200] = "null";
+	if (old_oid)
+		oid_to_hex_r(o, old_oid);
+	if (new_oid)
+		oid_to_hex_r(n, new_oid);
+
+	type &= 0xf; /* see refs.h REF_* */
+	flags &= REF_HAVE_NEW | REF_HAVE_OLD | REF_NO_DEREF |
+		REF_FORCE_CREATE_REFLOG; // XXX: LOG_ONLY
+	fprintf(stderr, "%d: %s %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
+		o, n, flags, type, msg);
+}
+
+static void print_transaction(struct ref_transaction *transaction)
+{
+	fprintf(stderr, "transaction {\n");
+	for (int i = 0; i < transaction->nr; i++) {
+		struct ref_update *u = transaction->updates[i];
+		print_update(i, u->refname, &u->old_oid, &u->new_oid, u->flags,
+			     u->type, u->msg);
+	}
+	fprintf(stderr, "}\n");
+}
+
+static int debug_transaction_finish(struct ref_store *refs,
+				    struct ref_transaction *transaction,
+				    struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_finish(drefs->refs, transaction,
+						  err);
+	print_transaction(transaction);
+	fprintf(stderr, "finish: %d\n", res);
+	return res;
+}
+
+static int debug_transaction_abort(struct ref_store *refs,
+				   struct ref_transaction *transaction,
+				   struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_abort(drefs->refs, transaction, err);
+	return res;
+}
+
+static int debug_initial_transaction_commit(struct ref_store *refs,
+					    struct ref_transaction *transaction,
+					    struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->initial_transaction_commit(drefs->refs,
+							  transaction, err);
+	return res;
+}
+
+static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->pack_refs(drefs->refs, flags);
+	fprintf(stderr, "pack_refs: %d\n", res);
+	return res;
+}
+
+static int debug_create_symref(struct ref_store *ref_store,
+			       const char *ref_name, const char *target,
+			       const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->create_symref(drefs->refs, ref_name, target,
+						 logmsg);
+	fprintf(stderr, "create_symref: %s -> %s \"%s\": %d\n", ref_name,
+		target, logmsg, res);
+	return res;
+}
+
+static int debug_delete_refs(struct ref_store *ref_store, const char *msg,
+			     struct string_list *refnames, unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res =
+		drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
+	int i = 0;
+	fprintf(stderr, "delete_refs {\n");
+	for (i = 0; i < refnames->nr; i++)
+		fprintf(stderr, "%s\n", refnames->items[i].string);
+	fprintf(stderr, "}: %d\n", res);
+	return res;
+}
+
+static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,
+			    const char *newref, const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->rename_ref(drefs->refs, oldref, newref,
+					      logmsg);
+	fprintf(stderr, "rename_ref: %s -> %s \"%s\": %d\n", oldref, newref,
+		logmsg, res);
+	return res;
+}
+
+static int debug_copy_ref(struct ref_store *ref_store, const char *oldref,
+			  const char *newref, const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res =
+		drefs->refs->be->copy_ref(drefs->refs, oldref, newref, logmsg);
+	fprintf(stderr, "copy_ref: %s -> %s \"%s\": %d\n", oldref, newref,
+		logmsg, res);
+	return res;
+}
+
+struct debug_ref_iterator {
+	struct ref_iterator base;
+	struct ref_iterator *iter;
+};
+
+static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->advance(diter->iter);
+	if (res)
+		fprintf(stderr, "iterator_advance: %d\n", res);
+	else
+		fprintf(stderr, "iterator_advance before: %s\n",
+			diter->iter->refname);
+
+	diter->base.ordered = diter->iter->ordered;
+	diter->base.refname = diter->iter->refname;
+	diter->base.oid = diter->iter->oid;
+	diter->base.flags = diter->iter->flags;
+	return res;
+}
+static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator,
+				   struct object_id *peeled)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->peel(diter->iter, peeled);
+	fprintf(stderr, "iterator_peel: %s: %d\n", diter->iter->refname, res);
+	return res;
+}
+
+static int debug_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->abort(diter->iter);
+	fprintf(stderr, "iterator_abort: %d\n", res);
+	return res;
+}
+
+static struct ref_iterator_vtable debug_ref_iterator_vtable = {
+	debug_ref_iterator_advance, debug_ref_iterator_peel,
+	debug_ref_iterator_abort
+};
+
+static struct ref_iterator *
+debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
+			 unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_iterator *res =
+		drefs->refs->be->iterator_begin(drefs->refs, prefix, flags);
+	struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));
+	base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);
+	diter->iter = res;
+	fprintf(stderr, "ref_iterator_begin: %s (0x%x)\n", prefix, flags);
+	return &diter->base;
+}
+
+static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
+			      struct object_id *oid, struct strbuf *referent,
+			      unsigned int *type)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = 0;
+
+	oidcpy(oid, &null_oid);
+	res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
+					    type);
+
+	if (res == 0) {
+		fprintf(stderr, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
+			refname, oid_to_hex(oid), referent->buf, *type, res);
+	} else {
+		fprintf(stderr, "read_raw_ref: %s err %d\n", refname, res);
+	}
+	return res;
+}
+
+static struct ref_iterator *
+debug_reflog_iterator_begin(struct ref_store *ref_store)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_iterator *res =
+		drefs->refs->be->reflog_iterator_begin(drefs->refs);
+	fprintf(stderr, "for_each_reflog_iterator_begin\n");
+	return res;
+}
+
+struct debug_reflog {
+	const char *refname;
+	each_reflog_ent_fn *fn;
+	void *cb_data;
+};
+
+static int debug_print_reflog_ent(struct object_id *old_oid,
+				  struct object_id *new_oid,
+				  const char *committer, timestamp_t timestamp,
+				  int tz, const char *msg, void *cb_data)
+{
+	struct debug_reflog *dbg = (struct debug_reflog *)cb_data;
+	int ret;
+	char o[100] = "null";
+	char n[100] = "null";
+	if (old_oid)
+		oid_to_hex_r(o, old_oid);
+	if (new_oid)
+		oid_to_hex_r(n, new_oid);
+
+	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
+		      dbg->cb_data);
+	fprintf(stderr, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
+		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	return ret;
+}
+
+static int debug_for_each_reflog_ent(struct ref_store *ref_store,
+				     const char *refname, each_reflog_ent_fn fn,
+				     void *cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog dbg = {
+		.refname = refname,
+		.fn = fn,
+		.cb_data = cb_data,
+	};
+
+	int res = drefs->refs->be->for_each_reflog_ent(
+		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
+	fprintf(stderr, "for_each_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_for_each_reflog_ent_reverse(struct ref_store *ref_store,
+					     const char *refname,
+					     each_reflog_ent_fn fn,
+					     void *cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog dbg = {
+		.refname = refname,
+		.fn = fn,
+		.cb_data = cb_data,
+	};
+	int res = drefs->refs->be->for_each_reflog_ent_reverse(
+		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
+	fprintf(stderr, "for_each_reflog_reverse: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->reflog_exists(drefs->refs, refname);
+	fprintf(stderr, "reflog_exists: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_create_reflog(struct ref_store *ref_store, const char *refname,
+			       int force_create, struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->create_reflog(drefs->refs, refname,
+						 force_create, err);
+	fprintf(stderr, "create_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_delete_reflog(struct ref_store *ref_store, const char *refname)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->delete_reflog(drefs->refs, refname);
+	fprintf(stderr, "delete_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+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,
+			       reflog_expiry_should_prune_fn should_prune_fn,
+			       reflog_expiry_cleanup_fn cleanup_fn,
+			       void *policy_cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->reflog_expire(drefs->refs, refname, oid,
+						 flags, prepare_fn,
+						 should_prune_fn, cleanup_fn,
+						 policy_cb_data);
+	fprintf(stderr, "reflog_expire: %s: %d\n", refname, res);
+	return res;
+}
+
+struct ref_storage_be refs_be_debug = {
+	NULL,
+	"debug",
+	NULL,
+	debug_init_db,
+	debug_transaction_prepare,
+	debug_transaction_finish,
+	debug_transaction_abort,
+	debug_initial_transaction_commit,
+
+	debug_pack_refs,
+	debug_create_symref,
+	debug_delete_refs,
+	debug_rename_ref,
+	debug_copy_ref,
+
+	debug_ref_iterator_begin,
+	debug_read_raw_ref,
+
+	debug_reflog_iterator_begin,
+	debug_for_each_reflog_ent,
+	debug_for_each_reflog_ent_reverse,
+	debug_reflog_exists,
+	debug_create_reflog,
+	debug_delete_reflog,
+	debug_reflog_expire,
+};
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 357359a0be..ec28ae5d86 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -681,4 +681,10 @@ struct ref_store {
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be);
 
+/*
+ * Print out ref operations as they occur. Useful for debugging alternate ref
+ * backends.
+ */
+struct ref_store *debug_wrap(const char *gitdir, struct ref_store *store);
+
 #endif /* REFS_REFS_INTERNAL_H */

base-commit: 675a4aaf3b226c0089108221b96559e0baae5de9
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] RFC: refs: add GIT_DEBUG_REFS debugging mechanism
  2020-08-28 15:28 [PATCH] RFC: refs: add GIT_DEBUG_REFS debugging mechanism Han-Wen Nienhuys via GitGitGadget
@ 2020-09-02 17:49 ` Jonathan Tan
  2020-09-03  5:44   ` Jonathan Nieder
  2020-09-07 15:24 ` [PATCH v2] refs: add GIT_TRACE_REFS " Han-Wen Nienhuys via GitGitGadget
  1 sibling, 1 reply; 10+ messages in thread
From: Jonathan Tan @ 2020-09-02 17:49 UTC (permalink / raw)
  To: gitgitgadget; +Cc: git, hanwenn, hanwen, git, stolee, Jonathan Tan

> This should be integrated with the trace2 sub-system, and I would appreciate
> pointers on how to start.

The trace2 subsystem seems to be designed to detect errors coarsely
(e.g. by looking at process invocations) and log timings. It currently
doesn't seem to be made for this kind of fine debugging information -
and perhaps this is by design, because such logging information would
not be useful to most users and would just clutter up the logs.

But I think there is a place for this in Git - in particular, we have
packet tracing (GIT_TRACE_PACKET), and this has been useful both in
automated tests (t/????-*.sh) and in manual tests. Ref backend tracing
seems to be similar. But this would be best if we had an additional
option that could control whether ref backend tracing was on or off,
independent from other things like the trace2 target.

Is the plan to migrate all usages of "trace" to "trace2" or for both to
exist simultaneously? If the latter, then ref backend tracing could just
use "trace", but if the former, we'd have to designing this additional
option.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] RFC: refs: add GIT_DEBUG_REFS debugging mechanism
  2020-09-02 17:49 ` Jonathan Tan
@ 2020-09-03  5:44   ` Jonathan Nieder
  2020-09-03  6:19     ` Jeff King
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Nieder @ 2020-09-03  5:44 UTC (permalink / raw)
  To: Jonathan Tan
  Cc: gitgitgadget, git, hanwenn, hanwen, git, stolee, Jeff King,
	Josh Steadmon

(+Peff and Josh, trace experts)
Jonathan Tan wrote:
> Han-Wen Nienhuys wrote:

>> This should be integrated with the trace2 sub-system, and I would appreciate
>> pointers on how to start.
>
> The trace2 subsystem seems to be designed to detect errors coarsely
> (e.g. by looking at process invocations) and log timings. It currently
> doesn't seem to be made for this kind of fine debugging information -
> and perhaps this is by design, because such logging information would
> not be useful to most users and would just clutter up the logs.
>
> But I think there is a place for this in Git - in particular, we have
> packet tracing (GIT_TRACE_PACKET), and this has been useful both in
> automated tests (t/????-*.sh) and in manual tests. Ref backend tracing
> seems to be similar. But this would be best if we had an additional
> option that could control whether ref backend tracing was on or off,
> independent from other things like the trace2 target.
>
> Is the plan to migrate all usages of "trace" to "trace2" or for both to
> exist simultaneously? If the latter, then ref backend tracing could just
> use "trace", but if the former, we'd have to designing this additional
> option.

Here's my not-completely-thought-through take:

trace.h defines a convenient and simple API for unstructured traces.
You can define a key

	static struct trace_key trace_refs = TRACE_KEY_INIT(REFS);

and then you have available

	trace_want(&trace_refs)
	trace_printf_key(&trace_refs, fmt, args...)
	trace_strbuf(&trace_refs, data)

and so on.

One use of trace_printf (i.e. tracing without a key) is to print lines
for important events like starting a subprocess.  For those, trace2 is a
natural replacement, with the advantage that the resulting event is
associated with any enclosing trace2 regions.

One design goal of trace2 was to be able to replace the original trace
subsystem completely.  Its output routines are flexible that it should
be able to produce GIT_TRACE style output.  It should be possible to
implement a trace_printf style API on top of trace2.

For ref tracing, do we care much about the human-friendliness of the
output?  Do we expect this to be useful for debugging once reftable is
more established?  Is it something we'd want to extract timing regions
from and use for performance improvements?

For ad hoc debugging, the structured tracing features of trace2 are
not all that important.  So if using the trace_key API is simpler,
then for that application I'd say go for it.  Some day we could
reimplement that same API on top of trace2 (or use a coccinelle
transform to a similar API) and your could would still work. :)

For producing logs that get aggregated, the structured output of
trace2 tends to be quite useful.  Event tracing doesn't have a notion
of setting which subsystems to trace on but it would be a natural
thing to add.  See TR2_SYSENV_CFG_PARAM for an example of configurable
"what to trace" information used by GIT_TRACE2_EVENT.  Happy to
provide more advice if this seems applicable for the problem at hand.

Summary:

- if this is only going to be used for tests and for ad hoc debugging,
  I'd suggest sticking to the simple trace_key based unstructured
  tracing API

- if we expect it to be something we want to aggregate over many runs
  and query, then structure becomes more important and trace2 starts
  to pull its weight

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] RFC: refs: add GIT_DEBUG_REFS debugging mechanism
  2020-09-03  5:44   ` Jonathan Nieder
@ 2020-09-03  6:19     ` Jeff King
       [not found]       ` <CAFQ2z_M_2mmL1LXcgR5cv3Ac1AJ363jaejZz3p6AcV6b3DftTQ@mail.gmail.com>
  0 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2020-09-03  6:19 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Jonathan Tan, gitgitgadget, git, hanwenn, hanwen, git, stolee,
	Josh Steadmon

On Wed, Sep 02, 2020 at 10:44:22PM -0700, Jonathan Nieder wrote:

> Summary:
> 
> - if this is only going to be used for tests and for ad hoc debugging,
>   I'd suggest sticking to the simple trace_key based unstructured
>   tracing API

Yeah, from my look at the debug patch, this seems sensible.

As clever as the "debug" ref-backend wrapper object is, I also wonder if
it might be simpler to just add trace calls to all of the outer
functions (e.g., ref_transaction_prepare(), etc). Unlike a "real" OO
system, we're not typically passing around the polymorphic ref_store
type, but always access it through those type-agnostic wrappers. But
maybe that would end up just as long. I dunno.

-Peff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Fwd: [PATCH] RFC: refs: add GIT_DEBUG_REFS debugging mechanism
       [not found]       ` <CAFQ2z_M_2mmL1LXcgR5cv3Ac1AJ363jaejZz3p6AcV6b3DftTQ@mail.gmail.com>
@ 2020-09-03 10:10         ` Han-Wen Nienhuys
  0 siblings, 0 replies; 10+ messages in thread
From: Han-Wen Nienhuys @ 2020-09-03 10:10 UTC (permalink / raw)
  To: git

On Thu, Sep 3, 2020 at 8:19 AM Jeff King <peff@peff.net> wrote:
>
> > - if this is only going to be used for tests and for ad hoc debugging,
> >   I'd suggest sticking to the simple trace_key based unstructured
> >   tracing API
>
> Yeah, from my look at the debug patch, this seems sensible.


thanks, I'll rework it to use the traditional trace API.

> As clever as the "debug" ref-backend wrapper object is, I also wonder if
> it might be simpler to just add trace calls to all of the outer
> functions (e.g., ref_transaction_prepare(), etc). Unlike a "real" OO
> system, we're not typically passing around the polymorphic ref_store
> type, but always access it through those type-agnostic wrappers. But
> maybe that would end up just as long. I dunno.

The ref backend supports callbacks, so it will always require some
amount of wrapping. The advantage of this approach is that the debug
support stays very neatly separate from the actual logic, and that the
compiler enforces that we add debug support if a new ref backend
method is added.

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



-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] refs: add GIT_TRACE_REFS debugging mechanism
  2020-08-28 15:28 [PATCH] RFC: refs: add GIT_DEBUG_REFS debugging mechanism Han-Wen Nienhuys via GitGitGadget
  2020-09-02 17:49 ` Jonathan Tan
@ 2020-09-07 15:24 ` Han-Wen Nienhuys via GitGitGadget
  2020-09-08 21:15   ` Junio C Hamano
  2020-09-09 10:15   ` [PATCH v3] " Han-Wen Nienhuys via GitGitGadget
  1 sibling, 2 replies; 10+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2020-09-07 15:24 UTC (permalink / raw)
  To: git
  Cc: Jonathan Tan, Jonathan Nieder, Jeff King, Han-Wen Nienhuys,
	Han-Wen Nienhuys, Han-Wen Nienhuys

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

When set in the environment, GIT_TRACE_REFS makes git print operations and
results as they flow through the ref storage backend. This helps debug
discrepancies between different ref backends.

Example:

    $ GIT_TRACE_REFS="1" ./git branch
    15:42:09.769631 refs/debug.c:26         ref_store for .git
    15:42:09.769681 refs/debug.c:249        read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
    15:42:09.769695 refs/debug.c:249        read_raw_ref: refs/heads/ref-debug: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a (=> refs/heads/ref-debug) type 0: 0
    15:42:09.770282 refs/debug.c:233        ref_iterator_begin: refs/heads/ (0x1)
    15:42:09.770290 refs/debug.c:189        iterator_advance: refs/heads/b4 (0)
    15:42:09.770295 refs/debug.c:189        iterator_advance: refs/heads/branch3 (0)

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
    refs: add GIT_TRACE_REFS debugging mechanism
    
    When set in the environment, GIT_TRACE_REFS makes git print operations
    and results as they flow through the ref storage backend. This helps
    debug discrepancies between different ref backends.
    
    Example:
    
        $ GIT_TRACE_REFS="1" ./git branch
        15:42:09.769631 refs/debug.c:26         ref_store for .git
        15:42:09.769681 refs/debug.c:249        read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
        15:42:09.769695 refs/debug.c:249        read_raw_ref: refs/heads/ref-debug: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a (=> refs/heads/ref-debug) type 0: 0
        15:42:09.770282 refs/debug.c:233        ref_iterator_begin: refs/heads/ (0x1)
        15:42:09.770290 refs/debug.c:189        iterator_advance: refs/heads/b4 (0)
        15:42:09.770295 refs/debug.c:189        iterator_advance: refs/heads/branch3 (0)
    
    Signed-off-by: Han-Wen Nienhuys hanwen@google.com [hanwen@google.com]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-713%2Fhanwen%2Fdebug-refs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-713/hanwen/debug-refs-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/713

Range-diff vs v1:

 1:  4d045e35be ! 1:  19330e894c RFC: refs: add GIT_DEBUG_REFS debugging mechanism
     @@ Metadata
      Author: Han-Wen Nienhuys <hanwen@google.com>
      
       ## Commit message ##
     -    RFC: refs: add GIT_DEBUG_REFS debugging mechanism
     +    refs: add GIT_TRACE_REFS debugging mechanism
      
     -    When set in the environment, GIT_DEBUG_REFS makes git print operations and
     +    When set in the environment, GIT_TRACE_REFS makes git print operations and
          results as they flow through the ref storage backend. This helps debug
          discrepancies between different ref backends.
      
     -    This should be integrated with the trace2 sub-system, and I would appreciate
     -    pointers on how to start.
     -
          Example:
      
     -        $ GIT_DEBUG_REFS=1 ./git branch
     -        ref_store for .git
     -        read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
     -        read_raw_ref: refs/heads/ref-debug: b0728cf8cb9dfd395bea41c646f48108c99230c0 (=> refs/heads/ref-debug) type 0: 0
     -        ref_iterator_begin: refs/heads/ (0x1)
     -        iterator_advance before: refs/heads/b4
     -        iterator_advance before: refs/heads/bla
     -        ...
     +        $ GIT_TRACE_REFS="1" ./git branch
     +        15:42:09.769631 refs/debug.c:26         ref_store for .git
     +        15:42:09.769681 refs/debug.c:249        read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
     +        15:42:09.769695 refs/debug.c:249        read_raw_ref: refs/heads/ref-debug: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a (=> refs/heads/ref-debug) type 0: 0
     +        15:42:09.770282 refs/debug.c:233        ref_iterator_begin: refs/heads/ (0x1)
     +        15:42:09.770290 refs/debug.c:189        iterator_advance: refs/heads/b4 (0)
     +        15:42:09.770295 refs/debug.c:189        iterator_advance: refs/heads/branch3 (0)
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
     + ## Documentation/git.txt ##
     +@@ Documentation/git.txt: of clones and fetches.
     + 	time of each Git command.
     + 	See `GIT_TRACE` for available trace output options.
     + 
     ++`GIT_TRACE_REFS`::
     ++	Enables trace messages for operations on the ref database.
     ++	See `GIT_TRACE` for available trace output options.
     ++
     + `GIT_TRACE_SETUP`::
     + 	Enables trace messages printing the .git, working tree and current
     + 	working directory after Git has completed its setup phase.
     +
       ## Makefile ##
      @@ Makefile: LIB_OBJS += rebase.o
       LIB_OBJS += ref-filter.o
     @@ refs.c: struct ref_store *get_main_ref_store(struct repository *r)
       		BUG("attempting to get main_ref_store outside of repository");
       
       	r->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
     -+	if (getenv("GIT_DEBUG_REFS")) {
     -+		// XXX - how to hook this up with the trace2 framework?
     -+		r->refs_private = debug_wrap(r->gitdir, r->refs_private);
     -+	}
     ++	r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private);
       	return r->refs_private;
       }
       
     @@ refs/debug.c (new)
      @@
      +
      +#include "refs-internal.h"
     ++#include "trace.h"
     ++
     ++static struct trace_key trace_refs = TRACE_KEY_INIT(REFS);
      +
      +struct debug_ref_store {
      +	struct ref_store base;
     @@ refs/debug.c (new)
      +};
      +
      +extern struct ref_storage_be refs_be_debug;
     -+struct ref_store *debug_wrap(const char *gitdir, struct ref_store *store);
      +
     -+struct ref_store *debug_wrap(const char *gitdir, struct ref_store *store)
     ++struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store)
      +{
     -+	struct debug_ref_store *res = malloc(sizeof(struct debug_ref_store));
     -+	struct ref_storage_be *be_copy = malloc(sizeof(*be_copy));
     ++	struct debug_ref_store *res;
     ++	struct ref_storage_be *be_copy;
     ++
     ++	if (!trace_want(&trace_refs)) {
     ++		return store;
     ++	}
     ++	res = malloc(sizeof(struct debug_ref_store));
     ++	be_copy = malloc(sizeof(*be_copy));
      +	*be_copy = refs_be_debug;
      +	be_copy->name = store->be->name;
     -+	fprintf(stderr, "ref_store for %s\n", gitdir);
     ++	trace_printf_key(&trace_refs, "ref_store for %s\n", gitdir);
      +	res->refs = store;
      +	base_ref_store_init((struct ref_store *)res, be_copy);
      +	return (struct ref_store *)res;
     @@ refs/debug.c (new)
      +{
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
      +	int res = drefs->refs->be->init_db(drefs->refs, err);
     -+	fprintf(stderr, "init_db: %d\n", res);
     ++	trace_printf_key(&trace_refs, "init_db: %d\n", res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +	transaction->ref_store = drefs->refs;
      +	res = drefs->refs->be->transaction_prepare(drefs->refs, transaction,
      +						   err);
     -+	fprintf(stderr, "transaction_prepare: %d\n", res);
     ++	trace_printf_key(&trace_refs, "transaction_prepare: %d\n", res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +
      +	type &= 0xf; /* see refs.h REF_* */
      +	flags &= REF_HAVE_NEW | REF_HAVE_OLD | REF_NO_DEREF |
     -+		REF_FORCE_CREATE_REFLOG; // XXX: LOG_ONLY
     -+	fprintf(stderr, "%d: %s %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
     ++		REF_FORCE_CREATE_REFLOG;
     ++	trace_printf_key(&trace_refs, "%d: %s %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
      +		o, n, flags, type, msg);
      +}
      +
      +static void print_transaction(struct ref_transaction *transaction)
      +{
     -+	fprintf(stderr, "transaction {\n");
     ++	trace_printf_key(&trace_refs, "transaction {\n");
      +	for (int i = 0; i < transaction->nr; i++) {
      +		struct ref_update *u = transaction->updates[i];
      +		print_update(i, u->refname, &u->old_oid, &u->new_oid, u->flags,
      +			     u->type, u->msg);
      +	}
     -+	fprintf(stderr, "}\n");
     ++	trace_printf_key(&trace_refs, "}\n");
      +}
      +
      +static int debug_transaction_finish(struct ref_store *refs,
     @@ refs/debug.c (new)
      +	res = drefs->refs->be->transaction_finish(drefs->refs, transaction,
      +						  err);
      +	print_transaction(transaction);
     -+	fprintf(stderr, "finish: %d\n", res);
     ++	trace_printf_key(&trace_refs, "finish: %d\n", res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +{
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	int res = drefs->refs->be->pack_refs(drefs->refs, flags);
     -+	fprintf(stderr, "pack_refs: %d\n", res);
     ++	trace_printf_key(&trace_refs, "pack_refs: %d\n", res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	int res = drefs->refs->be->create_symref(drefs->refs, ref_name, target,
      +						 logmsg);
     -+	fprintf(stderr, "create_symref: %s -> %s \"%s\": %d\n", ref_name,
     ++	trace_printf_key(&trace_refs, "create_symref: %s -> %s \"%s\": %d\n", ref_name,
      +		target, logmsg, res);
      +	return res;
      +}
     @@ refs/debug.c (new)
      +	int res =
      +		drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
      +	int i = 0;
     -+	fprintf(stderr, "delete_refs {\n");
     ++	trace_printf_key(&trace_refs, "delete_refs {\n");
      +	for (i = 0; i < refnames->nr; i++)
     -+		fprintf(stderr, "%s\n", refnames->items[i].string);
     -+	fprintf(stderr, "}: %d\n", res);
     ++		trace_printf_key(&trace_refs, "%s\n", refnames->items[i].string);
     ++	trace_printf_key(&trace_refs, "}: %d\n", res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	int res = drefs->refs->be->rename_ref(drefs->refs, oldref, newref,
      +					      logmsg);
     -+	fprintf(stderr, "rename_ref: %s -> %s \"%s\": %d\n", oldref, newref,
     ++	trace_printf_key(&trace_refs, "rename_ref: %s -> %s \"%s\": %d\n", oldref, newref,
      +		logmsg, res);
      +	return res;
      +}
     @@ refs/debug.c (new)
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	int res =
      +		drefs->refs->be->copy_ref(drefs->refs, oldref, newref, logmsg);
     -+	fprintf(stderr, "copy_ref: %s -> %s \"%s\": %d\n", oldref, newref,
     ++	trace_printf_key(&trace_refs, "copy_ref: %s -> %s \"%s\": %d\n", oldref, newref,
      +		logmsg, res);
      +	return res;
      +}
     @@ refs/debug.c (new)
      +		(struct debug_ref_iterator *)ref_iterator;
      +	int res = diter->iter->vtable->advance(diter->iter);
      +	if (res)
     -+		fprintf(stderr, "iterator_advance: %d\n", res);
     ++		trace_printf_key(&trace_refs, "iterator_advance: (%d)\n", res);
      +	else
     -+		fprintf(stderr, "iterator_advance before: %s\n",
     ++		trace_printf_key(&trace_refs, "iterator_advance: %s (0)\n",
      +			diter->iter->refname);
      +
      +	diter->base.ordered = diter->iter->ordered;
     @@ refs/debug.c (new)
      +	diter->base.flags = diter->iter->flags;
      +	return res;
      +}
     ++
      +static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator,
      +				   struct object_id *peeled)
      +{
      +	struct debug_ref_iterator *diter =
      +		(struct debug_ref_iterator *)ref_iterator;
      +	int res = diter->iter->vtable->peel(diter->iter, peeled);
     -+	fprintf(stderr, "iterator_peel: %s: %d\n", diter->iter->refname, res);
     ++	trace_printf_key(&trace_refs, "iterator_peel: %s: %d\n", diter->iter->refname, res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +	struct debug_ref_iterator *diter =
      +		(struct debug_ref_iterator *)ref_iterator;
      +	int res = diter->iter->vtable->abort(diter->iter);
     -+	fprintf(stderr, "iterator_abort: %d\n", res);
     ++	trace_printf_key(&trace_refs, "iterator_abort: %d\n", res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +	struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));
      +	base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);
      +	diter->iter = res;
     -+	fprintf(stderr, "ref_iterator_begin: %s (0x%x)\n", prefix, flags);
     ++	trace_printf_key(&trace_refs, "ref_iterator_begin: %s (0x%x)\n", prefix, flags);
      +	return &diter->base;
      +}
      +
     @@ refs/debug.c (new)
      +					    type);
      +
      +	if (res == 0) {
     -+		fprintf(stderr, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
     ++		trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
      +			refname, oid_to_hex(oid), referent->buf, *type, res);
      +	} else {
     -+		fprintf(stderr, "read_raw_ref: %s err %d\n", refname, res);
     ++		trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res);
      +	}
      +	return res;
      +}
     @@ refs/debug.c (new)
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	struct ref_iterator *res =
      +		drefs->refs->be->reflog_iterator_begin(drefs->refs);
     -+	fprintf(stderr, "for_each_reflog_iterator_begin\n");
     ++	trace_printf_key(&trace_refs, "for_each_reflog_iterator_begin\n");
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +
      +	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
      +		      dbg->cb_data);
     -+	fprintf(stderr, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
     ++	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
      +		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
      +	return ret;
      +}
     @@ refs/debug.c (new)
      +
      +	int res = drefs->refs->be->for_each_reflog_ent(
      +		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
     -+	fprintf(stderr, "for_each_reflog: %s: %d\n", refname, res);
     ++	trace_printf_key(&trace_refs, "for_each_reflog: %s: %d\n", refname, res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +	};
      +	int res = drefs->refs->be->for_each_reflog_ent_reverse(
      +		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
     -+	fprintf(stderr, "for_each_reflog_reverse: %s: %d\n", refname, res);
     ++	trace_printf_key(&trace_refs, "for_each_reflog_reverse: %s: %d\n", refname, res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +{
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	int res = drefs->refs->be->reflog_exists(drefs->refs, refname);
     -+	fprintf(stderr, "reflog_exists: %s: %d\n", refname, res);
     ++	trace_printf_key(&trace_refs, "reflog_exists: %s: %d\n", refname, res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	int res = drefs->refs->be->create_reflog(drefs->refs, refname,
      +						 force_create, err);
     -+	fprintf(stderr, "create_reflog: %s: %d\n", refname, res);
     ++	trace_printf_key(&trace_refs, "create_reflog: %s: %d\n", refname, res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +{
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	int res = drefs->refs->be->delete_reflog(drefs->refs, refname);
     -+	fprintf(stderr, "delete_reflog: %s: %d\n", refname, res);
     ++	trace_printf_key(&trace_refs, "delete_reflog: %s: %d\n", refname, res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +						 flags, prepare_fn,
      +						 should_prune_fn, cleanup_fn,
      +						 policy_cb_data);
     -+	fprintf(stderr, "reflog_expire: %s: %d\n", refname, res);
     ++	trace_printf_key(&trace_refs, "reflog_expire: %s: %d\n", refname, res);
      +	return res;
      +}
      +
     @@ refs/debug.c (new)
      +};
      
       ## refs/refs-internal.h ##
     -@@ refs/refs-internal.h: struct ref_store {
     +@@ refs/refs-internal.h: int parse_loose_ref_contents(const char *buf, struct object_id *oid,
       void base_ref_store_init(struct ref_store *refs,
       			 const struct ref_storage_be *be);
       
      +/*
     -+ * Print out ref operations as they occur. Useful for debugging alternate ref
     -+ * backends.
     ++ * Support GIT_TRACE_REFS by optionally wrapping the given ref_store instance.
      + */
     -+struct ref_store *debug_wrap(const char *gitdir, struct ref_store *store);
     ++struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store);
      +
       #endif /* REFS_REFS_INTERNAL_H */


 Documentation/git.txt |   4 +
 Makefile              |   1 +
 refs.c                |   1 +
 refs/debug.c          | 396 ++++++++++++++++++++++++++++++++++++++++++
 refs/refs-internal.h  |   5 +
 5 files changed, 407 insertions(+)
 create mode 100644 refs/debug.c

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2f72b10224..c463b937a8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -722,6 +722,10 @@ of clones and fetches.
 	time of each Git command.
 	See `GIT_TRACE` for available trace output options.
 
+`GIT_TRACE_REFS`::
+	Enables trace messages for operations on the ref database.
+	See `GIT_TRACE` for available trace output options.
+
 `GIT_TRACE_SETUP`::
 	Enables trace messages printing the .git, working tree and current
 	working directory after Git has completed its setup phase.
diff --git a/Makefile b/Makefile
index 86e5411f39..583b8c526e 100644
--- a/Makefile
+++ b/Makefile
@@ -950,6 +950,7 @@ LIB_OBJS += rebase.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
+LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
diff --git a/refs.c b/refs.c
index 156fdcd459..5febfe2281 100644
--- a/refs.c
+++ b/refs.c
@@ -1774,6 +1774,7 @@ struct ref_store *get_main_ref_store(struct repository *r)
 		BUG("attempting to get main_ref_store outside of repository");
 
 	r->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
+	r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private);
 	return r->refs_private;
 }
 
diff --git a/refs/debug.c b/refs/debug.c
new file mode 100644
index 0000000000..68ae531b45
--- /dev/null
+++ b/refs/debug.c
@@ -0,0 +1,396 @@
+
+#include "refs-internal.h"
+#include "trace.h"
+
+static struct trace_key trace_refs = TRACE_KEY_INIT(REFS);
+
+struct debug_ref_store {
+	struct ref_store base;
+	struct ref_store *refs;
+};
+
+extern struct ref_storage_be refs_be_debug;
+
+struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store)
+{
+	struct debug_ref_store *res;
+	struct ref_storage_be *be_copy;
+
+	if (!trace_want(&trace_refs)) {
+		return store;
+	}
+	res = malloc(sizeof(struct debug_ref_store));
+	be_copy = malloc(sizeof(*be_copy));
+	*be_copy = refs_be_debug;
+	be_copy->name = store->be->name;
+	trace_printf_key(&trace_refs, "ref_store for %s\n", gitdir);
+	res->refs = store;
+	base_ref_store_init((struct ref_store *)res, be_copy);
+	return (struct ref_store *)res;
+}
+
+static int debug_init_db(struct ref_store *refs, struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res = drefs->refs->be->init_db(drefs->refs, err);
+	trace_printf_key(&trace_refs, "init_db: %d\n", res);
+	return res;
+}
+
+static int debug_transaction_prepare(struct ref_store *refs,
+				     struct ref_transaction *transaction,
+				     struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_prepare(drefs->refs, transaction,
+						   err);
+	trace_printf_key(&trace_refs, "transaction_prepare: %d\n", res);
+	return res;
+}
+
+static void print_update(int i, const char *refname,
+			 const struct object_id *old_oid,
+			 const struct object_id *new_oid, unsigned int flags,
+			 unsigned int type, const char *msg)
+{
+	char o[200] = "null";
+	char n[200] = "null";
+	if (old_oid)
+		oid_to_hex_r(o, old_oid);
+	if (new_oid)
+		oid_to_hex_r(n, new_oid);
+
+	type &= 0xf; /* see refs.h REF_* */
+	flags &= REF_HAVE_NEW | REF_HAVE_OLD | REF_NO_DEREF |
+		REF_FORCE_CREATE_REFLOG;
+	trace_printf_key(&trace_refs, "%d: %s %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
+		o, n, flags, type, msg);
+}
+
+static void print_transaction(struct ref_transaction *transaction)
+{
+	trace_printf_key(&trace_refs, "transaction {\n");
+	for (int i = 0; i < transaction->nr; i++) {
+		struct ref_update *u = transaction->updates[i];
+		print_update(i, u->refname, &u->old_oid, &u->new_oid, u->flags,
+			     u->type, u->msg);
+	}
+	trace_printf_key(&trace_refs, "}\n");
+}
+
+static int debug_transaction_finish(struct ref_store *refs,
+				    struct ref_transaction *transaction,
+				    struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_finish(drefs->refs, transaction,
+						  err);
+	print_transaction(transaction);
+	trace_printf_key(&trace_refs, "finish: %d\n", res);
+	return res;
+}
+
+static int debug_transaction_abort(struct ref_store *refs,
+				   struct ref_transaction *transaction,
+				   struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_abort(drefs->refs, transaction, err);
+	return res;
+}
+
+static int debug_initial_transaction_commit(struct ref_store *refs,
+					    struct ref_transaction *transaction,
+					    struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->initial_transaction_commit(drefs->refs,
+							  transaction, err);
+	return res;
+}
+
+static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->pack_refs(drefs->refs, flags);
+	trace_printf_key(&trace_refs, "pack_refs: %d\n", res);
+	return res;
+}
+
+static int debug_create_symref(struct ref_store *ref_store,
+			       const char *ref_name, const char *target,
+			       const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->create_symref(drefs->refs, ref_name, target,
+						 logmsg);
+	trace_printf_key(&trace_refs, "create_symref: %s -> %s \"%s\": %d\n", ref_name,
+		target, logmsg, res);
+	return res;
+}
+
+static int debug_delete_refs(struct ref_store *ref_store, const char *msg,
+			     struct string_list *refnames, unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res =
+		drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
+	int i = 0;
+	trace_printf_key(&trace_refs, "delete_refs {\n");
+	for (i = 0; i < refnames->nr; i++)
+		trace_printf_key(&trace_refs, "%s\n", refnames->items[i].string);
+	trace_printf_key(&trace_refs, "}: %d\n", res);
+	return res;
+}
+
+static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,
+			    const char *newref, const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->rename_ref(drefs->refs, oldref, newref,
+					      logmsg);
+	trace_printf_key(&trace_refs, "rename_ref: %s -> %s \"%s\": %d\n", oldref, newref,
+		logmsg, res);
+	return res;
+}
+
+static int debug_copy_ref(struct ref_store *ref_store, const char *oldref,
+			  const char *newref, const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res =
+		drefs->refs->be->copy_ref(drefs->refs, oldref, newref, logmsg);
+	trace_printf_key(&trace_refs, "copy_ref: %s -> %s \"%s\": %d\n", oldref, newref,
+		logmsg, res);
+	return res;
+}
+
+struct debug_ref_iterator {
+	struct ref_iterator base;
+	struct ref_iterator *iter;
+};
+
+static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->advance(diter->iter);
+	if (res)
+		trace_printf_key(&trace_refs, "iterator_advance: (%d)\n", res);
+	else
+		trace_printf_key(&trace_refs, "iterator_advance: %s (0)\n",
+			diter->iter->refname);
+
+	diter->base.ordered = diter->iter->ordered;
+	diter->base.refname = diter->iter->refname;
+	diter->base.oid = diter->iter->oid;
+	diter->base.flags = diter->iter->flags;
+	return res;
+}
+
+static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator,
+				   struct object_id *peeled)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->peel(diter->iter, peeled);
+	trace_printf_key(&trace_refs, "iterator_peel: %s: %d\n", diter->iter->refname, res);
+	return res;
+}
+
+static int debug_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->abort(diter->iter);
+	trace_printf_key(&trace_refs, "iterator_abort: %d\n", res);
+	return res;
+}
+
+static struct ref_iterator_vtable debug_ref_iterator_vtable = {
+	debug_ref_iterator_advance, debug_ref_iterator_peel,
+	debug_ref_iterator_abort
+};
+
+static struct ref_iterator *
+debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
+			 unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_iterator *res =
+		drefs->refs->be->iterator_begin(drefs->refs, prefix, flags);
+	struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));
+	base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);
+	diter->iter = res;
+	trace_printf_key(&trace_refs, "ref_iterator_begin: %s (0x%x)\n", prefix, flags);
+	return &diter->base;
+}
+
+static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
+			      struct object_id *oid, struct strbuf *referent,
+			      unsigned int *type)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = 0;
+
+	oidcpy(oid, &null_oid);
+	res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
+					    type);
+
+	if (res == 0) {
+		trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
+			refname, oid_to_hex(oid), referent->buf, *type, res);
+	} else {
+		trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res);
+	}
+	return res;
+}
+
+static struct ref_iterator *
+debug_reflog_iterator_begin(struct ref_store *ref_store)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_iterator *res =
+		drefs->refs->be->reflog_iterator_begin(drefs->refs);
+	trace_printf_key(&trace_refs, "for_each_reflog_iterator_begin\n");
+	return res;
+}
+
+struct debug_reflog {
+	const char *refname;
+	each_reflog_ent_fn *fn;
+	void *cb_data;
+};
+
+static int debug_print_reflog_ent(struct object_id *old_oid,
+				  struct object_id *new_oid,
+				  const char *committer, timestamp_t timestamp,
+				  int tz, const char *msg, void *cb_data)
+{
+	struct debug_reflog *dbg = (struct debug_reflog *)cb_data;
+	int ret;
+	char o[100] = "null";
+	char n[100] = "null";
+	if (old_oid)
+		oid_to_hex_r(o, old_oid);
+	if (new_oid)
+		oid_to_hex_r(n, new_oid);
+
+	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
+		      dbg->cb_data);
+	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
+		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	return ret;
+}
+
+static int debug_for_each_reflog_ent(struct ref_store *ref_store,
+				     const char *refname, each_reflog_ent_fn fn,
+				     void *cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog dbg = {
+		.refname = refname,
+		.fn = fn,
+		.cb_data = cb_data,
+	};
+
+	int res = drefs->refs->be->for_each_reflog_ent(
+		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
+	trace_printf_key(&trace_refs, "for_each_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_for_each_reflog_ent_reverse(struct ref_store *ref_store,
+					     const char *refname,
+					     each_reflog_ent_fn fn,
+					     void *cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog dbg = {
+		.refname = refname,
+		.fn = fn,
+		.cb_data = cb_data,
+	};
+	int res = drefs->refs->be->for_each_reflog_ent_reverse(
+		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
+	trace_printf_key(&trace_refs, "for_each_reflog_reverse: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->reflog_exists(drefs->refs, refname);
+	trace_printf_key(&trace_refs, "reflog_exists: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_create_reflog(struct ref_store *ref_store, const char *refname,
+			       int force_create, struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->create_reflog(drefs->refs, refname,
+						 force_create, err);
+	trace_printf_key(&trace_refs, "create_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_delete_reflog(struct ref_store *ref_store, const char *refname)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->delete_reflog(drefs->refs, refname);
+	trace_printf_key(&trace_refs, "delete_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+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,
+			       reflog_expiry_should_prune_fn should_prune_fn,
+			       reflog_expiry_cleanup_fn cleanup_fn,
+			       void *policy_cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->reflog_expire(drefs->refs, refname, oid,
+						 flags, prepare_fn,
+						 should_prune_fn, cleanup_fn,
+						 policy_cb_data);
+	trace_printf_key(&trace_refs, "reflog_expire: %s: %d\n", refname, res);
+	return res;
+}
+
+struct ref_storage_be refs_be_debug = {
+	NULL,
+	"debug",
+	NULL,
+	debug_init_db,
+	debug_transaction_prepare,
+	debug_transaction_finish,
+	debug_transaction_abort,
+	debug_initial_transaction_commit,
+
+	debug_pack_refs,
+	debug_create_symref,
+	debug_delete_refs,
+	debug_rename_ref,
+	debug_copy_ref,
+
+	debug_ref_iterator_begin,
+	debug_read_raw_ref,
+
+	debug_reflog_iterator_begin,
+	debug_for_each_reflog_ent,
+	debug_for_each_reflog_ent_reverse,
+	debug_reflog_exists,
+	debug_create_reflog,
+	debug_delete_reflog,
+	debug_reflog_expire,
+};
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 527b0a6e2e..fecde40baf 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -690,4 +690,9 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be);
 
+/*
+ * Support GIT_TRACE_REFS by optionally wrapping the given ref_store instance.
+ */
+struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store);
+
 #endif /* REFS_REFS_INTERNAL_H */

base-commit: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] refs: add GIT_TRACE_REFS debugging mechanism
  2020-09-07 15:24 ` [PATCH v2] refs: add GIT_TRACE_REFS " Han-Wen Nienhuys via GitGitGadget
@ 2020-09-08 21:15   ` Junio C Hamano
  2020-09-09  9:43     ` Han-Wen Nienhuys
  2020-09-09 10:15   ` [PATCH v3] " Han-Wen Nienhuys via GitGitGadget
  1 sibling, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2020-09-08 21:15 UTC (permalink / raw)
  To: Han-Wen Nienhuys via GitGitGadget
  Cc: git, Jonathan Tan, Jonathan Nieder, Jeff King, Han-Wen Nienhuys,
	Han-Wen Nienhuys

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/refs.c b/refs.c
> index 156fdcd459..5febfe2281 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1774,6 +1774,7 @@ struct ref_store *get_main_ref_store(struct repository *r)
>  		BUG("attempting to get main_ref_store outside of repository");
>  
>  	r->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
> +	r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private);

So the idea is to capture calls to backend, emit tracing events and
defer to the real backend?

Cute.  I like it.

> diff --git a/refs/debug.c b/refs/debug.c
> new file mode 100644
> index 0000000000..68ae531b45
> --- /dev/null
> +++ b/refs/debug.c
> @@ -0,0 +1,396 @@
> +
> +#include "refs-internal.h"
> +#include "trace.h"
> +
> +static struct trace_key trace_refs = TRACE_KEY_INIT(REFS);
> +
> +struct debug_ref_store {
> +	struct ref_store base;
> +	struct ref_store *refs;
> +};
> +
> +extern struct ref_storage_be refs_be_debug;
> +
> +struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store)
> +{
> +	struct debug_ref_store *res;
> +	struct ref_storage_be *be_copy;
> +
> +	if (!trace_want(&trace_refs)) {
> +		return store;
> +	}
> +	res = malloc(sizeof(struct debug_ref_store));
> +	be_copy = malloc(sizeof(*be_copy));

Not xmalloc() and friends?

> +	*be_copy = refs_be_debug;
> +	be_copy->name = store->be->name;

I guess we never destroy the ref-store instances so it is OK to
assume that it would not cause problems later by sharing pieces of
memory with underlying "real" thing like this.

> +	trace_printf_key(&trace_refs, "ref_store for %s\n", gitdir);
> +	res->refs = store;
> +	base_ref_store_init((struct ref_store *)res, be_copy);
> +	return (struct ref_store *)res;
> +}
> + ...
> +static void print_update(int i, const char *refname,
> +			 const struct object_id *old_oid,
> +			 const struct object_id *new_oid, unsigned int flags,
> +			 unsigned int type, const char *msg)
> +{
> +	char o[200] = "null";
> +	char n[200] = "null";

I thought we had better constant than 200 (later you use 100 in the
same patch).  I am not sure how I feel about "null"; all places in
Git, we tend to use 0{length-of-the-hash} for "no object name on
this side", I think.

> +	if (old_oid)
> +		oid_to_hex_r(o, old_oid);
> +	if (new_oid)
> +		oid_to_hex_r(n, new_oid);
> +
> +	type &= 0xf; /* see refs.h REF_* */
> +	flags &= REF_HAVE_NEW | REF_HAVE_OLD | REF_NO_DEREF |
> +		REF_FORCE_CREATE_REFLOG;
> +	trace_printf_key(&trace_refs, "%d: %s %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
> +		o, n, flags, type, msg);
> +}
> +
> +static void print_transaction(struct ref_transaction *transaction)
> +{
> +	trace_printf_key(&trace_refs, "transaction {\n");
> +	for (int i = 0; i < transaction->nr; i++) {

We still do not allow variable decl inside the set-up part of a
for(;;) statement, if I recall Documentation/CodingGuidelines
correctly.

> +static int debug_delete_refs(struct ref_store *ref_store, const char *msg,
> +			     struct string_list *refnames, unsigned int flags)
> +{
> +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
> +	int res =
> +		drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
> +	int i = 0;

No need to initialize 'i' here.

> +	trace_printf_key(&trace_refs, "delete_refs {\n");
> +	for (i = 0; i < refnames->nr; i++)
> +		trace_printf_key(&trace_refs, "%s\n", refnames->items[i].string);
> +	trace_printf_key(&trace_refs, "}: %d\n", res);
> +	return res;
> +}

Thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] refs: add GIT_TRACE_REFS debugging mechanism
  2020-09-08 21:15   ` Junio C Hamano
@ 2020-09-09  9:43     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 10+ messages in thread
From: Han-Wen Nienhuys @ 2020-09-09  9:43 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Han-Wen Nienhuys via GitGitGadget, git, Jonathan Tan,
	Jonathan Nieder, Jeff King, Han-Wen Nienhuys

On Tue, Sep 8, 2020 at 11:15 PM Junio C Hamano <gitster@pobox.com> wrote:
> Cute.  I like it.

thanks :)

> > +     res = malloc(sizeof(struct debug_ref_store));
> > +     be_copy = malloc(sizeof(*be_copy));
>
> Not xmalloc() and friends?

done.

>
> > +     *be_copy = refs_be_debug;
> > +     be_copy->name = store->be->name;
>
> I guess we never destroy the ref-store instances so it is OK to
> assume that it would not cause problems later by sharing pieces of
> memory with underlying "real" thing like this.

correct. But changed to xstrdup to avoid confusion.

> > +     char o[200] = "null";
> > +     char n[200] = "null";
>
> I thought we had better constant than 200 (later you use 100 in the

done.

> same patch).  I am not sure how I feel about "null"; all places in
> Git, we tend to use 0{length-of-the-hash} for "no object name on
> this side", I think.

Some functions take NULL as a legitimate parameter, eg. a transaction
update with NULL as old OID (don't check old OID) is different from an
update with 0{40} as old OID. (ref must not exist yet).

> > +     for (int i = 0; i < transaction->nr; i++) {
>
> We still do not allow variable decl inside the set-up part of a
> for(;;) statement, if I recall Documentation/CodingGuidelines
> correctly.

Fixed.

> > +static int debug_delete_refs(struct ref_store *ref_store, const char *msg,
> > +                          struct string_list *refnames, unsigned int flags)
> > +{
> > +     struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
> > +     int res =
> > +             drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
> > +     int i = 0;
>
> No need to initialize 'i' here.

Fixed.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3] refs: add GIT_TRACE_REFS debugging mechanism
  2020-09-07 15:24 ` [PATCH v2] refs: add GIT_TRACE_REFS " Han-Wen Nienhuys via GitGitGadget
  2020-09-08 21:15   ` Junio C Hamano
@ 2020-09-09 10:15   ` Han-Wen Nienhuys via GitGitGadget
  2020-09-16 15:53     ` [PATCH v4] " Han-Wen Nienhuys via GitGitGadget
  1 sibling, 1 reply; 10+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2020-09-09 10:15 UTC (permalink / raw)
  To: git
  Cc: Jonathan Tan, Jonathan Nieder, Jeff King, Han-Wen Nienhuys,
	Han-Wen Nienhuys, Han-Wen Nienhuys

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

When set in the environment, GIT_TRACE_REFS makes git print operations and
results as they flow through the ref storage backend. This helps debug
discrepancies between different ref backends.

Example:

    $ GIT_TRACE_REFS="1" ./git branch
    15:42:09.769631 refs/debug.c:26         ref_store for .git
    15:42:09.769681 refs/debug.c:249        read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
    15:42:09.769695 refs/debug.c:249        read_raw_ref: refs/heads/ref-debug: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a (=> refs/heads/ref-debug) type 0: 0
    15:42:09.770282 refs/debug.c:233        ref_iterator_begin: refs/heads/ (0x1)
    15:42:09.770290 refs/debug.c:189        iterator_advance: refs/heads/b4 (0)
    15:42:09.770295 refs/debug.c:189        iterator_advance: refs/heads/branch3 (0)

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
    refs: add GIT_TRACE_REFS debugging mechanism
    
    When set in the environment, GIT_TRACE_REFS makes git print operations
    and results as they flow through the ref storage backend. This helps
    debug discrepancies between different ref backends.
    
    Example:
    
        $ GIT_TRACE_REFS="1" ./git branch
        15:42:09.769631 refs/debug.c:26         ref_store for .git
        15:42:09.769681 refs/debug.c:249        read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
        15:42:09.769695 refs/debug.c:249        read_raw_ref: refs/heads/ref-debug: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a (=> refs/heads/ref-debug) type 0: 0
        15:42:09.770282 refs/debug.c:233        ref_iterator_begin: refs/heads/ (0x1)
        15:42:09.770290 refs/debug.c:189        iterator_advance: refs/heads/b4 (0)
        15:42:09.770295 refs/debug.c:189        iterator_advance: refs/heads/branch3 (0)
    
    Signed-off-by: Han-Wen Nienhuys hanwen@google.com [hanwen@google.com]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-713%2Fhanwen%2Fdebug-refs-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-713/hanwen/debug-refs-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/713

Range-diff vs v2:

 1:  19330e894c ! 1:  e19e2a3433 refs: add GIT_TRACE_REFS debugging mechanism
     @@ refs/debug.c (new)
      +	if (!trace_want(&trace_refs)) {
      +		return store;
      +	}
     -+	res = malloc(sizeof(struct debug_ref_store));
     -+	be_copy = malloc(sizeof(*be_copy));
     ++	res = xmalloc(sizeof(struct debug_ref_store));
     ++	be_copy = xmalloc(sizeof(*be_copy));
      +	*be_copy = refs_be_debug;
     ++	/* we never deallocate backends, so safe to copy the pointer. */
      +	be_copy->name = store->be->name;
      +	trace_printf_key(&trace_refs, "ref_store for %s\n", gitdir);
      +	res->refs = store;
     @@ refs/debug.c (new)
      +			 const struct object_id *new_oid, unsigned int flags,
      +			 unsigned int type, const char *msg)
      +{
     -+	char o[200] = "null";
     -+	char n[200] = "null";
     ++	char o[GIT_MAX_HEXSZ + 1] = "null";
     ++	char n[GIT_MAX_HEXSZ + 1] = "null";
      +	if (old_oid)
      +		oid_to_hex_r(o, old_oid);
      +	if (new_oid)
     @@ refs/debug.c (new)
      +
      +static void print_transaction(struct ref_transaction *transaction)
      +{
     ++	int i;
      +	trace_printf_key(&trace_refs, "transaction {\n");
     -+	for (int i = 0; i < transaction->nr; i++) {
     ++	for (i = 0; i < transaction->nr; i++) {
      +		struct ref_update *u = transaction->updates[i];
      +		print_update(i, u->refname, &u->old_oid, &u->new_oid, u->flags,
      +			     u->type, u->msg);
     @@ refs/debug.c (new)
      +	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
      +	int res =
      +		drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
     -+	int i = 0;
     ++	int i;
      +	trace_printf_key(&trace_refs, "delete_refs {\n");
      +	for (i = 0; i < refnames->nr; i++)
      +		trace_printf_key(&trace_refs, "%s\n", refnames->items[i].string);
     @@ refs/debug.c (new)
      +{
      +	struct debug_reflog *dbg = (struct debug_reflog *)cb_data;
      +	int ret;
     -+	char o[100] = "null";
     -+	char n[100] = "null";
     ++	char o[GIT_MAX_HEXSZ + 1] = "null";
     ++	char n[GIT_MAX_HEXSZ + 1] = "null";
      +	if (old_oid)
      +		oid_to_hex_r(o, old_oid);
      +	if (new_oid)


 Documentation/git.txt |   4 +
 Makefile              |   1 +
 refs.c                |   1 +
 refs/debug.c          | 398 ++++++++++++++++++++++++++++++++++++++++++
 refs/refs-internal.h  |   5 +
 5 files changed, 409 insertions(+)
 create mode 100644 refs/debug.c

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2f72b10224..c463b937a8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -722,6 +722,10 @@ of clones and fetches.
 	time of each Git command.
 	See `GIT_TRACE` for available trace output options.
 
+`GIT_TRACE_REFS`::
+	Enables trace messages for operations on the ref database.
+	See `GIT_TRACE` for available trace output options.
+
 `GIT_TRACE_SETUP`::
 	Enables trace messages printing the .git, working tree and current
 	working directory after Git has completed its setup phase.
diff --git a/Makefile b/Makefile
index 86e5411f39..583b8c526e 100644
--- a/Makefile
+++ b/Makefile
@@ -950,6 +950,7 @@ LIB_OBJS += rebase.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
+LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
diff --git a/refs.c b/refs.c
index 156fdcd459..5febfe2281 100644
--- a/refs.c
+++ b/refs.c
@@ -1774,6 +1774,7 @@ struct ref_store *get_main_ref_store(struct repository *r)
 		BUG("attempting to get main_ref_store outside of repository");
 
 	r->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
+	r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private);
 	return r->refs_private;
 }
 
diff --git a/refs/debug.c b/refs/debug.c
new file mode 100644
index 0000000000..922e64fa6a
--- /dev/null
+++ b/refs/debug.c
@@ -0,0 +1,398 @@
+
+#include "refs-internal.h"
+#include "trace.h"
+
+static struct trace_key trace_refs = TRACE_KEY_INIT(REFS);
+
+struct debug_ref_store {
+	struct ref_store base;
+	struct ref_store *refs;
+};
+
+extern struct ref_storage_be refs_be_debug;
+
+struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store)
+{
+	struct debug_ref_store *res;
+	struct ref_storage_be *be_copy;
+
+	if (!trace_want(&trace_refs)) {
+		return store;
+	}
+	res = xmalloc(sizeof(struct debug_ref_store));
+	be_copy = xmalloc(sizeof(*be_copy));
+	*be_copy = refs_be_debug;
+	/* we never deallocate backends, so safe to copy the pointer. */
+	be_copy->name = store->be->name;
+	trace_printf_key(&trace_refs, "ref_store for %s\n", gitdir);
+	res->refs = store;
+	base_ref_store_init((struct ref_store *)res, be_copy);
+	return (struct ref_store *)res;
+}
+
+static int debug_init_db(struct ref_store *refs, struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res = drefs->refs->be->init_db(drefs->refs, err);
+	trace_printf_key(&trace_refs, "init_db: %d\n", res);
+	return res;
+}
+
+static int debug_transaction_prepare(struct ref_store *refs,
+				     struct ref_transaction *transaction,
+				     struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_prepare(drefs->refs, transaction,
+						   err);
+	trace_printf_key(&trace_refs, "transaction_prepare: %d\n", res);
+	return res;
+}
+
+static void print_update(int i, const char *refname,
+			 const struct object_id *old_oid,
+			 const struct object_id *new_oid, unsigned int flags,
+			 unsigned int type, const char *msg)
+{
+	char o[GIT_MAX_HEXSZ + 1] = "null";
+	char n[GIT_MAX_HEXSZ + 1] = "null";
+	if (old_oid)
+		oid_to_hex_r(o, old_oid);
+	if (new_oid)
+		oid_to_hex_r(n, new_oid);
+
+	type &= 0xf; /* see refs.h REF_* */
+	flags &= REF_HAVE_NEW | REF_HAVE_OLD | REF_NO_DEREF |
+		REF_FORCE_CREATE_REFLOG;
+	trace_printf_key(&trace_refs, "%d: %s %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
+		o, n, flags, type, msg);
+}
+
+static void print_transaction(struct ref_transaction *transaction)
+{
+	int i;
+	trace_printf_key(&trace_refs, "transaction {\n");
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *u = transaction->updates[i];
+		print_update(i, u->refname, &u->old_oid, &u->new_oid, u->flags,
+			     u->type, u->msg);
+	}
+	trace_printf_key(&trace_refs, "}\n");
+}
+
+static int debug_transaction_finish(struct ref_store *refs,
+				    struct ref_transaction *transaction,
+				    struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_finish(drefs->refs, transaction,
+						  err);
+	print_transaction(transaction);
+	trace_printf_key(&trace_refs, "finish: %d\n", res);
+	return res;
+}
+
+static int debug_transaction_abort(struct ref_store *refs,
+				   struct ref_transaction *transaction,
+				   struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_abort(drefs->refs, transaction, err);
+	return res;
+}
+
+static int debug_initial_transaction_commit(struct ref_store *refs,
+					    struct ref_transaction *transaction,
+					    struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->initial_transaction_commit(drefs->refs,
+							  transaction, err);
+	return res;
+}
+
+static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->pack_refs(drefs->refs, flags);
+	trace_printf_key(&trace_refs, "pack_refs: %d\n", res);
+	return res;
+}
+
+static int debug_create_symref(struct ref_store *ref_store,
+			       const char *ref_name, const char *target,
+			       const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->create_symref(drefs->refs, ref_name, target,
+						 logmsg);
+	trace_printf_key(&trace_refs, "create_symref: %s -> %s \"%s\": %d\n", ref_name,
+		target, logmsg, res);
+	return res;
+}
+
+static int debug_delete_refs(struct ref_store *ref_store, const char *msg,
+			     struct string_list *refnames, unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res =
+		drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
+	int i;
+	trace_printf_key(&trace_refs, "delete_refs {\n");
+	for (i = 0; i < refnames->nr; i++)
+		trace_printf_key(&trace_refs, "%s\n", refnames->items[i].string);
+	trace_printf_key(&trace_refs, "}: %d\n", res);
+	return res;
+}
+
+static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,
+			    const char *newref, const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->rename_ref(drefs->refs, oldref, newref,
+					      logmsg);
+	trace_printf_key(&trace_refs, "rename_ref: %s -> %s \"%s\": %d\n", oldref, newref,
+		logmsg, res);
+	return res;
+}
+
+static int debug_copy_ref(struct ref_store *ref_store, const char *oldref,
+			  const char *newref, const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res =
+		drefs->refs->be->copy_ref(drefs->refs, oldref, newref, logmsg);
+	trace_printf_key(&trace_refs, "copy_ref: %s -> %s \"%s\": %d\n", oldref, newref,
+		logmsg, res);
+	return res;
+}
+
+struct debug_ref_iterator {
+	struct ref_iterator base;
+	struct ref_iterator *iter;
+};
+
+static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->advance(diter->iter);
+	if (res)
+		trace_printf_key(&trace_refs, "iterator_advance: (%d)\n", res);
+	else
+		trace_printf_key(&trace_refs, "iterator_advance: %s (0)\n",
+			diter->iter->refname);
+
+	diter->base.ordered = diter->iter->ordered;
+	diter->base.refname = diter->iter->refname;
+	diter->base.oid = diter->iter->oid;
+	diter->base.flags = diter->iter->flags;
+	return res;
+}
+
+static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator,
+				   struct object_id *peeled)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->peel(diter->iter, peeled);
+	trace_printf_key(&trace_refs, "iterator_peel: %s: %d\n", diter->iter->refname, res);
+	return res;
+}
+
+static int debug_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->abort(diter->iter);
+	trace_printf_key(&trace_refs, "iterator_abort: %d\n", res);
+	return res;
+}
+
+static struct ref_iterator_vtable debug_ref_iterator_vtable = {
+	debug_ref_iterator_advance, debug_ref_iterator_peel,
+	debug_ref_iterator_abort
+};
+
+static struct ref_iterator *
+debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
+			 unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_iterator *res =
+		drefs->refs->be->iterator_begin(drefs->refs, prefix, flags);
+	struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));
+	base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);
+	diter->iter = res;
+	trace_printf_key(&trace_refs, "ref_iterator_begin: %s (0x%x)\n", prefix, flags);
+	return &diter->base;
+}
+
+static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
+			      struct object_id *oid, struct strbuf *referent,
+			      unsigned int *type)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = 0;
+
+	oidcpy(oid, &null_oid);
+	res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
+					    type);
+
+	if (res == 0) {
+		trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
+			refname, oid_to_hex(oid), referent->buf, *type, res);
+	} else {
+		trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res);
+	}
+	return res;
+}
+
+static struct ref_iterator *
+debug_reflog_iterator_begin(struct ref_store *ref_store)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_iterator *res =
+		drefs->refs->be->reflog_iterator_begin(drefs->refs);
+	trace_printf_key(&trace_refs, "for_each_reflog_iterator_begin\n");
+	return res;
+}
+
+struct debug_reflog {
+	const char *refname;
+	each_reflog_ent_fn *fn;
+	void *cb_data;
+};
+
+static int debug_print_reflog_ent(struct object_id *old_oid,
+				  struct object_id *new_oid,
+				  const char *committer, timestamp_t timestamp,
+				  int tz, const char *msg, void *cb_data)
+{
+	struct debug_reflog *dbg = (struct debug_reflog *)cb_data;
+	int ret;
+	char o[GIT_MAX_HEXSZ + 1] = "null";
+	char n[GIT_MAX_HEXSZ + 1] = "null";
+	if (old_oid)
+		oid_to_hex_r(o, old_oid);
+	if (new_oid)
+		oid_to_hex_r(n, new_oid);
+
+	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
+		      dbg->cb_data);
+	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
+		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	return ret;
+}
+
+static int debug_for_each_reflog_ent(struct ref_store *ref_store,
+				     const char *refname, each_reflog_ent_fn fn,
+				     void *cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog dbg = {
+		.refname = refname,
+		.fn = fn,
+		.cb_data = cb_data,
+	};
+
+	int res = drefs->refs->be->for_each_reflog_ent(
+		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
+	trace_printf_key(&trace_refs, "for_each_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_for_each_reflog_ent_reverse(struct ref_store *ref_store,
+					     const char *refname,
+					     each_reflog_ent_fn fn,
+					     void *cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog dbg = {
+		.refname = refname,
+		.fn = fn,
+		.cb_data = cb_data,
+	};
+	int res = drefs->refs->be->for_each_reflog_ent_reverse(
+		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
+	trace_printf_key(&trace_refs, "for_each_reflog_reverse: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->reflog_exists(drefs->refs, refname);
+	trace_printf_key(&trace_refs, "reflog_exists: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_create_reflog(struct ref_store *ref_store, const char *refname,
+			       int force_create, struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->create_reflog(drefs->refs, refname,
+						 force_create, err);
+	trace_printf_key(&trace_refs, "create_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_delete_reflog(struct ref_store *ref_store, const char *refname)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->delete_reflog(drefs->refs, refname);
+	trace_printf_key(&trace_refs, "delete_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+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,
+			       reflog_expiry_should_prune_fn should_prune_fn,
+			       reflog_expiry_cleanup_fn cleanup_fn,
+			       void *policy_cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->reflog_expire(drefs->refs, refname, oid,
+						 flags, prepare_fn,
+						 should_prune_fn, cleanup_fn,
+						 policy_cb_data);
+	trace_printf_key(&trace_refs, "reflog_expire: %s: %d\n", refname, res);
+	return res;
+}
+
+struct ref_storage_be refs_be_debug = {
+	NULL,
+	"debug",
+	NULL,
+	debug_init_db,
+	debug_transaction_prepare,
+	debug_transaction_finish,
+	debug_transaction_abort,
+	debug_initial_transaction_commit,
+
+	debug_pack_refs,
+	debug_create_symref,
+	debug_delete_refs,
+	debug_rename_ref,
+	debug_copy_ref,
+
+	debug_ref_iterator_begin,
+	debug_read_raw_ref,
+
+	debug_reflog_iterator_begin,
+	debug_for_each_reflog_ent,
+	debug_for_each_reflog_ent_reverse,
+	debug_reflog_exists,
+	debug_create_reflog,
+	debug_delete_reflog,
+	debug_reflog_expire,
+};
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 527b0a6e2e..fecde40baf 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -690,4 +690,9 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be);
 
+/*
+ * Support GIT_TRACE_REFS by optionally wrapping the given ref_store instance.
+ */
+struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store);
+
 #endif /* REFS_REFS_INTERNAL_H */

base-commit: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4] refs: add GIT_TRACE_REFS debugging mechanism
  2020-09-09 10:15   ` [PATCH v3] " Han-Wen Nienhuys via GitGitGadget
@ 2020-09-16 15:53     ` Han-Wen Nienhuys via GitGitGadget
  0 siblings, 0 replies; 10+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2020-09-16 15:53 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

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

When set in the environment, GIT_TRACE_REFS makes git print operations and
results as they flow through the ref storage backend. This helps debug
discrepancies between different ref backends.

Example:

    $ GIT_TRACE_REFS="1" ./git branch
    15:42:09.769631 refs/debug.c:26         ref_store for .git
    15:42:09.769681 refs/debug.c:249        read_raw_ref: HEAD: 0000000000000000000000000000000000000000 (=> refs/heads/ref-debug) type 1: 0
    15:42:09.769695 refs/debug.c:249        read_raw_ref: refs/heads/ref-debug: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a (=> refs/heads/ref-debug) type 0: 0
    15:42:09.770282 refs/debug.c:233        ref_iterator_begin: refs/heads/ (0x1)
    15:42:09.770290 refs/debug.c:189        iterator_advance: refs/heads/b4 (0)
    15:42:09.770295 refs/debug.c:189        iterator_advance: refs/heads/branch3 (0)

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
    refs: add GIT_TRACE_REFS debugging mechanism
    
    When set in the environment, GIT_TRACE_REFS makes git print operations
    and results as they flow through the ref storage backend. This helps
    debug discrepancies between different ref backends.
    
    cc: Jonathan Tan jonathantanmy@google.com [jonathantanmy@google.com]cc:
    Jonathan Nieder jrnieder@gmail.com [jrnieder@gmail.com]cc: Jeff King 
    peff@peff.net [peff@peff.net]cc: Han-Wen Nienhuys hanwen@google.com
    [hanwen@google.com]
    
    v3: use xstrdup to copy backend name; I forgot to commit this bit in the
    last round.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-713%2Fhanwen%2Fdebug-refs-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-713/hanwen/debug-refs-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/713

Range-diff vs v3:

 1:  e19e2a3433 ! 1:  e73293f0f7 refs: add GIT_TRACE_REFS debugging mechanism
     @@ refs/debug.c (new)
      +	res = xmalloc(sizeof(struct debug_ref_store));
      +	be_copy = xmalloc(sizeof(*be_copy));
      +	*be_copy = refs_be_debug;
     -+	/* we never deallocate backends, so safe to copy the pointer. */
     -+	be_copy->name = store->be->name;
     ++	be_copy->name = xstrdup(store->be->name);
      +	trace_printf_key(&trace_refs, "ref_store for %s\n", gitdir);
      +	res->refs = store;
      +	base_ref_store_init((struct ref_store *)res, be_copy);


 Documentation/git.txt |   4 +
 Makefile              |   1 +
 refs.c                |   1 +
 refs/debug.c          | 397 ++++++++++++++++++++++++++++++++++++++++++
 refs/refs-internal.h  |   5 +
 5 files changed, 408 insertions(+)
 create mode 100644 refs/debug.c

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2f72b10224..c463b937a8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -722,6 +722,10 @@ of clones and fetches.
 	time of each Git command.
 	See `GIT_TRACE` for available trace output options.
 
+`GIT_TRACE_REFS`::
+	Enables trace messages for operations on the ref database.
+	See `GIT_TRACE` for available trace output options.
+
 `GIT_TRACE_SETUP`::
 	Enables trace messages printing the .git, working tree and current
 	working directory after Git has completed its setup phase.
diff --git a/Makefile b/Makefile
index 86e5411f39..583b8c526e 100644
--- a/Makefile
+++ b/Makefile
@@ -950,6 +950,7 @@ LIB_OBJS += rebase.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
+LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
diff --git a/refs.c b/refs.c
index 156fdcd459..5febfe2281 100644
--- a/refs.c
+++ b/refs.c
@@ -1774,6 +1774,7 @@ struct ref_store *get_main_ref_store(struct repository *r)
 		BUG("attempting to get main_ref_store outside of repository");
 
 	r->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
+	r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private);
 	return r->refs_private;
 }
 
diff --git a/refs/debug.c b/refs/debug.c
new file mode 100644
index 0000000000..6588a556d0
--- /dev/null
+++ b/refs/debug.c
@@ -0,0 +1,397 @@
+
+#include "refs-internal.h"
+#include "trace.h"
+
+static struct trace_key trace_refs = TRACE_KEY_INIT(REFS);
+
+struct debug_ref_store {
+	struct ref_store base;
+	struct ref_store *refs;
+};
+
+extern struct ref_storage_be refs_be_debug;
+
+struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store)
+{
+	struct debug_ref_store *res;
+	struct ref_storage_be *be_copy;
+
+	if (!trace_want(&trace_refs)) {
+		return store;
+	}
+	res = xmalloc(sizeof(struct debug_ref_store));
+	be_copy = xmalloc(sizeof(*be_copy));
+	*be_copy = refs_be_debug;
+	be_copy->name = xstrdup(store->be->name);
+	trace_printf_key(&trace_refs, "ref_store for %s\n", gitdir);
+	res->refs = store;
+	base_ref_store_init((struct ref_store *)res, be_copy);
+	return (struct ref_store *)res;
+}
+
+static int debug_init_db(struct ref_store *refs, struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res = drefs->refs->be->init_db(drefs->refs, err);
+	trace_printf_key(&trace_refs, "init_db: %d\n", res);
+	return res;
+}
+
+static int debug_transaction_prepare(struct ref_store *refs,
+				     struct ref_transaction *transaction,
+				     struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_prepare(drefs->refs, transaction,
+						   err);
+	trace_printf_key(&trace_refs, "transaction_prepare: %d\n", res);
+	return res;
+}
+
+static void print_update(int i, const char *refname,
+			 const struct object_id *old_oid,
+			 const struct object_id *new_oid, unsigned int flags,
+			 unsigned int type, const char *msg)
+{
+	char o[GIT_MAX_HEXSZ + 1] = "null";
+	char n[GIT_MAX_HEXSZ + 1] = "null";
+	if (old_oid)
+		oid_to_hex_r(o, old_oid);
+	if (new_oid)
+		oid_to_hex_r(n, new_oid);
+
+	type &= 0xf; /* see refs.h REF_* */
+	flags &= REF_HAVE_NEW | REF_HAVE_OLD | REF_NO_DEREF |
+		REF_FORCE_CREATE_REFLOG;
+	trace_printf_key(&trace_refs, "%d: %s %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
+		o, n, flags, type, msg);
+}
+
+static void print_transaction(struct ref_transaction *transaction)
+{
+	int i;
+	trace_printf_key(&trace_refs, "transaction {\n");
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *u = transaction->updates[i];
+		print_update(i, u->refname, &u->old_oid, &u->new_oid, u->flags,
+			     u->type, u->msg);
+	}
+	trace_printf_key(&trace_refs, "}\n");
+}
+
+static int debug_transaction_finish(struct ref_store *refs,
+				    struct ref_transaction *transaction,
+				    struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_finish(drefs->refs, transaction,
+						  err);
+	print_transaction(transaction);
+	trace_printf_key(&trace_refs, "finish: %d\n", res);
+	return res;
+}
+
+static int debug_transaction_abort(struct ref_store *refs,
+				   struct ref_transaction *transaction,
+				   struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_abort(drefs->refs, transaction, err);
+	return res;
+}
+
+static int debug_initial_transaction_commit(struct ref_store *refs,
+					    struct ref_transaction *transaction,
+					    struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->initial_transaction_commit(drefs->refs,
+							  transaction, err);
+	return res;
+}
+
+static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->pack_refs(drefs->refs, flags);
+	trace_printf_key(&trace_refs, "pack_refs: %d\n", res);
+	return res;
+}
+
+static int debug_create_symref(struct ref_store *ref_store,
+			       const char *ref_name, const char *target,
+			       const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->create_symref(drefs->refs, ref_name, target,
+						 logmsg);
+	trace_printf_key(&trace_refs, "create_symref: %s -> %s \"%s\": %d\n", ref_name,
+		target, logmsg, res);
+	return res;
+}
+
+static int debug_delete_refs(struct ref_store *ref_store, const char *msg,
+			     struct string_list *refnames, unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res =
+		drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
+	int i;
+	trace_printf_key(&trace_refs, "delete_refs {\n");
+	for (i = 0; i < refnames->nr; i++)
+		trace_printf_key(&trace_refs, "%s\n", refnames->items[i].string);
+	trace_printf_key(&trace_refs, "}: %d\n", res);
+	return res;
+}
+
+static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,
+			    const char *newref, const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->rename_ref(drefs->refs, oldref, newref,
+					      logmsg);
+	trace_printf_key(&trace_refs, "rename_ref: %s -> %s \"%s\": %d\n", oldref, newref,
+		logmsg, res);
+	return res;
+}
+
+static int debug_copy_ref(struct ref_store *ref_store, const char *oldref,
+			  const char *newref, const char *logmsg)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res =
+		drefs->refs->be->copy_ref(drefs->refs, oldref, newref, logmsg);
+	trace_printf_key(&trace_refs, "copy_ref: %s -> %s \"%s\": %d\n", oldref, newref,
+		logmsg, res);
+	return res;
+}
+
+struct debug_ref_iterator {
+	struct ref_iterator base;
+	struct ref_iterator *iter;
+};
+
+static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->advance(diter->iter);
+	if (res)
+		trace_printf_key(&trace_refs, "iterator_advance: (%d)\n", res);
+	else
+		trace_printf_key(&trace_refs, "iterator_advance: %s (0)\n",
+			diter->iter->refname);
+
+	diter->base.ordered = diter->iter->ordered;
+	diter->base.refname = diter->iter->refname;
+	diter->base.oid = diter->iter->oid;
+	diter->base.flags = diter->iter->flags;
+	return res;
+}
+
+static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator,
+				   struct object_id *peeled)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->peel(diter->iter, peeled);
+	trace_printf_key(&trace_refs, "iterator_peel: %s: %d\n", diter->iter->refname, res);
+	return res;
+}
+
+static int debug_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+	struct debug_ref_iterator *diter =
+		(struct debug_ref_iterator *)ref_iterator;
+	int res = diter->iter->vtable->abort(diter->iter);
+	trace_printf_key(&trace_refs, "iterator_abort: %d\n", res);
+	return res;
+}
+
+static struct ref_iterator_vtable debug_ref_iterator_vtable = {
+	debug_ref_iterator_advance, debug_ref_iterator_peel,
+	debug_ref_iterator_abort
+};
+
+static struct ref_iterator *
+debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
+			 unsigned int flags)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_iterator *res =
+		drefs->refs->be->iterator_begin(drefs->refs, prefix, flags);
+	struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));
+	base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);
+	diter->iter = res;
+	trace_printf_key(&trace_refs, "ref_iterator_begin: %s (0x%x)\n", prefix, flags);
+	return &diter->base;
+}
+
+static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
+			      struct object_id *oid, struct strbuf *referent,
+			      unsigned int *type)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = 0;
+
+	oidcpy(oid, &null_oid);
+	res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
+					    type);
+
+	if (res == 0) {
+		trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
+			refname, oid_to_hex(oid), referent->buf, *type, res);
+	} else {
+		trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res);
+	}
+	return res;
+}
+
+static struct ref_iterator *
+debug_reflog_iterator_begin(struct ref_store *ref_store)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_iterator *res =
+		drefs->refs->be->reflog_iterator_begin(drefs->refs);
+	trace_printf_key(&trace_refs, "for_each_reflog_iterator_begin\n");
+	return res;
+}
+
+struct debug_reflog {
+	const char *refname;
+	each_reflog_ent_fn *fn;
+	void *cb_data;
+};
+
+static int debug_print_reflog_ent(struct object_id *old_oid,
+				  struct object_id *new_oid,
+				  const char *committer, timestamp_t timestamp,
+				  int tz, const char *msg, void *cb_data)
+{
+	struct debug_reflog *dbg = (struct debug_reflog *)cb_data;
+	int ret;
+	char o[GIT_MAX_HEXSZ + 1] = "null";
+	char n[GIT_MAX_HEXSZ + 1] = "null";
+	if (old_oid)
+		oid_to_hex_r(o, old_oid);
+	if (new_oid)
+		oid_to_hex_r(n, new_oid);
+
+	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
+		      dbg->cb_data);
+	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
+		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	return ret;
+}
+
+static int debug_for_each_reflog_ent(struct ref_store *ref_store,
+				     const char *refname, each_reflog_ent_fn fn,
+				     void *cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog dbg = {
+		.refname = refname,
+		.fn = fn,
+		.cb_data = cb_data,
+	};
+
+	int res = drefs->refs->be->for_each_reflog_ent(
+		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
+	trace_printf_key(&trace_refs, "for_each_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_for_each_reflog_ent_reverse(struct ref_store *ref_store,
+					     const char *refname,
+					     each_reflog_ent_fn fn,
+					     void *cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct debug_reflog dbg = {
+		.refname = refname,
+		.fn = fn,
+		.cb_data = cb_data,
+	};
+	int res = drefs->refs->be->for_each_reflog_ent_reverse(
+		drefs->refs, refname, &debug_print_reflog_ent, &dbg);
+	trace_printf_key(&trace_refs, "for_each_reflog_reverse: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->reflog_exists(drefs->refs, refname);
+	trace_printf_key(&trace_refs, "reflog_exists: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_create_reflog(struct ref_store *ref_store, const char *refname,
+			       int force_create, struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->create_reflog(drefs->refs, refname,
+						 force_create, err);
+	trace_printf_key(&trace_refs, "create_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+static int debug_delete_reflog(struct ref_store *ref_store, const char *refname)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->delete_reflog(drefs->refs, refname);
+	trace_printf_key(&trace_refs, "delete_reflog: %s: %d\n", refname, res);
+	return res;
+}
+
+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,
+			       reflog_expiry_should_prune_fn should_prune_fn,
+			       reflog_expiry_cleanup_fn cleanup_fn,
+			       void *policy_cb_data)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	int res = drefs->refs->be->reflog_expire(drefs->refs, refname, oid,
+						 flags, prepare_fn,
+						 should_prune_fn, cleanup_fn,
+						 policy_cb_data);
+	trace_printf_key(&trace_refs, "reflog_expire: %s: %d\n", refname, res);
+	return res;
+}
+
+struct ref_storage_be refs_be_debug = {
+	NULL,
+	"debug",
+	NULL,
+	debug_init_db,
+	debug_transaction_prepare,
+	debug_transaction_finish,
+	debug_transaction_abort,
+	debug_initial_transaction_commit,
+
+	debug_pack_refs,
+	debug_create_symref,
+	debug_delete_refs,
+	debug_rename_ref,
+	debug_copy_ref,
+
+	debug_ref_iterator_begin,
+	debug_read_raw_ref,
+
+	debug_reflog_iterator_begin,
+	debug_for_each_reflog_ent,
+	debug_for_each_reflog_ent_reverse,
+	debug_reflog_exists,
+	debug_create_reflog,
+	debug_delete_reflog,
+	debug_reflog_expire,
+};
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 527b0a6e2e..fecde40baf 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -690,4 +690,9 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be);
 
+/*
+ * Support GIT_TRACE_REFS by optionally wrapping the given ref_store instance.
+ */
+struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store);
+
 #endif /* REFS_REFS_INTERNAL_H */

base-commit: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-09-16 21:10 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-28 15:28 [PATCH] RFC: refs: add GIT_DEBUG_REFS debugging mechanism Han-Wen Nienhuys via GitGitGadget
2020-09-02 17:49 ` Jonathan Tan
2020-09-03  5:44   ` Jonathan Nieder
2020-09-03  6:19     ` Jeff King
     [not found]       ` <CAFQ2z_M_2mmL1LXcgR5cv3Ac1AJ363jaejZz3p6AcV6b3DftTQ@mail.gmail.com>
2020-09-03 10:10         ` Fwd: " Han-Wen Nienhuys
2020-09-07 15:24 ` [PATCH v2] refs: add GIT_TRACE_REFS " Han-Wen Nienhuys via GitGitGadget
2020-09-08 21:15   ` Junio C Hamano
2020-09-09  9:43     ` Han-Wen Nienhuys
2020-09-09 10:15   ` [PATCH v3] " Han-Wen Nienhuys via GitGitGadget
2020-09-16 15:53     ` [PATCH v4] " Han-Wen Nienhuys via GitGitGadget

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).