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

[Jeff King <peff@peff.net>]

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

> > 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.

> > 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