Re: [PATCHv3 3/3] http.c: implements the GIT_TRACE_CURL environment variable
Elia Pinto <gitter.spiros@gmail.com> writes:
> Implements the GIT_TRACE_CURL environment variable to allow a
s/Implements/Implement/; speak as if you are giving an order to the codebase to "be like so".
Show 33 quoted lines
> greater degree of detail of GIT_CURL_VERBOSE, in particular
> the complete transport header and all the data payload exchanged.
> It might be useful if a particular situation could require a more
> thorough debugging analysis.
>
> Helped-by: Torsten Bögershausen <tboegi@web.de>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> http.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> http.h | 6 ++++
> 2 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 4304b80..507c386 100644
> --- a/http.c
> +++ b/http.c
> @@ -11,6 +11,7 @@
> #include "gettext.h"
> #include "transport.h"
>
> +static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
> #if LIBCURL_VERSION_NUM >= 0x070a08
> long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
> #else
> @@ -464,6 +465,100 @@ static void set_curl_keepalive(CURL *c)
> }
> #endif
>
> +
No need for this extra blank line.
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
> +{
> + size_t i;
> + size_t w;Is it better to narrow the scope of 'w' to the "for (i)" loop?
> + struct strbuf out = STRBUF_INIT;
> +
No need for this extra blank line.
Show 8 quoted lines
> + unsigned int width = 0x10;
> +
> + if (nohex)
> + /* without the hex output, we can fit more on screen */
> + width = 0x40;
> +
> + strbuf_addf(&out, "%s, %10.10ld bytes (0x%8.8lx)\n",
> + text, (long)size, (long)size);
Inconsistent indentation that uses only two HT, when existing lines and other new lines in thsi patch align with HT with SP to match "(" on the previous line.
Show 7 quoted lines
> +
> + for (i = 0; i < size; i += width) {
> +
> + strbuf_addf(&out, "%4.4lx: ", (long)i);
> +
> + if (!nohex) {
> + /* hex not disabled, show it */Unlike the previous "without the hex, we can fit more", this comment is probably adds more noise than value.
Show 16 quoted lines
> + for (w = 0; w < width; w++)
> + if (i + w < size)
> + strbuf_addf(&out, "%02x ", ptr[i + w]);
> + else
> + strbuf_addf(&out," ");
> + }
> +
> + for (w = 0; (w < width) && (i + w < size); w++) {
> + /* check for 0D0A; if found, skip past and start a new line of output */
> + if (nohex && (i + w + 1 < size) && ptr[i + w] == '\r'
> + && ptr[i + w + 1] == '\n') {
> + i += (w + 2 - width);
> + break;
> + }
> + strbuf_addch(&out, (ptr[i + w] >= 0x20)
> + && (ptr[i + w] < 0x80) ? ptr[i + w] : '.');Show 8 quoted lines
> + /* check again for 0D0A, to avoid an extra \n if it's at width */
> + if (nohex && (i + w + 2 < size)
> + && ptr[i + w + 1] == '\r'
> + && ptr[i + w + 2] == '\n') {
> + i += (w + 3 - width);
> + break;
> + }
> + }This may only be the matter of taste, but I somehow found this "we pretend to go width bytes at a time, unless there is a line-break in which case we may fold before we hit width bytes" and need for compensating with "w+3-width" here unnecessarily convoluted. I wonder if the code becomes clearer if insterad you said "we go one line at a time, but we may fold the line if it is wider than width bytes"?
Show 13 quoted lines
> + strbuf_addch(&out, '\n');
> + trace_strbuf(&trace_curl, &out);
> + strbuf_release(&out);
> + }
> +}
> +
> +int curl_trace_want(void)
> +{
> + return trace_want(&trace_curl);
> +}
> +
> +int curl_trace(CURL *handle, curl_infotype type,
> + char *data, size_t size, void *userp)Inconsistent indentation.
> +{
> + const char *text;
> + (void)handle; /* prevent compiler warning */What compiler warning? Usually unused parameter (not unused variable) is not something compilers warn against.
> + switch (type) {
> + case CURLINFO_TEXT:
> + trace_printf_key(&trace_curl, "== Info: %s", data);
> + default: /* in case a new one is introduced to shock us */The comment bothers me.
What is the longer term plan for this function? Do we expect to ignore some type of data, or do we expect to show all new type of data? If the former, "we ignore unknown types by default" is fine, and if the latter, it probably makes more sense to show with text="unknown type"?
Show 25 quoted lines
> + return 0;
> +
> + case CURLINFO_HEADER_OUT:
> + text = "=> Send header";
> + break;
> + case CURLINFO_DATA_OUT:
> + text = "=> Send data";
> + break;
> + case CURLINFO_SSL_DATA_OUT:
> + text = "=> Send SSL data";
> + break;
> + case CURLINFO_HEADER_IN:
> + text = "<= Recv header";
> + break;
> + case CURLINFO_DATA_IN:
> + text = "<= Recv data";
> + break;
> + case CURLINFO_SSL_DATA_IN:
> + text = "<= Recv SSL data";
> + break;
> + }
> +
> + curl_dump(text, (unsigned char *)data, size, 1);
> + return 0;
> +}
Elia Pinto
