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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 16, 2021, 18:43 UTC
Message-ID: <YZP771pJl30ujluT@nand.local>
In-Reply-To: <RFC-cover-00.21-00000000000-20211115T220831Z-avarab@gmail.com>

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

Show 20 quoted lines

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

Show 32 quoted lines

> 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