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

From: Ivan Frade <ifrade@google.com>
Date: Oct 28, 2021, 22:41 UTC
Message-ID: <CANQMx9WPt93e3pNghtG01L4GqGvPgGeRNH4RCedMTV=Vcu8vWQ@mail.gmail.com>
In-Reply-To: <211028.86sfwlw10o.gmgdl@evledraar.gmail.com>

On Thu, Oct 28, 2021 at 9:46 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 6 quoted lines

>
>
> On Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote:
>
> > From: Ivan Frade <ifrade@google.com>
> >

...

Show 6 quoted lines

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

Done

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

Done

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

Indeed, "%.*s" did the trick. Thanks!

Ivan