[PATCHv4 0/2] Implement the GIT_TRACE_CURL environment variable

[PATCHv4 0/2] Implement the GIT_TRACE_CURL environment variable

[Elia Pinto]

This is the fourth version but in reality is the complete rewriting of the patches discussed here
(here called V1)

$gmane/290520
$gmane/290521

*Changes from V3
($gmane/292040)

- add missing static to curl_dump
- reorder the patch order
- tried to fix all (but i am not sure) the problems reported by Julio ($gmane/292055)
- * squash the documentation with the http.c commit.
  * in the trace prefix each line to indicate it is about sending a header, and drop the 
    initial hex count
  * auto-censor Authorization headers by default 

    as suggested by Jeff King ($gmane/292074)

*Changes from V2
($gmane/291868)

- fix garbage comment in http.c (i am very sorry for that)
- add final '.' to the commit message for imap-send.c and to other commit messages
- typofix double ; in http.c
- merge the nice cleanup and code refactoring of Ramsay Jones (Thank you very much !!)
- squash the previous commit 2/4

*Changes from V1

- introduced GIT_TRACE_CURL variable with its documentation
- changed the name of the temporary variable "i" in "w" in the helper routine
- used the c escape sequences instead of the hex equivalent
- dropped the previous GIT_DEBUG_CURL env var
- curl_dump and curl_trace factored out to a shared implementation
in http.c
Elia Pinto (2):
  http.c: implement the GIT_TRACE_CURL environment variable
  imap-send.c: introduce the GIT_TRACE_CURL enviroment variable

 Documentation/git.txt |   8 ++++
 http.c                | 109 +++++++++++++++++++++++++++++++++++++++++++++++++-
 http.h                |   4 ++
 imap-send.c           |   6 +++
 4 files changed, 126 insertions(+), 1 deletion(-)

-- 
2.8.1.487.gf8c3767.dirty

[PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Elia Pinto]

Implement the GIT_TRACE_CURL environment variable to allow a
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. Document the new GIT_TRACE_CURL
environment variable.

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>
---
 Documentation/git.txt |   8 ++++
 http.c                | 109 +++++++++++++++++++++++++++++++++++++++++++++++++-
 http.h                |   4 ++
 3 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 8afe349..958db0f 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1075,6 +1075,14 @@ of clones and fetches.
 	cloning of shallow repositories.
 	See 'GIT_TRACE' for available trace output options.
 
+'GIT_TRACE_CURL'::
+	Enables a curl full trace dump of all incoming and outgoing data,
+	including descriptive information, of the git transport protocol.
+	This is similar to doing curl --trace-ascii on the command line.
+	This option overrides setting the GIT_CURL_VERBOSE environment
+	variable.
+	See 'GIT_TRACE' for available trace output options.
+
 'GIT_LITERAL_PATHSPECS'::
 	Setting this variable to `1` will cause Git to treat all
 	pathspecs literally, rather than as glob patterns. For example,
diff --git a/http.c b/http.c
index 985b995..5c2c729 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
@@ -478,6 +479,108 @@ static void set_curl_keepalive(CURL *c)
 }
 #endif
 
+static void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex, char nopriv)
+{
+	size_t i;
+	struct strbuf out = STRBUF_INIT;
+	unsigned int width = 0x10;
+
+	/* without the hex output, we can fit more on screen */
+	if (nohex) width = 0x50;
+
+	strbuf_addf(&out, "%s, %10.10ld bytes (0x%8.8lx)\n",
+		text, (long)size, (long)size);
+	trace_strbuf(&trace_curl, &out);
+	strbuf_reset(&out);
+
+	for (i = 0; i < size; i += width) {
+		size_t w;
+		strbuf_addf(&out, "%s: ", text);
+		if (!nohex) {
+			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++) {
+			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] : '.');
+			if (nohex && (i + w + 2 < size)
+			    && ptr[i + w + 1] == '\r'
+			    && ptr[i + w + 2] == '\n') {
+				i += (w + 3 - width);
+				break;
+			}
+		}
+		/* if we are called with nopriv we skip the Authorization field if present
+		 * and print a blank line
+		*/
+		if ( nopriv && strstr(out.buf, "Authorization:"))
+			strbuf_reset(&out);
+
+		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)
+{
+	const char *text;
+	(void)handle;		/* prevent compiler unused parameter warning if checked */
+	(void)userp;		/* prevent compiler unused parameter warning if checked */
+	char nopriv = 0;	/* default: there are no sensitive data
+				 * in the trace to be skipped
+				*/
+
+	switch (type) {
+	case CURLINFO_TEXT:
+		trace_printf_key(&trace_curl, "== Info: %s", data);
+	default:		/* we ignore unknown types by default */
+		return 0;
+
+	case CURLINFO_HEADER_OUT:
+		text = "=> Send header";
+		nopriv = 1;
+		break;
+	case CURLINFO_DATA_OUT:
+		text = "=> Send data";
+		nopriv = 0;
+		break;
+	case CURLINFO_SSL_DATA_OUT:
+		text = "=> Send SSL data";
+		nopriv = 0;
+		break;
+	case CURLINFO_HEADER_IN:
+		text = "<= Recv header";
+		nopriv = 0;
+		break;
+	case CURLINFO_DATA_IN:
+		text = "<= Recv data";
+		nopriv = 0;
+		break;
+	case CURLINFO_SSL_DATA_IN:
+		text = "<= Recv SSL data";
+		nopriv = 0;
+		break;
+	}
+
+	curl_dump(text, (unsigned char *)data, size, 1, nopriv);
+	return 0;
+}
+
+
 static CURL *get_curl_handle(void)
 {
 	CURL *result = curl_easy_init();
@@ -577,7 +680,11 @@ static CURL *get_curl_handle(void)
 			"your curl version is too old (>= 7.19.4)");
 #endif
 
-	if (getenv("GIT_CURL_VERBOSE"))
+	if (curl_trace_want()) {
+		curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
+		curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
+		curl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);
+	} else if (getenv("GIT_CURL_VERBOSE"))
 		curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
 
 	curl_easy_setopt(result, CURLOPT_USERAGENT,
diff --git a/http.h b/http.h
index 36f558b..1ee80e5 100644
--- a/http.h
+++ b/http.h
@@ -225,4 +225,8 @@ extern int finish_http_object_request(struct http_object_request *freq);
 extern void abort_http_object_request(struct http_object_request *freq);
 extern void release_http_object_request(struct http_object_request *freq);
 
+/* Debug callback and helper routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */
+int curl_trace_want(void);
+int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);
+
 #endif /* HTTP_H */
-- 
2.8.1.487.gf8c3767.dirty

Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Jeff King]

On Thu, Apr 28, 2016 at 11:57:47AM +0000, Elia Pinto wrote:

> +static void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex, char nopriv)

We usually use "int" for our boolean flags. Space savings don't matter
outside of a struct (and if they did, you should be using a single flags
field), and this way the user does not have to guess whether the "char"
is significant.

It looks like we never pass anything but "1" for nohex. Can we drop this
parameter entirely? But see below...

> +{
> +	size_t i;
> +	struct strbuf out = STRBUF_INIT;
> +	unsigned int width = 0x10;
> +
> +	/* without the hex output, we can fit more on screen */
> +	if (nohex) width = 0x50;

Maybe it is just me, but I think this is more readable using decimal
constants. I mind it less in checking ASCII values like 0x20, but here I
think just saying "80" is more customary.

> +	for (i = 0; i < size; i += width) {
> +		size_t w;
> +		strbuf_addf(&out, "%s: ", text);

I really like this new format. Doing:

  GIT_TRACE_CURL=1 git ... 2>&1 | grep '=> Send header: '

is very readable.

However, I did run into an interesting case. The output looks like:

  10:24:04.540803 http.c:527              => Send header: Host: github.com
  10:24:04.540809 http.c:527              => Send header: x
  10:24:04.540811 http.c:527              => Send header: User-Agent: git/2.8.1.341.g2caf4c9.dirty

What's that weird "x" line?

It turns out that the line before it is:

  Authorization: Basic some-really-long-opaque-token-that-ends-in-x

Since we break at a newline _or_ at the width, that gets broken onto the
following line. The Authorization line hits the code below to suppress
the output.

So not only do I find the breaking of the line hard to read, but it
means we may leak data from the Authorization line that got broken into
the next chunk (here it was only one character, but with a sufficiently
long header, it could be real data).

So I think we probably want to _just_ break at newlines, however long
they are.

But that probably isn't a good idea for binary data. So I'd suggest that
sending/receiving headers break on newlines, and actual body data should
respect the width field (we may still have line-oriented data in the
body which would be easier to read without line-breaking, but if you are
debugging that you are better off with GIT_TRACE_PACKET anyway).

> +		 for (w = 0; (w < width) && (i + w < size); w++) {
> +			   if (nohex && (i + w + 1 < size) && ptr[i + w] == '\r'
> +				&& ptr[i + w + 1] == '\n') {
> +				    i += (w + 2 - width);
> +				    break;
> +			   }

This loop puzzled me for a bit. When we end early due to a newline, we
subtract out the width here. I guess that's to accomodate the "i +=
width" that the outer for-loop is going to do.

If you follow my suggestion above to split the code paths for
line-oriented and fixed-width data, then this all gets much simpler.

> +		/* if we are called with nopriv we skip the Authorization field if present
> +		 * and print a blank line
> +		*/
> +		if ( nopriv && strstr(out.buf, "Authorization:"))
> +			strbuf_reset(&out);

Style: multi-line comments should look like:

  /*
   * the comment
   * goes here
   */

and there should be no whitespace after the opening "(".

Removing the field entirely may be a bit confusing when you're
debugging. Instead, perhaps we can just redact the interesting bits,
like:

diff --git a/http.c b/http.c
index 8ab0adc..30e8858 100644
--- a/http.c
+++ b/http.c
@@ -481,7 +481,11 @@ static void curl_dump(const char *text, unsigned char *ptr, size_t size, char no
 
 	for (i = 0; i < size; i += width) {
 		size_t w;
+		size_t prefix_len;
+		const char *header;
+
 		strbuf_addf(&out, "%s: ", text);
+		prefix_len = out.len;
 		if (!nohex) {
 			for (w = 0; w < width; w++)
 				if (i + w < size)
@@ -507,8 +511,17 @@ static void curl_dump(const char *text, unsigned char *ptr, size_t size, char no
 		/* if we are called with nopriv we skip the Authorization field if present
 		 * and print a blank line
 		*/
-		if ( nopriv && strstr(out.buf, "Authorization:"))
-			strbuf_reset(&out);
+		if (nopriv &&
+		    skip_prefix(out.buf + prefix_len, "Authorization:", &header)) {
+			/* The first token is the type, which is OK to log */
+			while (isspace(*header))
+				header++;
+			while (*header && !isspace(*header))
+				header++;
+			/* Everything else is opaque and possibly sensitive */
+			strbuf_setlen(&out, header - out.buf);
+			strbuf_addstr(&out, " <redacted>");
+		}
 
 		strbuf_addch(&out, '\n');
 		trace_strbuf(&trace_curl, &out);

That tells the viewer that we did in fact send the header (which is
useful to know), and which type it used.

> +		strbuf_addch(&out, '\n');
> +		trace_strbuf(&trace_curl, &out);
> +		strbuf_release(&out);
> +	}
> +}

This is the only strbuf_release() in the function, and it's inside the
loop. Yet we use the strbuf to print the initial line (and do a reset()
after). So if the field we get is 0 bytes, we'll leak the strbuf memory
used by the initial line.

I don't know if that's possible with curl or not. But just in case, we
could structure the loop more like:

  ... output initial line ...
  for (i = 0; i < size; i += width) {
	strbuf_reset(&out);
	... output data line ...
  }
  strbuf_release(&out);

That has the added bonus that we do not have to reallocate for each
iteration of the loop (we just reset the length back to zero each time,
and then free the memory at the very end).

-Peff

Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +		 for (w = 0; (w < width) && (i + w < size); w++) {
>> +			   if (nohex && (i + w + 1 < size) && ptr[i + w] == '\r'
>> +				&& ptr[i + w + 1] == '\n') {
>> +				    i += (w + 2 - width);
>> +				    break;
>> +			   }
>
> This loop puzzled me for a bit. When we end early due to a newline, we
> subtract out the width here. I guess that's to accomodate the "i +=
> width" that the outer for-loop is going to do.

I think I essentially said the same thing on the previous round and
I thought I suggested to restructure the loop to primarily aim to
split at line-end (instead of the above which primarily aims to
split at width but line-end may cause a premature split).

Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Stefan Beller]

On Thu, Apr 28, 2016 at 4:57 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
> Implement the GIT_TRACE_CURL environment variable to allow a
> 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. Document the new GIT_TRACE_CURL
> environment variable.
>
> 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>
> ---
>  Documentation/git.txt |   8 ++++
>  http.c                | 109 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  http.h                |   4 ++
>  3 files changed, 120 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 8afe349..958db0f 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1075,6 +1075,14 @@ of clones and fetches.
>         cloning of shallow repositories.
>         See 'GIT_TRACE' for available trace output options.
>
> +'GIT_TRACE_CURL'::
> +       Enables a curl full trace dump of all incoming and outgoing data,
> +       including descriptive information, of the git transport protocol.
> +       This is similar to doing curl --trace-ascii on the command line.
> +       This option overrides setting the GIT_CURL_VERBOSE environment
> +       variable.

How does it overwrite the GIT_CURL_VERBOSE variable?
After a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things:

* apparently GIT_CURL_VERBOSE is used as a boolean,
  so I presume we assume True for GIT_CURL_VERBOSE, but
  extend it?
* GIT_CURL_VERBOSE is not documented at all. (It is mentioned in
  the release notes for 2.3.0, not sure if that counts as documentation)
  As you know the area, care to send a documentation patch for
  GIT_CURL_VERBOSE?

I am trying to understand how much more information I get by using
GIT_TRACE_CURL instead of GIT_CURL_VERBOSE.

GIT_TRACE_CURL follows the standard of GIT_TRACE_$subsystem, so I
guess that will be the encouraged way of debugging and GIT_CURL_VERBOSE
will not be encouraged to the user?

Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Jeff King]

On Thu, Apr 28, 2016 at 10:26:06AM -0700, Stefan Beller wrote:

> > +'GIT_TRACE_CURL'::
> > +       Enables a curl full trace dump of all incoming and outgoing data,
> > +       including descriptive information, of the git transport protocol.
> > +       This is similar to doing curl --trace-ascii on the command line.
> > +       This option overrides setting the GIT_CURL_VERBOSE environment
> > +       variable.
> 
> How does it overwrite the GIT_CURL_VERBOSE variable?

You can't use both, as they are both triggered using the CURLOPT_VERBOSE
option of curl. The main difference is that with GIT_CURL_VERBOSE, we
rely on curl to print the information to stderr. With GIT_CURL_TRACE, we
do the printing ourselves (so we can tweak the output format, send it to
places other than stderr, etc).

> After a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things:
> 
> * apparently GIT_CURL_VERBOSE is used as a boolean,
>   so I presume we assume True for GIT_CURL_VERBOSE, but
>   extend it?

It's not a boolean. If the variable exists at all, we turn on verbose
output (so I guess you can consider it a boolean, but we do not parse
its contents as boolean; GIT_CURL_VERBOSE=false does not do what you
might think).

> * GIT_CURL_VERBOSE is not documented at all. (It is mentioned in
>   the release notes for 2.3.0, not sure if that counts as documentation)
>   As you know the area, care to send a documentation patch for
>   GIT_CURL_VERBOSE?

I think there is no need for GIT_CURL_VERBOSE once we have
GIT_TRACE_CURL. The latter is more flexible and matches the GIT_TRACE_*
interface we use elsewhere.

So I think we should consider GIT_CURL_VERBOSE deprecated (though I do
not mind keeping it for old-timers since it is literally one line of
code).

-Peff

Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Stefan Beller]

On Thu, Apr 28, 2016 at 10:44 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 10:26:06AM -0700, Stefan Beller wrote:
>
>> > +'GIT_TRACE_CURL'::
>> > +       Enables a curl full trace dump of all incoming and outgoing data,
>> > +       including descriptive information, of the git transport protocol.
>> > +       This is similar to doing curl --trace-ascii on the command line.
>> > +       This option overrides setting the GIT_CURL_VERBOSE environment
>> > +       variable.
>>
>> How does it overwrite the GIT_CURL_VERBOSE variable?
>
> You can't use both, as they are both triggered using the CURLOPT_VERBOSE
> option of curl. The main difference is that with GIT_CURL_VERBOSE, we
> rely on curl to print the information to stderr. With GIT_CURL_TRACE, we
> do the printing ourselves (so we can tweak the output format, send it to
> places other than stderr, etc).

Well that's the information I'd rather find in the documentation
than in a mailing list archive ;)

>
>> After a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things:
>>
>> * apparently GIT_CURL_VERBOSE is used as a boolean,
>>   so I presume we assume True for GIT_CURL_VERBOSE, but
>>   extend it?
>
> It's not a boolean. If the variable exists at all, we turn on verbose
> output (so I guess you can consider it a boolean, but we do not parse
> its contents as boolean; GIT_CURL_VERBOSE=false does not do what you
> might think).
>
>> * GIT_CURL_VERBOSE is not documented at all. (It is mentioned in
>>   the release notes for 2.3.0, not sure if that counts as documentation)
>>   As you know the area, care to send a documentation patch for
>>   GIT_CURL_VERBOSE?
>
> I think there is no need for GIT_CURL_VERBOSE once we have
> GIT_TRACE_CURL. The latter is more flexible and matches the GIT_TRACE_*
> interface we use elsewhere.
>
> So I think we should consider GIT_CURL_VERBOSE deprecated (though I do
> not mind keeping it for old-timers since it is literally one line of
> code).

I see, so by this patch there is no need to document
GIT_CURL_VERBOSE any more?

>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Jeff King]

On Thu, Apr 28, 2016 at 10:48:38AM -0700, Stefan Beller wrote:

> >> How does it overwrite the GIT_CURL_VERBOSE variable?
> >
> > You can't use both, as they are both triggered using the CURLOPT_VERBOSE
> > option of curl. The main difference is that with GIT_CURL_VERBOSE, we
> > rely on curl to print the information to stderr. With GIT_CURL_TRACE, we
> > do the printing ourselves (so we can tweak the output format, send it to
> > places other than stderr, etc).
> 
> Well that's the information I'd rather find in the documentation
> than in a mailing list archive ;)

Sure, but I'm not sure what of part of that you want to put in the
documentation.  I was just explaining why the implementation constrains
us to overriding, and there really aren't any other sane options.

I don't think we want to get into defining the exact set of information,
which is not up to us anyway. We're just relaying whatever curl gives
us.

IMHO, we do not even need to mention GIT_CURL_VERBOSE here at all. That
is an undocumented interface that can hopefully just be forgotten over
time.

> > So I think we should consider GIT_CURL_VERBOSE deprecated (though I do
> > not mind keeping it for old-timers since it is literally one line of
> > code).
> 
> I see, so by this patch there is no need to document
> GIT_CURL_VERBOSE any more?

Exactly.

-Peff

[PATCHv4 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable

[Elia Pinto]

Permit the use of the GIT_TRACE_CURL environment variable calling
the curl_trace and curl_dump http.c helper routine.

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>
---
 imap-send.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/imap-send.c b/imap-send.c
index 938c691..61c6787 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1444,6 +1444,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
 		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
 
+	if (curl_trace_want()) {
+		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
+		curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
+		curl_easy_setopt(curl, CURLOPT_DEBUGDATA, NULL);
+	}
+
 	return curl;
 }
 
-- 
2.8.1.487.gf8c3767.dirty

Re: [PATCHv4 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable

[Jeff King]

On Thu, Apr 28, 2016 at 11:57:48AM +0000, Elia Pinto wrote:

> diff --git a/imap-send.c b/imap-send.c
> index 938c691..61c6787 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1444,6 +1444,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>  	if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
>  		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
>  
> +	if (curl_trace_want()) {
> +		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
> +		curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
> +		curl_easy_setopt(curl, CURLOPT_DEBUGDATA, NULL);
> +	}

In the only other caller of curl_trace_want(), we repeat these exact
same lines (and really, what else would one do with that flag besides
enable curl_trace?).

Perhaps a better abstraction would be:

  int setup_curl_trace(CURL *handle)
  {
	if (!trace_want(&trace_curl))
		return 0;
	curl_easy_setopt(handle, CURLOPT_VERBOSE, 1L);
	curl_easy_setopt(handle, CURLOPT_DEBUGFUNCTION, curl_trace);
	curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
	return 1;
  }

You could even get rid of the return value, too. We do not use it here,
but just let GIT_TRACE_CURL naturally override GIT_CURL_VERBOSE by
setting the DEBUGFUNCTION. In the other caller, we do:

  if (curl_trace_want()) {
     ... set up handle ...
  } else {
     ... check and setup up GIT_CURL_VERBOSE ...
  }

but we can do the else block regardless. It is a noop if tracing is set
up.

-Peff