On Fri, Oct 8, 2021 at 12:42 PM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
>
> On Fri, Oct 08 2021, Ivan Frade via GitGitGadget wrote:
>
> > diff --git a/http-fetch.c b/http-fetch.c
> > index fa642462a9e..d35e33e4f65 100644
> > --- a/http-fetch.c
> > +++ b/http-fetch.c
> > @@ -63,7 +63,9 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
> > if (start_active_slot(preq->slot)) {
> > run_active_slot(preq->slot);
> > if (results.curl_result != CURLE_OK) {
> > - die("Unable to get pack file %s\n%s", preq->url,
> > + int showUrl = git_env_bool("GIT_TRACE_REDACT", 1);
> > + die("Unable to get offloaded pack file %s\n%s",
> > + showUrl ? preq->url : "<redacted>",
> > curl_errorstr);
> > }
> > } else {
>
> Your CL and commit message just talk about traes, but this is a die()
> message.
>
> Perhaps it makes sense to redact it there too for some reason, but that
> seems to be a thing to separately argue for.
>
> This message is shown interactively to users, and I could see it be
> annoying to not have the URL that failed in your terminal output, even
> if it has some one-time token.
For a regular user the URL could be confusing (should they click on
it? try to download it by themselves?). I also got a suggestion to
print e.g. only the domain and maybe the packname.
In any case, I agree it is a different thing than trace logging. I
removed it from this patch.
>
> > +
> > + grep -A1 "clone<\ ..packfile-uris" log | grep "clone<\ <redacted>"
>
> We don't rely on GNU options like those for the test suite, it'll break
> on various supported platformrs.
>
> In this case the whole LHS of the pipe looks like it could be dropped,
> why not grep for "^clone< <redacted>"?
>
>
> Also you don't need to quote the space character in regexes, it's not a
> metacharacter.
Updated the grep expressions to look only for the relevant lines and
removed the escaping of the space char.
I was trying to limit the grep to the "packfile-uri" section, not to
match something else by accident, but I think "obj-id http://";
shouldn't match anything else in the clone response (no ref can start
with http://).
Thanks for the quick review!
Ivan
