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

[Junio C Hamano <gitster@pobox.com>]

Ivan Frade <ifrade@google.com> writes:

>> Hmph, the format we expect is "<hash> <uri>"; don't we need to
>> validate the leading <hash> followed by SP?
>
> I was trying to find a uri in a packet in general, not counting on the
> packfile-uri line format. That is probably an overgeneralization.

Ah, I see.  This is merely a tracing, so we might benefit from a
generalized version of redactor, and from that point of view, the
use of strstr and stopping at the whitespace do make sort-of sense
to me, but then we lack any attempt to redact more than one instance
of URL in a packet, so the generalization may have quite a limited
usefulness.

> Next patch version follows these suggestions to look for a packfile-uri line.

Yeah, I think that is a good way to go, at least for now.  When we
want a more general one, we can revisit it, but not now.

>> > -     packet_trace(buffer, len, 0);
>> > +     if (options & PACKET_READ_REDACT_URL_PATH &&
>> > +         (url_path_start = find_url_path(buffer, &url_path_len))) {
>> > +             const char *redacted = "<redacted>";
>> > +             struct strbuf tracebuf = STRBUF_INIT;
>> > +             strbuf_insert(&tracebuf, 0, buffer, len);
>> > +             strbuf_splice(&tracebuf, url_path_start - buffer,
>> > +                           url_path_len, redacted, strlen(redacted));
>> > +             packet_trace(tracebuf.buf, tracebuf.len, 0);
>> > +             strbuf_release(&tracebuf);
>>
>> I briefly wondered if the repeated allocation (and more
>> fundamentally, preparing the redacted copy of packet whether we are
>> actually tracing the packet in the first place) is blindly wasting
>> the resources too much, but this only happens in the protocol header
>> part, so it might be OK.
>
> We only allocate and redact if it looks like a packfile-uri line, so
> it shouldn't happen too frequently.

I was mostly wondering about the cost of determining "if it looks
like?".  But we do this only for the protocol header part, so we
won't have thousands of attempts to match, I guess.  Oh, or if we
also do this for the ref advertisement packets, then we might have
quite a many.  Hmph.

> I move the set/unset of the redacting flag to the FETCH_GET_PACK
> around the "packfile-uris" section.
> There is no need to check every incoming packet for a packfile-uri
> line, we know when they should come.

Yeah, that is quite a wise design decision, I would think.

Thanks.