Re: [RFC PATCH 00/21] C99: show meaningful <file>:<line> in trace2 via macros

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Nov 16 2021, Taylor Blau wrote:

> On Mon, Nov 15, 2021 at 11:18:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Since everyone's getting in on the C99 fun.
>>
>> Well, $subject and a bit more. This RFC series has bits and pieces
>> from thing I've submitted before. I'd proposed to make variadic macros
>> a hard dependency before in [1] because I wanted to get to the goal in
>> $subject, perhaps the whole thing will be more convincing.
>>
>> This also includes the die_message() in a recent series of mine[2]
>> that I abandoned.
>>
>> At the end of this series we expose a config variable to have
>> usage/die/warning emit line numbers. I.e. going from:
>>
>>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
>>     fatal: bad boolean config value 'y' for 'core.x'
>>
>> To:
>>
>>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
>>     fatal: config.c:1241: bad boolean config value 'y' for 'core.x'
>
> Just picking on this output change in particular. I agree that this is
> easier for folks hacking on Git to trace down errors. But I'm not sure
> that I could say then same about users, who will likely treat this extra
> output as noise.
>
> Now we may find it helpful if they include it in a bug report, but I
> feel reasonably comfortable saying that the value there is pretty
> marginal. I don't find it all that problematic to grep for a specific
> error string, and usually find myself in the right place.

I wouldn't suggest exposing this to users, except perhaps as part of
some "how to submit a bugreport" instructions. It's thoroughly optional.

I thought it was easy enough to do with the preceding steps since all
the data is there, and would help my workflow a lot.

If you've got the file/line number like that you can make it clickable
in your terminal/compile mode, e.g. Emacs's M-x compile. Saves time over
having to grep manually select the string, grep for it etc.

Anyway, I can certainly live with peeling this patch off the end and
just stopping at the trace2 data for now, if you/others feel strongly
about it.

>> I find that to make tracing down errors in the test suite, and 21/21
>> has a GIT_TEST_* mode to turn it on there (which fails a lot now, but
>> I'm hoping I'll eventually get passing).
>>
>> But most importantly we've now got meaningful file/line numbers in
>> trace2 error events. I.e. from all of them being some line in usage.c:
>>
>>     $ GIT_TRACE2_EVENT=/dev/stdout ~/g/git/git -c core.usageAddSource=false -c core.x=y config --get --bool core.x 2>&1 2>/dev/null|grep error | jq -r .
>>     {
>>       "event": "error",
>>       "sid": "20211115T221343.534151Z-Hc2f5b994-P003f3980",
>>       "thread": "main",
>>       "time": "2021-11-15T22:13:43.537981Z",
>>       "file": "usage.c",
>>       "line": 65,
>>       "msg": "bad boolean config value 'y' for 'core.x'",
>>       "fmt": "bad boolean config value '%s' for '%s'"
>>     }
>>
>> To:
>>
>>     $ GIT_TRACE2_EVENT=/dev/stdout ~/g/git/git -c core.usageAddSource=false -c core.x=y config --get --bool core.x 2>&1 2>/dev/null|grep error | jq -r .
>>     {
>>       "event": "error",
>>       "sid": "20211115T221357.083824Z-Hc2f5b994-P003f4a82",
>>       "thread": "main",
>>       "time": "2021-11-15T22:13:57.087596Z",
>>       "file": "config.c",
>>       "line": 1241,
>>       "msg": "bad boolean config value 'y' for 'core.x'",
>>       "fmt": "bad boolean config value '%s' for '%s'"
>>     }
>
> Neat. This is a use-case that has all of the value without putting it in
> front of users all of the time. I like it.
>
>> This is "RFC" mainly because there's a CI failure in 0061.2 with this,
>> I still can't figure out what that's about (or if it's some fluke
>> unrelated to this topic), but that has to be investigated.
>
> Hmm. Putting the CI failures aside for a second, wouldn't we want to
> hold off on something like this until we have flown the C99 weather
> balloon for a while? If we suddenly start introducing C99-isms into the
> code while brian's patch is still young, then we can suddenly no longer
> say, "oh, just drop this #if because there are no other C99-specific
> uses here", and instead compilers that don't support the newer standard
> are out of luck.
>
> That may have been already communicated elsewhere in this message and/or
> throughout your patch series, so if I missed it, I apologize. Just
> felt that it was worth stating the obvious before we go too far down the
> wrong path.

As noted in 02/21 we're hard depending on this particular C99 feature
already fon a few releases now, the only change on that front in this
series is to stop committing to maintaining the non-C99 codepaths.

We've already had hard dependencies on various bits of C99 for years now
without any trouble, and I wouldn't expect any problems on this front
either.

Brian's series and the current weatherbaloon from Junio are a bit
different in trying out new things we either haven't done before, or
have run into some trouble with in the past.

No need at all to apologize, it's a lot of patches, and raising this
sort of thing is what patch review is good for.

Thanks a lot for looking this over.