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

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 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.

Thanks,
Taylor