Re: [PATCH 1/2] fetch-pack: redact packfile urls in traces
- From
Ivan Frade <ifrade@google.com>
- Date
- Oct 8, 2021, 23:15 UTC
- 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:
Show 29 quoted lines
>
>
> 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.
Show 13 quoted lines
> > > + > > + 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