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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2022, 15:53 UTC
Message-ID: <xmqqr12is9gp.fsf@gitster.g>
In-Reply-To: <de8d38fc-23e3-b876-87e8-7b4193bff44c@jeffhostetler.com>

Jeff Hostetler <git@jeffhostetler.com> writes:

Show 6 quoted lines

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

At these sites where the new traces are added, we cannot detect the remaining case (d) "requested by the user, asked for the server, but the server dropped the ball", I think the above covers the possible cases that are interesting to us entirely.

Show 20 quoted lines

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

Sounds alright. We could standardize the other way, which might make the interpretation of individual trace entries independent of the context easier, though.

Thanks.