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

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Jul 18, 2022, 14:08 UTC
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:

...

Show 8 quoted lines

>>> +		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