Jeff King <peff@xxxxxxxx> writes: > I.e., doing this, with an explicit value for the config option: > > GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.* git -c status.relativePaths=true version > > should (and does) show: > > 20:48:11.662470 trace2.c:437 def_param scope:command status.relativepaths=true > > If we swap that our for "-c status.relativePaths", then the outcome is > the same: we've turned on that config option. But with your patch, the > trace won't mention it at all. which may be improvement, but ideally, the "valueless truth" case should be given differently, perhaps like 20:48:11.662470 trace2.c:437 def_param scope:command status.relativepaths to allow showing what exactly the system has seen. After all, trace output is often used for debugging, and it is not unusual for a buggy code path to behave on explicit truth and valueless truth differently. > So here I think we need to either: > > 1. Just quietly substitute "true" for the value. For a bool, the two > are equivalent, and this is probably an acceptable fiction for a > trace to show. For a non-bool (e.g., something like "author.name"), > though, it's an error, and the trace is somewhat misleading. > > 2. Put in some special marker for the NULL value. Something like > "(null)" works, but it's ambiguous with a config of the same value > (which obviously you wouldn't expect in normal use, but since the > point of tracing is often to debug, I could see it being > misleading). > > All of this is made harder by the fact that there are multiple output > targets. So you'd have to pass the NULL down to them and let them handle > it. Something like: > ... > diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c > index baef48aa69..924736ab36 100644 > --- a/trace2/tr2_tgt_normal.c > +++ b/trace2/tr2_tgt_normal.c > @@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param, > enum config_scope scope = kvi->scope; > const char *scope_name = config_scope_name(scope); > > - strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param, > - value); > + strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param); > + if (value) > + strbuf_addf(&buf_payload, "=%s", value); Yes, exactly. > normal_io_write_fl(file, line, &buf_payload); > strbuf_release(&buf_payload); > } > > but you'd need to do the same for each target implementation. Thanks.
