On Thu, Oct 28, 2021 at 9:46 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > > On Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote: > > > From: Ivan Frade <ifrade@xxxxxxxxxx> > > ... > > + 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 > 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 > > + } 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