Elia Pinto <gitter.spi...@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".
> 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 <tbo...@web.de>
> Helped-by: Ramsay Jones <ram...@ramsayjones.plus.com>
> Helped-by: Junio C Hamano <gits...@pobox.com>
> Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
> Helped-by: Jeff King <p...@peff.net>
> Signed-off-by: Elia Pinto <gitter.spi...@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.
> + 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.
> +
> + 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.
> + 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] : '.');
Likewise.
> + /* 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"?
> + 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"?
> + 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;
> +}
--
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
