Re: [PATCH v4 2/2] http-fetch: redact url on die() message

On Thu, Oct 28, 2021 at 12:46 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote:

quoted

              if (results.curl_result != CURLE_OK) {
-                     die("Unable to get pack file %s\n%s", preq->url,
-                         curl_errorstr);
+                     struct url_info url;
+                     char *nurl = url_normalize(preq->url, &url);
+                     if (!git_env_bool("GIT_TRACE_REDACT", 1) || !nurl) {
+                             die("Unable to get pack file %s\n%s", preq->url,
+                                 curl_errorstr);

small nit: arrange if's from "if (cheap || expensive)", i.e. no need for
getenv() if !nurl, but maybe compilers are smart enough for that...

I had the same passing thought when glancing over this code (although
this appears to be an error patch, thus not performance critical, so
not terribly important).

nit: die() messages should start with lower-case (in CodingGuidelines), and I think it's better to quote both, so:

    die("unable to get pack '%s': '%s'", ...)

Or maybe without the second '%s', as in 3e8084f1884 (http: check
CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors, 2021-09-24) (which
I authored, but just copy/pasted the convention in the surrounding
code)>

Note that this is not a new die() call; it just got indented as-is by
this patch, so the changes you suggest to the message string are
potentially outside the scope of this patch. Possibilities: (1) make
the changes in this patch but mention them in the commit message; (2)
make the changes in a preparatory patch; (3) punt on the changes for
now.

quoted

+                     } else {
+                             char *schema = xstrndup(url.url, url.scheme_len);
+                             char *host = xstrndup(&url.url[url.host_off], url.host_len);
+                             die("failed to get '%s' url from '%s' "
+                                 "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
+                                 schema, host, curl_errorstr);

Hrm, I haven't tested, but aren't both of those xstrndup's redundant to
using %*s instead of %s for the printf format? I.e.:

    die("failed to get '%*s'[...]", url.schema_len, url.url, )

I wondered the same when reading the patch. Thanks for mentioning it.