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

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

[Elia Pinto]

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)
+{
+	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

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

[Elia Pinto]

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
@@ -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

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

[Torsten Bögershausen]

On 01.04.16 12: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
> @@ -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.

> +
> +	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)

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

[Ramsay Jones]

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 )

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 ...]

> +		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

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

[Ramsay Jones]

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 ...

ATB,
Ramsay Jones

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

[Junio C Hamano]

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 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.

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

[Eric Sunshine]

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?

> 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;
> +}

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

[Elia Pinto]

2016-04-01 17:03 GMT+02:00 Ramsay Jones <ramsay@ramsayjones.plus.com>:
>
>
> 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
>
> ATB,
> Ramsay Jones
>
>

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

[Elia Pinto]

2016-04-01 16:56 GMT+02:00 Ramsay Jones <ramsay@ramsayjones.plus.com>:
>
>
> 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?

>
> [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.
>>
>>       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
> Hope That Helps.
>
> ATB,
> Ramsay Jones
>
>
>

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

[Elia Pinto]

2016-04-01 17:35 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> 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
>
> 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.

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

[Junio C Hamano]

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

>> 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.

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

[Elia Pinto]

2016-04-01 22:25 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
> 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
>
>> 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;
>> +}

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

[Eric Sunshine]

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?

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

[Jeff King]

On Wed, Apr 06, 2016 at 01:53:35AM -0400, Eric Sunshine wrote:

> 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