On 6/28/22 2:12 PM, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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
