Re: [PATCH v2] refs: add GIT_TRACE_REFS debugging mechanism

[Junio C Hamano <gitster@pobox.com>]

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