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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
