Re: [PATCH] trace2: prevent segfault on config collection where no value specified

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> 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.