Re: [PATCH 7/8] add packet tracing debug code

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 24, 2011, 21:44 UTC
Message-ID: <20110224214412.GG17412@elie>
In-Reply-To: <20110224143019.GG15477@sigill.intra.peff.net>

Jeff King wrote:

> This shows a trace of all packets coming in or out of a given
> program. This can help with debugging object negotiation or
> other protocol issues.

Sounds handy.

> Packet tracing can be enabled with GIT_TRACE_PACKET=<foo>,
> where <foo> takes the same arguments as GIT_TRACE.

[...]

Show 7 quoted lines

> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -383,6 +383,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  
>  	junk_pid = getpid();
>  
> +	packet_trace_identity("clone");

My first thought on reading this was "identity operation" (= pass-through), which left me a little confused.

Maybe "packet_trace_set_identity" (or ...set_prefix) would be a little clearer?

> +++ b/pkt-line.c
> @@ -1,6 +1,51 @@

[...]

Show 6 quoted lines

> +	if ((len >= 4 && !prefixcmp(buf, "PACK")) ||
> +	    (len >= 5 && !prefixcmp(buf+1, "PACK"))) {
> +		strbuf_addstr(&out, "PACK ...");
> +		unsetenv(trace_key);
> +	}
> +	else {

Style: should be cuddled, I think.

> +		/* XXX we should really handle printable utf8 */
> +		for (i = 0; i < len; i++) {

Maybe using is_utf8, extended to take a length argument? (Assuming it's okay if non-ASCII is escaped in packets containing non-utf8 parts.)

[...]

Show 6 quoted lines

> @@ -39,11 +84,13 @@ ssize_t safe_write(int fd, const void *buf, ssize_t n)
>   */
>  void packet_flush(int fd)
>  {
> +	packet_trace("0000", 4, 1);
>  	safe_write(fd, "0000", 4);

... I wonder if it would make sense to do

 static void packet_writebuf(int fd, const char *buf, size_t buflen)
 {
	packet_trace(buf, buflen, 1);
	safe_write(fd, buf, buflen);
 }

Nah, that would include the size noise for even nonempty packets, which your implementation conveniently omits.

Thanks, looks useful.