Re: [PATCH v6 1/2] fetch-pack: redact packfile urls in traces

From: Ivan Frade <ifrade@google.com>
Date: Nov 10, 2021, 21:18 UTC
Message-ID: <CANQMx9VFeLAJQn1+AyF-rtXikpgr_LotnudPqOP=k0qwWgZdDA@mail.gmail.com>
In-Reply-To: <20211108230111.1101434-1-jonathantanmy@google.com>

On Mon, Nov 8, 2021 at 3:01 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 5 quoted lines

>
> > +                     reader.options &= ~PACKET_READ_REDACT_URI_PATH;
>
> Probably worth commenting why you're resetting the flag (avoid the
> relatively expensive URI check when we don't need it).

Done

>
> > diff --git a/pkt-line.c b/pkt-line.c

...

Show 6 quoted lines

> > +     len = strspn(buffer, "0123456789abcdefABCDEF");
> > +     if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')
> > +             return NULL; /* required "<hash>SP" not seen */
>
> Optional: As I said in my reply (just sent out), checking for both SHA-1
> and SHA-256 lengths is reasonable too.

Done (with a comment indicating they are the hash sizes of SHA1 and SHA256)

Show 5 quoted lines

> > +     GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
>
> No need for GIT_TRACE=1 since you're not checking stdout. Also I don't
> think GIT_TEST_SIDEBAND_ALL=1 is needed - we should check that it works
> even without a test variable (and I've checked and it seems to work).

Done in both tests.

Thanks,