From: Jeff King <peff@peff.net>
Date: Thu, 28 Apr 2016 14:55:24 GMT
Subject: Re: [PATCHv4 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable
Message-ID: <20160428145524.GE25364@sigill.intra.peff.net>
In-Reply-To: <20160428115748.37177-3-gitter.spiros@gmail.com>

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