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

On Tue, Nov 16 2021, Taylor Blau wrote:

On Mon, Nov 15, 2021 at 11:18:10PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

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.

quoted

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.

quoted

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.