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
