[PATCHv2 0/4] Implements the GIT_TRACE_CURL environment variable

[PATCHv2 0/4] Implements the GIT_TRACE_CURL environment variable

[Elia Pinto]

This is the second version but in reality is the complete rewriting of the patches discussed here

$gmane/290520
$gmane/290521

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 (4):
  http.h: Add debug callback and helper routine for implementing     the
    GIT_TRACE_CURL environment variable in http.c
  http.c: implements the GIT_TRACE_CURL environment variable
  git.txt: document the new GIT_TRACE_CURL environment variable
  imap-send.c: introduce the GIT_TRACE_CURL enviroment variable

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

-- 
2.5.0

[PATCH 1/4] http.h: Add debug callback and helper routine for implementing the GIT_TRACE_CURL environment variable in http.c

[Elia Pinto]

Add the debug callback and helper routine prototype used by
curl_easy_setopt CURLOPT_DEBUGFUNCTION in http.c
for implementing the 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>
---
 http.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/http.h b/http.h
index 4ef4bbd..a2d10bc 100644
--- a/http.h
+++ b/http.h
@@ -224,4 +224,10 @@ 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 */
+static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
+int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);
+void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex);
+
+
 #endif /* HTTP_H */
-- 
2.5.0

[PATCH 2/4] http.c: implements the GIT_TRACE_CURL environment variable

[Elia Pinto]

Implements 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


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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 4304b80..278991e 100644
--- a/http.c
+++ b/http.c
@@ -11,6 +11,9 @@
 #include "gettext.h"
 #include "transport.h"
 
+/*
+tatic 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 +467,95 @@ static void set_curl_keepalive(CURL *c)
 }
 #endif
 
+
+void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
+{
+	size_t i;
+	size_t w;
+	struct strbuf out = STRBUF_INIT;;
+
+	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);
+
+	for (i = 0; i < size; i += width) {
+
+		strbuf_addf(&out, "%4.4lx: ", (long)i);
+
+		if (!nohex) {
+			/* hex not disabled, show it */
+			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] : '.');
+			/* 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;
+			}
+		}
+		strbuf_addch(&out, '\n');
+		trace_strbuf(&trace_curl, &out);
+		strbuf_release(&out);
+	}
+}
+
+int curl_trace(CURL *handle, curl_infotype type,
+	     char *data, size_t size, void *userp)
+{
+	const char *text;
+	(void)handle;		/* prevent compiler warning */
+
+	switch (type) {
+	case CURLINFO_TEXT:
+		trace_printf_key(&trace_curl, "== Info: %s", data);
+	default:		/* in case a new one is introduced to shock us */
+		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;
+}
+
+
 static CURL *get_curl_handle(void)
 {
 	CURL *result = curl_easy_init();
@@ -563,7 +655,11 @@ static CURL *get_curl_handle(void)
 			"your curl version is too old (>= 7.19.4)");
 #endif
 
-	if (getenv("GIT_CURL_VERBOSE"))
+	if (trace_want(&trace_curl)) {
+		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,
-- 
2.5.0

[PATCH 3/4] git.txt: document the new GIT_TRACE_CURL environment variable

[Elia Pinto]

Describe the purpose of the 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 ++++++++
 1 file changed, 8 insertions(+)

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

[PATCH 4/4] 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..b371a78 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 (trace_want(&trace_curl)) {
+		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.5.0

Re: [PATCH 1/4] http.h: Add debug callback and helper routine for implementing the GIT_TRACE_CURL environment variable in http.c

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> writes:

> Add the debug callback and helper routine prototype used by
> curl_easy_setopt CURLOPT_DEBUGFUNCTION in http.c
> for implementing the 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>
> ---
>  http.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/http.h b/http.h
> index 4ef4bbd..a2d10bc 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,4 +224,10 @@ 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 */
> +static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);

This will be multiply instanciated in every *.c file that includes http.h?

> +int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex);
> +
> +
>  #endif /* HTTP_H */

In any case, you'd want to combine this with 2/4 and write a single
log message for the combined change.

Re: [PATCH 2/4] http.c: implements the GIT_TRACE_CURL environment variable

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> writes:

> Implements 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
>
>
> 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 4304b80..278991e 100644
> --- a/http.c
> +++ b/http.c
> @@ -11,6 +11,9 @@
>  #include "gettext.h"
>  #include "transport.h"
>  
> +/*
> +tatic struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
> +*/

Is this a sign that this hasn't been proof-read before sending?

>  #if LIBCURL_VERSION_NUM >= 0x070a08
>  long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
>  #else
> @@ -464,6 +467,95 @@ static void set_curl_keepalive(CURL *c)
>  }
>  #endif
>  
> +
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
> +{
> +	size_t i;
> +	size_t w;
> +	struct strbuf out = STRBUF_INIT;;
> +
> +	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);
> +
> +	for (i = 0; i < size; i += width) {
> +
> +		strbuf_addf(&out, "%4.4lx: ", (long)i);
> +
> +		if (!nohex) {
> +			/* hex not disabled, show it */
> +			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] : '.');
> +			/* 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;
> +			}
> +		}
> +		strbuf_addch(&out, '\n');
> +		trace_strbuf(&trace_curl, &out);
> +		strbuf_release(&out);
> +	}
> +}
> +
> +int curl_trace(CURL *handle, curl_infotype type,
> +	     char *data, size_t size, void *userp)
> +{
> +	const char *text;
> +	(void)handle;		/* prevent compiler warning */
> +
> +	switch (type) {
> +	case CURLINFO_TEXT:
> +		trace_printf_key(&trace_curl, "== Info: %s", data);
> +	default:		/* in case a new one is introduced to shock us */
> +		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;
> +}
> +
> +
>  static CURL *get_curl_handle(void)
>  {
>  	CURL *result = curl_easy_init();
> @@ -563,7 +655,11 @@ static CURL *get_curl_handle(void)
>  			"your curl version is too old (>= 7.19.4)");
>  #endif
>  
> -	if (getenv("GIT_CURL_VERBOSE"))
> +	if (trace_want(&trace_curl)) {
> +		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,

Re: [PATCH 4/4] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> writes:

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

s/$/./; the patch itself is very concise and the "dump" thing in 3/4
looked sensible.

>
> 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..b371a78 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 (trace_want(&trace_curl)) {
> +		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
> +		curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
> +		curl_easy_setopt(curl, CURLOPT_DEBUGDATA, NULL);
> +	}
> +
>  	return curl;
>  }

Re: [PATCH 2/4] http.c: implements the GIT_TRACE_CURL environment variable

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> writes:

> Implements 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
>
>
> 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 4304b80..278991e 100644
> --- a/http.c
> +++ b/http.c
> @@ -11,6 +11,9 @@
>  #include "gettext.h"
>  #include "transport.h"
>  
> +/*
> +tatic 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 +467,95 @@ static void set_curl_keepalive(CURL *c)
>  }
>  #endif
>  
> +
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
> +{
> +	size_t i;
> +	size_t w;
> +	struct strbuf out = STRBUF_INIT;;

;; makes the next line decl-after-stmt.

> +
> +	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);
> +
> +	for (i = 0; i < size; i += width) {
> +
> +		strbuf_addf(&out, "%4.4lx: ", (long)i);
> +
> +		if (!nohex) {
> +			/* hex not disabled, show it */
> +			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] : '.');
> +			/* 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;
> +			}
> +		}
> +		strbuf_addch(&out, '\n');
> +		trace_strbuf(&trace_curl, &out);
> +		strbuf_release(&out);
> +	}
> +}
> +
> +int curl_trace(CURL *handle, curl_infotype type,
> +	     char *data, size_t size, void *userp)
> +{
> +	const char *text;
> +	(void)handle;		/* prevent compiler warning */
> +
> +	switch (type) {
> +	case CURLINFO_TEXT:
> +		trace_printf_key(&trace_curl, "== Info: %s", data);
> +	default:		/* in case a new one is introduced to shock us */
> +		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;
> +}
> +
> +
>  static CURL *get_curl_handle(void)
>  {
>  	CURL *result = curl_easy_init();
> @@ -563,7 +655,11 @@ static CURL *get_curl_handle(void)
>  			"your curl version is too old (>= 7.19.4)");
>  #endif
>  
> -	if (getenv("GIT_CURL_VERBOSE"))
> +	if (trace_want(&trace_curl)) {
> +		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,

Re: [PATCH 1/4] http.h: Add debug callback and helper routine for implementing the GIT_TRACE_CURL environment variable in http.c

[Ramsay Jones]

On 19/04/16 16:10, Elia Pinto wrote:
> Add the debug callback and helper routine prototype used by
> curl_easy_setopt CURLOPT_DEBUGFUNCTION in http.c
> for implementing the 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>
> ---
>  http.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/http.h b/http.h
> index 4ef4bbd..a2d10bc 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,4 +224,10 @@ 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 */
> +static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);

Ah no, this would add 6 instances of the 'trace_curl' key in http-fetch.c,
http-push.c, http-walker.c, http.c, imap-send.c and remote-curl.c. Hmm ...
since these would end up in different executables (by and large) it might
work OK, ... but is simply not necessary.

Also, patches #1 and #2 should be squashed into one patch and, since the
curl_dump() function is only called from http.c, it can be a static symbol.

I think the minimal fixup (including Junio's comment on patch #2, which also
triggered for me) is given in the patch below.

Hope that helps.

ATB,
Ramsay Jones

> +int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex);
> +
> +
>  #endif /* HTTP_H */

-- >8 --
Subject: [PATCH] curl-trace: fix scope/visibility of various symbols

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 http.c | 9 +++------
 http.h | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/http.c b/http.c
index 64dd975..ce91421 100644
--- a/http.c
+++ b/http.c
@@ -11,9 +11,7 @@
 #include "gettext.h"
 #include "transport.h"
 
-/*
-tatic struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
-*/
+struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
 #if LIBCURL_VERSION_NUM >= 0x070a08
 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
 #else
@@ -468,12 +466,11 @@ static void set_curl_keepalive(CURL *c)
 #endif
 
 
-void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
+static void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
 {
 	size_t i;
 	size_t w;
-	struct strbuf out = STRBUF_INIT;;
-
+	struct strbuf out = STRBUF_INIT;
 	unsigned int width = 0x10;
 
 	if (nohex)
diff --git a/http.h b/http.h
index a2d10bc..00e4ad7 100644
--- a/http.h
+++ b/http.h
@@ -225,9 +225,8 @@ 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 */
-static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
+extern struct trace_key trace_curl;
 int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);
-void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex);
 
 
 #endif /* HTTP_H */
-- 
2.8.0

Re: [PATCH 1/4] http.h: Add debug callback and helper routine for implementing the GIT_TRACE_CURL environment variable in http.c

[Ramsay Jones]

On 19/04/16 22:48, Ramsay Jones wrote:
> 
[snip]

> I think the minimal fixup (including Junio's comment on patch #2, which also
> triggered for me) is given in the patch below.

BTW, if you want to have a single static instance of the 'struct trace_key',
then the following patch on top should work. (Also, I have compiled and run
the testsuite on these patches, but _not_ tested that the trace functionality
actually works!) ;-)

HTH

ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] curl-trace: reduce visibility of the trace key struct

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 http.c      | 9 +++++++--
 http.h      | 2 +-
 imap-send.c | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index ce91421..eed6dba 100644
--- a/http.c
+++ b/http.c
@@ -11,7 +11,7 @@
 #include "gettext.h"
 #include "transport.h"
 
-struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
+static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
 #if LIBCURL_VERSION_NUM >= 0x070a08
 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
 #else
@@ -516,6 +516,11 @@ static void curl_dump(const char *text, unsigned char *ptr, size_t size, char no
 	}
 }
 
+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)
 {
@@ -652,7 +657,7 @@ static CURL *get_curl_handle(void)
 			"your curl version is too old (>= 7.19.4)");
 #endif
 
-	if (trace_want(&trace_curl)) {
+	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);
diff --git a/http.h b/http.h
index 00e4ad7..153ff17 100644
--- a/http.h
+++ b/http.h
@@ -225,7 +225,7 @@ 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 */
-extern struct trace_key trace_curl;
+int curl_trace_want(void);
 int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);
 
 
diff --git a/imap-send.c b/imap-send.c
index 36ff843..627ffa4 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1444,7 +1444,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
 		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
 
-	if (trace_want(&trace_curl)) {
+	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);
-- 
2.8.0