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

From: Ivan Frade <ifrade@google.com>
Date: Nov 10, 2021, 23:44 UTC
Message-ID: <CANQMx9U2sRB9Qm3zxvpOwn8cqRYyA0S0jJ2=JsspJ5hcRd_XOA@mail.gmail.com>
In-Reply-To: <211109.86mtmedrhr.gmgdl@evledraar.gmail.com>

On Mon, Nov 8, 2021 at 5:53 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

...

Show 6 quoted lines

>... Let's just:
>
>  1. Start reading the section
>  2. Turn off tracing
>  3. Parse the URIs as we go
>  3. When done (or on the fly), scrub URIs, log any backlog suppressed trace, and turn on tracing again

This is a more generic redacting mechanism, but I understood that there is no need for it. Previous comments went in the direction of removing generality (e.g. not looking for a URI anywhere in the packet, but specifically for the packfile line format) and now this patch is very specific to redact packfile-uri lines in the protocol.

Show 6 quoted lines

> Instead of:
>
>  1. Set a flag to scrub stuff
>  2. Because of the disconnect between fetch-pack.c and pkt-line.c,
>     effectively implement a new parser for data we're already going to be
>     parsing some microseconds later during the course of the request.

pkt-line is only looking for the "<n-hex-chars>SP" shape. True that it encodes some protocol knowledge, but it is hardly a new parser.

> That "turn off the trace" could be passing down a string_list/strbuf, or
> even doing the same via a nev member in "struct packet_reader", both
> would be simpler than needing to re-do the parse.

Saving the lines and delaying the tracing could also produce weird outputs, no? e.g. 3 lines received, the second doesn't validate, the program aborts and the trace doesn't show any of the lines that caused the problem. Or we would need to iterate in parallel through lines and saved-log-lines assuming they match 1:1. Nothing unsolvable, but I am not sure it is worthy the effort now.

Thanks,