threads / patch / 41898

patch, 2 partshttp.c: implements the GIT_CURL_DEBUG environment variable

Subject: [PATCH 2/2] http.c: implements the GIT_CURL_DEBUG environment variable

## tl;dr

14 messages between Apr 1, 2016 and Apr 6, 2016. Diffs are folded; open one to read it.

replies: 13people: 6as markdown or json

Elia Pinto· Apr 1, 2016, 10:44 UTC · lore

[PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

Implements the GIT_CURL_DEBUG 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.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 imap-send.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 97 insertions(+), 2 deletions(-)
Show changes to imap-send.c +97 −2
diff --git a/imap-send.c b/imap-send.c
index 4d3b773..cf79e7f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
+
+static
+void curl_dump(const char *text,
+	  FILE * stream, unsigned char *ptr, size_t size, char nohex)
+{
+	size_t i;
+	size_t c;
+
+	unsigned int width = 0x10;
+
+	if (nohex)
+		/* without the hex output, we can fit more on screen */
+		width = 0x40;
+
+	fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
+		text, (long)size, (long)size);
+
+	for (i = 0; i < size; i += width) {
+
+		fprintf(stream, "%4.4lx: ", (long)i);
+
+		if (!nohex) {
+			/* hex not disabled, show it */
+			for (c = 0; c < width; c++)
+				if (i + c < size)
+					fprintf(stream, "%02x ", ptr[i + c]);
+				else
+					fputs("   ", stream);
+		}
+
+		for (c = 0; (c < width) && (i + c < size); c++) {
+			/* check for 0D0A; if found, skip past and start a new line of output */
+			if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D
+			    && ptr[i + c + 1] == 0x0A) {
+				i += (c + 2 - width);
+				break;
+			}
+			fprintf(stream, "%c",
+				(ptr[i + c] >= 0x20)
+				&& (ptr[i + c] < 0x80) ? ptr[i + c] : '.');
+			/* check again for 0D0A, to avoid an extra \n if it's at width */
+			if (nohex && (i + c + 2 < size)
+			    && ptr[i + c + 1] == 0x0D
+			    && ptr[i + c + 2] == 0x0A) {
+				i += (c + 3 - width);
+				break;
+			}
+		}
+		fputc('\n', stream);	/* newline */
+	}
+	fflush(stream);
+}
+
+static
+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:
+		fprintf(stderr, "== 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, stderr, (unsigned char *)data, size, 1);
+	return 0;
+}
+
 static CURL *setup_curl(struct imap_server_conf *srvc)
 {
 	CURL *curl;
@@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 
 	curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
 
-	if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
-		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
+	if (0 < verbosity )
+		if (getenv("GIT_CURL_DEBUG")) {
+			curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
+			curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
+		} else if (getenv("GIT_CURL_VERBOSE"))
+			curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
+
 
 	return curl;
 }
-- 
2.7.0.416.gbf6b42c.dirty
Elia Pinto· Apr 1, 2016, 10:44 UTC · re: Elia Pinto · lore

Implements the GIT_CURL_DEBUG 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.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 http.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)
Show changes to http.c +96 −1
diff --git a/http.c b/http.c
index dfc53c1..079779d 100644
--- a/http.c
+++ b/http.c
@@ -437,6 +437,97 @@ static void set_curl_keepalive(CURL *c)
 }
 #endif
 
+
+static
+void curl_dump(const char *text,
+	  FILE * stream, unsigned char *ptr, size_t size, char nohex)
+{
+	size_t i;
+	size_t c;
+
+	unsigned int width = 0x10;
+
+	if (nohex)
+		/* without the hex output, we can fit more on screen */
+		width = 0x40;
+
+	fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
+		text, (long)size, (long)size);
+
+	for (i = 0; i < size; i += width) {
+
+		fprintf(stream, "%4.4lx: ", (long)i);
+
+		if (!nohex) {
+			/* hex not disabled, show it */
+			for (c = 0; c < width; c++)
+				if (i + c < size)
+					fprintf(stream, "%02x ", ptr[i + c]);
+				else
+					fputs("   ", stream);
+		}
+
+		for (c = 0; (c < width) && (i + c < size); c++) {
+			/* check for 0D0A; if found, skip past and start a new line of output */
+			if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D
+			    && ptr[i + c + 1] == 0x0A) {
+				i += (c + 2 - width);
+				break;
+			}
+			fprintf(stream, "%c",
+				(ptr[i + c] >= 0x20)
+				&& (ptr[i + c] < 0x80) ? ptr[i + c] : '.');
+			/* check again for 0D0A, to avoid an extra \n if it's at width */
+			if (nohex && (i + c + 2 < size)
+			    && ptr[i + c + 1] == 0x0D
+			    && ptr[i + c + 2] == 0x0A) {
+				i += (c + 3 - width);
+				break;
+			}
+		}
+		fputc('\n', stream);	/* newline */
+	}
+	fflush(stream);
+}
+
+static
+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:
+		fprintf(stderr, "== 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, stderr, (unsigned char *)data, size, 1);
+	return 0;
+}
+
+
 static CURL *get_curl_handle(void)
 {
 	CURL *result = curl_easy_init();
@@ -532,7 +623,11 @@ static CURL *get_curl_handle(void)
 			"your curl version is too old (>= 7.19.4)");
 #endif
 
-	if (getenv("GIT_CURL_VERBOSE"))
+	if (getenv("GIT_CURL_DEBUG")) {
+		curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
+		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.7.0.416.gbf6b42c.dirty
Ramsay Jones· Apr 1, 2016, 15:03 UTC · re: Elia Pinto · lore

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

On 01/04/16 11:44, Elia Pinto wrote:
Show 15 quoted lines
> Implements the GIT_CURL_DEBUG 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.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  http.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/http.c b/http.c
> index dfc53c1..079779d 100644
> --- a/http.c
> +++ b/http.c
[snip]
Show 14 quoted lines
> @@ -532,7 +623,11 @@ static CURL *get_curl_handle(void)
>  			"your curl version is too old (>= 7.19.4)");
>  #endif
>  
> -	if (getenv("GIT_CURL_VERBOSE"))
> +	if (getenv("GIT_CURL_DEBUG")) {
> +		curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
> +		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,
> 
Again, maybe something like:
if (getenv("GIT_CURL_VERBOSE")) {
	curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
	if (getenv("GIT_CURL_DEBUG"))
		curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
}

Although that does make GIT_CURL_DEBUG subordinate to GIT_CURL_VERBOSE. So, that may not be desired ...

ATB, Ramsay Jones

Elia Pinto· Apr 4, 2016, 12:41 UTC · re: Ramsay Jones · lore

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

2016-04-01 17:03 GMT+02:00 Ramsay Jones <ramsay@ramsayjones.plus.com>:
Show 45 quoted lines
>
>
> On 01/04/16 11:44, Elia Pinto wrote:
>> Implements the GIT_CURL_DEBUG 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.
>>
>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>> ---
>>  http.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 96 insertions(+), 1 deletion(-)
>>
>> diff --git a/http.c b/http.c
>> index dfc53c1..079779d 100644
>> --- a/http.c
>> +++ b/http.c
> [snip]
>
>> @@ -532,7 +623,11 @@ static CURL *get_curl_handle(void)
>>                       "your curl version is too old (>= 7.19.4)");
>>  #endif
>>
>> -     if (getenv("GIT_CURL_VERBOSE"))
>> +     if (getenv("GIT_CURL_DEBUG")) {
>> +             curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
>> +             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,
>>
>
> Again, maybe something like:
>
> if (getenv("GIT_CURL_VERBOSE")) {
>         curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
>         if (getenv("GIT_CURL_DEBUG"))
>                 curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
> }
>
> Although that does make GIT_CURL_DEBUG subordinate to GIT_CURL_VERBOSE.
> So, that may not be desired ...

Thank you. But actually it is not a desirable change, for me almost, I prefer that the two definitions are independent. And it is true the opposite: if it is defined the curl DEBUG flag then it is implicitly defined the curl VERBOSE flag, because it is a prerequisite of the DEBUG functionality.

Thanks in any case for the review.
Best
Show 5 quoted lines
>
> ATB,
> Ramsay Jones
>
>
Torsten Bögershausen· Apr 1, 2016, 11:44 UTC · re: Elia Pinto · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

On 01.04.16 12:44, Elia Pinto wrote:
Show 23 quoted lines
> Implements the GIT_CURL_DEBUG 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.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  imap-send.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 97 insertions(+), 2 deletions(-)
> 
> diff --git a/imap-send.c b/imap-send.c
> index 4d3b773..cf79e7f 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
>  }
>  
>  #ifdef USE_CURL_FOR_IMAP_SEND
> +
> +static
> +void curl_dump(const char *text,
> +	  FILE * stream, unsigned char *ptr, size_t size, char nohex)
Style: FILE *stream
> +{
> +	size_t i;
> +	size_t c;

c is typically a character. 'j' may be better, unless you find a longer, and more descriptive name.

Show 25 quoted lines
> +
> +	unsigned int width = 0x10;
> +
> +	if (nohex)
> +		/* without the hex output, we can fit more on screen */
> +		width = 0x40;
> +
> +	fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
> +		text, (long)size, (long)size);
> +
> +	for (i = 0; i < size; i += width) {
> +
> +		fprintf(stream, "%4.4lx: ", (long)i);
> +
> +		if (!nohex) {
> +			/* hex not disabled, show it */
> +			for (c = 0; c < width; c++)
> +				if (i + c < size)
> +					fprintf(stream, "%02x ", ptr[i + c]);
> +				else
> +					fputs("   ", stream);
> +		}
> +
> +		for (c = 0; (c < width) && (i + c < size); c++) {
> +			/* check for 0D0A; if found, skip past and start a new line of output */

Should we write '\r' and '\n' instead for hex codes ? and CRLF instead of 0D0A ? (Yes the RFC use hexcodes, but we write readable C-code)

Ramsay Jones· Apr 1, 2016, 14:56 UTC · re: Elia Pinto · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

On 01/04/16 11:44, Elia Pinto wrote:
Show 15 quoted lines
> Implements the GIT_CURL_DEBUG 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.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  imap-send.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 97 insertions(+), 2 deletions(-)
> 
> diff --git a/imap-send.c b/imap-send.c
> index 4d3b773..cf79e7f 100644
> --- a/imap-send.c
> +++ b/imap-send.c
[snip]
Show 7 quoted lines
> @@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>  
>  	curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
>  
> -	if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
> -		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
> +	if (0 < verbosity )

previously it was sufficient to set GIT_CURL_VERBOSE, now I have to set verbosity too?

[Does it matter that you change "1L" to "1" in the curl_easy_setopt() call? In http.c (line 567) it also uses "1", but ...]

Show 10 quoted lines
> +		if (getenv("GIT_CURL_DEBUG")) {
> +			curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
> +			curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
> +		} else if (getenv("GIT_CURL_VERBOSE"))
> +			curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
> +
>  
>  	return curl;
>  }
> 
I would have expected something like:
if (0 < verbosity || getenv("GIT_CURL_VERBOSE")) {
	curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
	if (getenv("GIT_CURL_DEBUG"))
		curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
}
Hope That Helps.

ATB, Ramsay Jones

Elia Pinto· Apr 4, 2016, 12:45 UTC · re: Ramsay Jones · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

2016-04-01 16:56 GMT+02:00 Ramsay Jones <ramsay@ramsayjones.plus.com>:
Show 27 quoted lines
>
>
> On 01/04/16 11:44, Elia Pinto wrote:
>> Implements the GIT_CURL_DEBUG 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.
>>
>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>> ---
>>  imap-send.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 97 insertions(+), 2 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 4d3b773..cf79e7f 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
> [snip]
>
>> @@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>>
>>       curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
>>
>> -     if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
>> -             curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
>> +     if (0 < verbosity )

It was already so in the previous code, I have not changed it. If it is a desirable change it would take another patch

>
> previously it was sufficient to set GIT_CURL_VERBOSE, now I have to
> set verbosity too?
Show 10 quoted lines
>
> [Does it matter that you change "1L" to "1" in the curl_easy_setopt()
> call? In http.c (line 567) it also uses "1", but ...]
>
>> +             if (getenv("GIT_CURL_DEBUG")) {
>> +                     curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
>> +                     curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
>> +             } else if (getenv("GIT_CURL_VERBOSE"))
>> +                     curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
>> +
OK.
Show 13 quoted lines
>>
>>       return curl;
>>  }
>>
>
> I would have expected something like:
>
> if (0 < verbosity || getenv("GIT_CURL_VERBOSE")) {
>         curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
>         if (getenv("GIT_CURL_DEBUG"))
>                 curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
> }
>
No. Thank you. But actually it is not a desirable change
Show 7 quoted lines
> Hope That Helps.
>
> ATB,
> Ramsay Jones
>
>
>
Junio C Hamano· Apr 1, 2016, 15:35 UTC · re: Elia Pinto · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

Elia Pinto <gitter.spiros@gmail.com> writes:
Show 5 quoted lines
> Implements the GIT_CURL_DEBUG 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.

My impression is that using GIT_TRACE_* is the more mainstream trend, and it may be beneficial to work any new debugging aid like this one to fit within that mechanism.

I am not saying new GIT_*_DEBUG is wrong. I just wanted to make sure you have considered doing this as a new trace in GIT_TRACE_* family and rejected that apporach with a very good reason, in which case that rationale deserves to be in the log message.

Elia Pinto· Apr 4, 2016, 16:08 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

2016-04-01 17:35 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 11 quoted lines
> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> Implements the GIT_CURL_DEBUG 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.
>
> My impression is that using GIT_TRACE_* is the more mainstream
> trend, and it may be beneficial to work any new debugging aid like
> this one to fit within that mechanism.
I thought about it, and I agree with you. The idea could be
- Call the variable GIT_TRACE_CURL_DEBUG instead
- Add the new GIT_TRACE_CURL_VERBOSE variable, keeping
GIT_CURL_VERBOSE for compatibility
- Documenting these GIT_TRACE_CURL_XXX variables (GIT_CURL_VERBOSE
it is not even documented i think)
- perhaps use the git trace api in doing these new patches
Look reasonable? It seems reasonable? I'd like your own opinion
Thank you for your suggestion
Show 5 quoted lines
>
> I am not saying new GIT_*_DEBUG is wrong.  I just wanted to make
> sure you have considered doing this as a new trace in GIT_TRACE_*
> family and rejected that apporach with a very good reason, in
> which case that rationale deserves to be in the log message.
Junio C Hamano· Apr 4, 2016, 16:50 UTC · re: Elia Pinto · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

Elia Pinto <gitter.spiros@gmail.com> writes:
Show 7 quoted lines
>> My impression is that using GIT_TRACE_* is the more mainstream
>> trend, and it may be beneficial to work any new debugging aid like
>> this one to fit within that mechanism.
>
> I thought about it, and I agree with you. The idea could be
>
> - Call the variable GIT_TRACE_CURL_DEBUG instead

I think GIT_TRACE_CURL_DEBUG is overly verbose; tracing by definition is a debugging aid.

> - Add the new GIT_TRACE_CURL_VERBOSE variable, keeping
> GIT_CURL_VERBOSE for compatibility
I do not care too deeply either way.
 - GIT_CURL_VERBOSE can stay the same as-is and show its output to
   whatever output channel it spits things out.
 - Or it can be a synonym for GIT_TRACE_CURL=2 (as I understand that
   the VERBOSE output goes to the standard error stream)

If you want tracing as debugging aid and existing CURL_VERBOSE orthogonal, it would probably make more sense to go the former route, not linking this new "DEBUG" thing with the existing "VERBOSE" thing.

> - Documenting these GIT_TRACE_CURL_XXX variables (GIT_CURL_VERBOSE
> it is not even documented i think)
If we decide to leave them untangled, this is not necessary.
> - perhaps use the git trace api in doing these new patches
>
> Look reasonable? It seems reasonable? I'd like your own opinion
Not really sensible as long as you have that "perhaps" in the list.

Something that does not use the trace API shouldn't pretend to by using GIT_TRACE_* names.

GIT_TRACE_CURL could be your new thing and would decide where to show its output by using the GIT_TRACE_* api.

Eric Sunshine· Apr 1, 2016, 20:25 UTC · re: Elia Pinto · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

On Fri, Apr 1, 2016 at 6:44 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
Show 5 quoted lines
> Implements the GIT_CURL_DEBUG 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.

In addition to review comments by others, why are the new curl_dump() and curl_trace() functions duplicated in both patches rather than factored out to a shared implementation?

Show 96 quoted lines
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> diff --git a/imap-send.c b/imap-send.c
> @@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
>  }
>
>  #ifdef USE_CURL_FOR_IMAP_SEND
> +
> +static
> +void curl_dump(const char *text,
> +         FILE * stream, unsigned char *ptr, size_t size, char nohex)
> +{
> +       size_t i;
> +       size_t c;
> +
> +       unsigned int width = 0x10;
> +
> +       if (nohex)
> +               /* without the hex output, we can fit more on screen */
> +               width = 0x40;
> +
> +       fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
> +               text, (long)size, (long)size);
> +
> +       for (i = 0; i < size; i += width) {
> +
> +               fprintf(stream, "%4.4lx: ", (long)i);
> +
> +               if (!nohex) {
> +                       /* hex not disabled, show it */
> +                       for (c = 0; c < width; c++)
> +                               if (i + c < size)
> +                                       fprintf(stream, "%02x ", ptr[i + c]);
> +                               else
> +                                       fputs("   ", stream);
> +               }
> +
> +               for (c = 0; (c < width) && (i + c < size); c++) {
> +                       /* check for 0D0A; if found, skip past and start a new line of output */
> +                       if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D
> +                           && ptr[i + c + 1] == 0x0A) {
> +                               i += (c + 2 - width);
> +                               break;
> +                       }
> +                       fprintf(stream, "%c",
> +                               (ptr[i + c] >= 0x20)
> +                               && (ptr[i + c] < 0x80) ? ptr[i + c] : '.');
> +                       /* check again for 0D0A, to avoid an extra \n if it's at width */
> +                       if (nohex && (i + c + 2 < size)
> +                           && ptr[i + c + 1] == 0x0D
> +                           && ptr[i + c + 2] == 0x0A) {
> +                               i += (c + 3 - width);
> +                               break;
> +                       }
> +               }
> +               fputc('\n', stream);    /* newline */
> +       }
> +       fflush(stream);
> +}
> +
> +static
> +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:
> +               fprintf(stderr, "== 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, stderr, (unsigned char *)data, size, 1);
> +       return 0;
> +}
Elia Pinto· Apr 5, 2016, 10:21 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

2016-04-01 22:25 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
Show 10 quoted lines
> On Fri, Apr 1, 2016 at 6:44 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
>> Implements the GIT_CURL_DEBUG 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.
>
> In addition to review comments by others, why are the new curl_dump()
> and curl_trace() functions duplicated in both patches rather than
> factored out to a shared implementation?

It's right. Do you think i can use some existing file or should I create a new object file ?

Thank you very much
Show 97 quoted lines
>
>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>> ---
>> diff --git a/imap-send.c b/imap-send.c
>> @@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
>>  }
>>
>>  #ifdef USE_CURL_FOR_IMAP_SEND
>> +
>> +static
>> +void curl_dump(const char *text,
>> +         FILE * stream, unsigned char *ptr, size_t size, char nohex)
>> +{
>> +       size_t i;
>> +       size_t c;
>> +
>> +       unsigned int width = 0x10;
>> +
>> +       if (nohex)
>> +               /* without the hex output, we can fit more on screen */
>> +               width = 0x40;
>> +
>> +       fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
>> +               text, (long)size, (long)size);
>> +
>> +       for (i = 0; i < size; i += width) {
>> +
>> +               fprintf(stream, "%4.4lx: ", (long)i);
>> +
>> +               if (!nohex) {
>> +                       /* hex not disabled, show it */
>> +                       for (c = 0; c < width; c++)
>> +                               if (i + c < size)
>> +                                       fprintf(stream, "%02x ", ptr[i + c]);
>> +                               else
>> +                                       fputs("   ", stream);
>> +               }
>> +
>> +               for (c = 0; (c < width) && (i + c < size); c++) {
>> +                       /* check for 0D0A; if found, skip past and start a new line of output */
>> +                       if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D
>> +                           && ptr[i + c + 1] == 0x0A) {
>> +                               i += (c + 2 - width);
>> +                               break;
>> +                       }
>> +                       fprintf(stream, "%c",
>> +                               (ptr[i + c] >= 0x20)
>> +                               && (ptr[i + c] < 0x80) ? ptr[i + c] : '.');
>> +                       /* check again for 0D0A, to avoid an extra \n if it's at width */
>> +                       if (nohex && (i + c + 2 < size)
>> +                           && ptr[i + c + 1] == 0x0D
>> +                           && ptr[i + c + 2] == 0x0A) {
>> +                               i += (c + 3 - width);
>> +                               break;
>> +                       }
>> +               }
>> +               fputc('\n', stream);    /* newline */
>> +       }
>> +       fflush(stream);
>> +}
>> +
>> +static
>> +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:
>> +               fprintf(stderr, "== 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, stderr, (unsigned char *)data, size, 1);
>> +       return 0;
>> +}
Eric Sunshine· Apr 6, 2016, 05:53 UTC · re: Elia Pinto · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

On Tue, Apr 5, 2016 at 6:21 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
Show 7 quoted lines
> 2016-04-01 22:25 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
>> In addition to review comments by others, why are the new curl_dump()
>> and curl_trace() functions duplicated in both patches rather than
>> factored out to a shared implementation?
>
> It's right. Do you think i can use some existing file or should I
> create a new object file ?

Peff or Junio would be more qualified to answer, but perhaps the shared implementation could go in http.c?

Jeff King· Apr 6, 2016, 06:16 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

On Wed, Apr 06, 2016 at 01:53:35AM -0400, Eric Sunshine wrote:
Show 11 quoted lines
> On Tue, Apr 5, 2016 at 6:21 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
> > 2016-04-01 22:25 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
> >> In addition to review comments by others, why are the new curl_dump()
> >> and curl_trace() functions duplicated in both patches rather than
> >> factored out to a shared implementation?
> >
> > It's right. Do you think i can use some existing file or should I
> > create a new object file ?
> 
> Peff or Junio would be more qualified to answer, but perhaps the
> shared implementation could go in http.c?

Yeah. If it is linking with curl it must be in one of the objects that gets compiled only when NO_CURL is not set. Which is currently http.c, http-walker.c, and remote-curl.c. Of the three, the only one that makes sense is http.c.

A new file would be OK (provided it gets added to the OBJECTS line in Makefile:1921) but I think it fits well in http.c, which is essentially our bucket of curl-related wrappers.

-Peff

← back to recent threads