Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE

From: Jeff King <peff@peff.net>
Date: May 12, 2020, 19:16 UTC
Message-ID: <20200512191610.GB54565@coredump.intra.peff.net>
In-Reply-To: <1df9e9deb7831b32694ea453759bf5d21952e165.1589218693.git.jonathantanmy@google.com>

On Mon, May 11, 2020 at 10:43:10AM -0700, Jonathan Tan wrote:

Show 12 quoted lines

> Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if
> GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting
> CURLOPT_VERBOSE.
> 
> This is to prevent inadvertent revelation of sensitive data. In
> particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header
> nor any cookies specified by GIT_REDACT_COOKIES.
> 
> Unifying the tracing mechanism also has the future benefit that any
> improvements to the tracing mechanism will benefit both users of
> GIT_CURL_VERBOSE and GIT_TRACE_CURL, and we do not need to remember to
> implement any improvement twice.

Yeah, I think this is worth doing. The patch looks OK to me, though:

Show 5 quoted lines

> +void http_trace_curl_no_data(void)
> +{
> +	trace_override_envvar(&trace_curl, "1");
> +	trace_curl_data = 0;
> +}

Would:

  setenv("GIT_TRACE_CURL", "1", 0);
  setenv("GIT_TRACE_CURL_NO_DATA", "0", 0);

be simpler? Perhaps it makes the flow more convoluted as we'd go on to parse those variables, but it puts us on the same paths that we'd use if the user specified those things (and avoids the need for the special "override" function entirely).

Other than that nit, it seems very cleanly done.

-Peff

PS I sometimes find the normal trace a bit verbose, but I do still want
   to see data. Do others feel the same? Particularly I find the "SSL"
   lines totally worthless (I guess maybe you could be debugging ssl
   stuff, but that would be the exception, I'd think). Ditto the split
   of data into two lines: one with the size and one with the actual
   data.

   I dunno. I haven't been debugging any git-over-http stuff lately, so
   it hasn't been bothering me. But I definitely have written perl
   scripts to extract the data to a more readable format. Maybe it would
   be easier if it had a few more knobs.