Re: [PATCH v2] refs: add GIT_TRACE_REFS debugging mechanism
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 8, 2020, 21:15 UTC
- Message-ID
- <xmqq7dt4x957.fsf@gitster.c.googlers.com>
- In-Reply-To
- <pull.713.v2.git.1599492274909.gitgitgadget@gmail.com>
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> 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.
Show 29 quoted lines
> 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.
Show 13 quoted lines
> + 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.
Show 16 quoted lines
> + 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.
Show 7 quoted lines
> +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.
Show 6 quoted lines
> + 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.