Re: [PATCHv3 0/3] Implements the GIT_TRACE_CURL environment variable

From: Jeff King <peff@peff.net>
Date: Apr 20, 2016, 20:45 UTC
Message-ID: <20160420204500.GB14904@sigill.intra.peff.net>
In-Reply-To: <5717DD63.6050002@ramsayjones.plus.com>

On Wed, Apr 20, 2016 at 08:49:55PM +0100, Ramsay Jones wrote:

Show 7 quoted lines

> > Strictly speaking 1 should come at the end for the same reason, as
> > setting GIT_TRACE_CURL after seeing that commit would not give users
> > anything new.
> 
> Yep, I was just about to send an email saying that the patches should
> be in the exact opposite order! (ie. 1->3 and 3->1) That is *if* you
> want to keep them as a series. I would squash them into one patch ...

I also wondered about simply squashing them. IMHO it does not help to split documentation from the addition of a feature. It is not as if we will take one over the other, and by putting them in the same patch you do not have to justify one without the other.

Show 5 quoted lines

> > Other than that, I didn't find anything blatantly wrong ;-).  Will
> > nitpick individual patches later but I expect that it would be
> > sufficient to locally tweak while queuing without rerolling.
> 
> I have one small issue ...

Overall I'm pleased at the concept, though I find the output a little funny in places.

Most of the "Send/Recv SSL data" chunks are just line noise. Do people actually care about seeing them? I can conceive of a case where you are debugging SSL-level stuff, but I feel like you might do better using openssl s_client to do so, and not git. Should we stick to more HTTP-level debugging?

For the actual data packets, the first line gets treated differently than the rest, and you get:

16:33:38.164068 http.c:515 => Send header, 0000000167 bytes (0x000000a7) 0000: GET /git/git/info/refs?service=git-upload-pack HTTP/1.1 16:33:38.164070 http.c:515 0039: Host: github.com 16:33:38.164072 http.c:515 004b: User-Agent: git/2.8.1.220.g9816fc6 ...

for instance. Would it be saner to break the "Send header..." bit and the first data line into separate trace outputs, and end up with something more like:

16:33:38.164068 http.c:515 => Send header, 0000000167 bytes (0x000000a7) 16:33:38.164069 http.c:515 0000: GET /git/git/info/refs?service=git-upload-pack HTTP/1.1 16:33:38.164070 http.c:515 0039: Host: github.com 16:33:38.164072 http.c:515 004b: User-Agent: git/2.8.1.220.g9816fc6

Or it might even be nice to prefix each line to indicate it is about sending a header. That would make it much easier to grep for just particular

It might even be nice to prefix _all_ of the lines with some state information, like "send header". That's more verbose, but makes it much easier to pick out snippets with line-oriented tools like grep. I often find myself doing that kind of thing, either to inspect a subset of the output, or because I want to be able to pull out things like request content verbatim so I can replay it.

One of my complaints with GIT_CURL_VERBOSE is that it puts your credentials into the debugging output. Since it looks like we're parsing through the data anyway, I wonder if we could auto-censor Authorization headers by default (and then possibly output them if an extra variable is given). That would make it safe to ask people to show the output of GIT_CURL_TRACE on the list without having to explain further.

-Peff