From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mon, 18 Jul 2022 14:08:42 GMT
Subject: Re: [PATCH] fetch-pack: write effective filter to trace2
Message-ID: <de8d38fc-23e3-b876-87e8-7b4193bff44c@jeffhostetler.com>
In-Reply-To: <xmqqmtdaz0vt.fsf@gitster.g>



On 7/15/22 2:28 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
>> On 7/15/22 1:29 PM, Jonathan Tan wrote:
...
>>> +		trace2_data_string("fetch", the_repository, "fetch/effective-filter", "none");
> 
> At the first glance, this seems to lose data, because you should be
> able to use expand_list_objects_filter_spec() to report the filter
> spec.  But this is reporting the filter that was actually in effect,
> so it is OK.
> 
> But the same question about "none" remains.

Yeah, the use of "none" gave me pause, but I didn't have a better idea
at the time.  I guess we have:
(a) requested, supported, used.
(b) "none used because the server doesn't support it" and
(c) "none used because the user didn't request it" cases,
right?

Perhaps it would be better to do:
     if (server_supports_filtering && args->filter_options.choice)
         trace2_data_string("fetch", r, "filter/effective", spec);
     else
         trace2_data_string("fetch", r, "filter/unsupported", spec);

Using two different keywords.

So that the log only contains "filter/effective" when it was actually
used.  And there is no "filter/effective" event when (for whatever
reason) it was not in effect.

Then the "filter/unsupported" event helps you with debugging.  Did they
hit a server that doesn't support filtering or did they have a typo in
their filter spec?

Then don't emit a message at all for the "not requested" case.  And you
can use the Git version number to know how to interpret it.  There are
other places where we don't bother sending messages where the value is
a zero or empty.

Jeff