Re: [PATCH] fetch-pack: write effective filter to trace2

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jul 15, 2022, 20:49 UTC
Message-ID: <20220715204956.2927778-1-jonathantanmy@google.com>
In-Reply-To: <xmqqfsj2yw6b.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 15 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >> >> This is implemented for protocol v0, v1, and v2.
> >> 
> >> Is that different to say "for all protocols"?  I am wondering if it
> >> is worth saying (unlike in a hypothetical case where we do not
> >> support v0 and v1 we may want to state why we only support v2).
> >
> > I wrote it that way to avoid confusion with things like HTTP (which is a
> > protocol, at least in its name). Maybe better would be "This is
> > implemented for all protocols (v0, v1, and v2)".
> 
> I still wonder if it is better left unsaid.  When adding a new
> thing, it would not be noteworthy if the new thing is available no
> matter what protocol is being used, no?

I was thinking that a reviewer would think "why is this done twice" and wrote this comment to answer this issue in advance, but it makes sense to just remove it. OK - I'll do that.

Show 10 quoted lines

> > My intent is to report what is being sent to the server in the fetch
> > request.
> 
> Then I'd be OK with a design that reports "none", if we send "none"
> to the server in this case.  If not, then I do not think we should.
> 
> Perhaps report an empty string or not reporting at all?  After all,
> the reader knows the client version and capability in the log so it
> is easy to tell between 'no filter was used' and 'too old to report
> the filter', no?  I dunno.

We don't send "none" to the server. If an empty string is OK then I'll use that, since it will make it slightly easier for analysis since we do not have to check against the version (and also to know which version has support for this feature). But I'm OK either way.