Re: [PATCH 4/8] trace: refactor to support multiple env variables

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2011, 18:25 UTC
Message-ID: <7vsjvd1e9r.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110224142841.GD15477@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> Right now you turn all tracing off and on with GIT_TRACE. To
> support new types of tracing without forcing the user to see
> all of them, we will soon support turning each tracing area
> on with GIT_TRACE_*.
>
> This patch lays the groundwork by providing an interface
> which does not assume GIT_TRACE. However, we still maintain
> the trace_printf interface so that existing callers do not
> need to be refactored.

One thing I found missing in the patch description is a statement of the overall design and expected usage. It appears to me that the design is built around the idea to give each named/namable area to be traceable its own trace output file descriptor, so that different kinds of trace events are sent to different files.

I however expect that majority of "trace only areas A and B" users would want to see logs of events from these two areas in the same stream to see them in the order they happened. Perhaps you are envisioning that these users would use redirection from the command line to send GIT_TRACE_A and GIT_TRACE_B to the same place; that probably needs to be spelled out more explicitly somewhere in the documentation, as that would be a more common thing to do.

I think your [7/8] is kind of strange when viewed in that light. Imagine what would happen if you gave separate GIT_TRACE_* to each packet class, instead of giving them a single umbrella variable GIT_TRACE_PACKET. If the user wants to see them all in a single stream, the same redirection you would use to unify GIT_TRACE_A and GIT_TRACE_B can be used.

Instead, you have packet class prefix in the output so that later the different kinds of packet events can be sifted out from the unified output, even though they are forced to go to the same output stream. In a sense, you have two-tier classification system for traceable events (the top layer that can be separated or merged at the file descriptor level, and the bottom layer that can only be separated by looking at the prefix).

Is this necessarily a good thing (not a rhetorical question)?

To put it another and opposite way, I wonder if it would be better to instead use a single output stream named by GIT_TRACE and add trace event class prefix to the output for classes like SETUP and PACKET (again, not a rhetorical question).

Also instead of wasting environment variable names, it might be a more compact design from the user's point of view if we took a list of trace event classes in a single environment variable, e.g.

	GIT_TRACE_CLASS=setup,packet \
        GIT_TRACE=/tmp/tr \
        git push

I dunno.