Re: [PATCH v4 2/2] http-fetch: redact url on die() message
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Oct 28, 2021, 17:25 UTC
- Message-ID
- <CAPig+cTKSp28oUvESCWLB+OLBjbUSt3vhz6n3eVmkfYf9arcrg@mail.gmail.com>
- In-Reply-To
- <211028.86sfwlw10o.gmgdl@evledraar.gmail.com>
On Thu, Oct 28, 2021 at 12:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 12 quoted lines
> On Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote:
> > 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).
Show 8 quoted lines
> 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.
Show 11 quoted lines
> > + } 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.