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

[Jonathan Nieder <jrnieder@gmail.com>]

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.
[...]
> --- 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 @@
[...]
> +	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.)

[...]
> @@ -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.