From: Ivan Frade <ifrade@google.com>
Date: Fri, 08 Oct 2021 23:15:10 GMT
Subject: Re: [PATCH 1/2] fetch-pack: redact packfile urls in traces
Message-ID: <CANQMx9Wd36xdMS5xyu759=aw1gVVnuGqsWzGsDVXqYLO_wuh1A@mail.gmail.com>
In-Reply-To: <87zgrjmhgd.fsf@evledraar.gmail.com>

On Fri, Oct 8, 2021 at 12:42 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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