Re: [PATCH v5 5/5] tr2: avoid to print "interesting" config repeatedly

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Jul 1, 2022, 14:31 UTC
Message-ID: <838e1273-b704-bd11-e9ed-e25390989027@jeffhostetler.com>
In-Reply-To: <xmqqbkuck66t.fsf@gitster.g>

On 6/28/22 2:12 PM, Junio C Hamano wrote:

Show 25 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
>> Do we care in this case? I really don't know, but perhaps we can declare
>> "dedup" using the same facility we're using to wildcard-match keys, and
>> either make that optional or the default, e.g.:
>>
>> 	GIT_TRACE2_CONFIG_PARAMS='*:dedup,core.*:'
>>
>> I.e. to make it a list of <glob>[:<options>].
>>
>> Maybe not worth the effort...
> 
> I happen to think that the trace output is primarily for machine
> consumption (i.e. if you want to make it readable by humans, it is
> OK to require them to pre-process).
> 
> How does this "duplicate output" come about?  If it is because
> end-user configuration files have multiple entries that are either
> list-valued or relying on last-one-wins semantics, I suspect that it
> is better not to prematurely filter these at the trace generation
> side (which by definition is an opration that loses information).
> 
> So, to me, it smells that the whole "dedup at the source" thing is
> not just not worth the effort but is misguided.
>

I agree. Let's not try to dedup these. IIRC, the tr2_cfg_cb() machinery runs during a "read early" or "read very early" scan of the config values and the program is still starting up (in some sense). For example, the command line may not have been fully processed yet (again, IIRC). So injecting a call here to force an explicit lookup could cause problems.

And you'll be arbitrarily taking the first value, which is probably the system level setting rather than the last value, which is probably the local setting. So the output would be misleading.

And I don't think it is worth the effort. Let's just log the context (system, global, local) as you described in the previous version and be happy that we get multiple-but-now-qualified values.

This was intended as a machine-readable feature to allow telemetry post-processing to group or filter command events. For example, if we want to do some "feature-x" is ON or OFF and compute averages for the ONs and averages for the OFFs and compare them. Such post- processing has (I'm assuming) in the past always looked at the last value logged. And I assume that that would not change here with the additional qualification.

Also, your new qualification would help us answer support questions using telemetry where a user thought they had a feature on (say globally) but was actually off at another level (say locally).

Jeff