| Message ID | RFC-cover-00.21-00000000000-20211115T220831Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | C99: show meaningful <file>:<line> in trace2 via macros | expand |
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
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.
On Tue, Nov 16, 2021 at 07:58:01PM +0100, Ævar Arnfjörð Bjarmason wrote: > On Tue, Nov 16 2021, Taylor Blau wrote: > >> 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 don't feel strongly, and I was just noting that it seemed like users would treat this extra information more often as noise than anything else. When you talk about making it optional, do you mean through configuration / an environment variable, or by including / not including the patch? In other words, the latter seems much more like us making a decision on whether or not to include line numbers rather than presenting a new option to users, though I may be misunderstanding. > 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. Interesting; so this and others are likely part of MSVC's kind-of support for C99 features? In other words, that MSVC supports some features from C99 (and we are depending on a subset of those) but not all features so that it could reasonably be called a spec-compliant compiler for the C99 standard? If so, makes sense. Thanks, Taylor
On Tue, Nov 16 2021, Taylor Blau wrote: > On Tue, Nov 16, 2021 at 07:58:01PM +0100, Ævar Arnfjörð Bjarmason wrote: >> On Tue, Nov 16 2021, Taylor Blau wrote: >> >> 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 don't feel strongly, and I was just noting that it seemed like users > would treat this extra information more often as noise than anything > else. > > When you talk about making it optional, do you mean through > configuration / an environment variable, or by including / not including > the patch? In other words, the latter seems much more like us making a > decision on whether or not to include line numbers rather than > presenting a new option to users, though I may be misunderstanding. Not surprising, since I see that I screwed up the summary in both the CL and 21/21. I.e. both of these are =false (I copy/pasted the error around, but didn't adjust the command that was invoked): $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x fatal: bad boolean config value 'y' for 'core.x' $ 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' That second one should be core.usageAddSource=true, i.e. as seen in the 21/21 implentation and core.txt docs you only get these line numebers if you opt-in to them via configuration or the new GIT_TEST_* environment variable. >> 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. > > Interesting; so this and others are likely part of MSVC's kind-of > support for C99 features? In other words, that MSVC supports some > features from C99 (and we are depending on a subset of those) but not > all features so that it could reasonably be called a spec-compliant > compiler for the C99 standard? If so, makes sense. Yes, I think this is one of the things that's the same or similar enough to C++ that MSVC has good support for it. See the "We try to support a wide range of C compilers[...]" section in the CodingGuidelines for a list of some other C99 features we've used for years already.
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' 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'" } I've got some speculation in 19/21 that this may make the "fmt" part redundant, i.e. did we only add that because we couldn't group these by file/line, but as noted there there's still some use-cases for "fmt" even with this series. In any case, this series doesn't touch that "fmt" key at all. 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. But I wanted to see if people found the general idea interesting too. I picked the CC list mainly from paging through "--grep=trace2", and people who'd modified the tricker bits of usage.c code being modified here. 1. https://lore.kernel.org/git/cover-0.2-00000000000-20210412T105422Z-avarab@gmail.com/ 2. https://lore.kernel.org/git/cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com/ 3. https://github.com/avar/git/runs/4216916706?check_suite_focus=true Ævar Arnfjörð Bjarmason (21): git-compat-util.h: clarify GCC v.s. C99-specific in comment C99 support: hard-depend on C99 variadic macros usage.c: add a die_message() routine usage.c API users: use die_message() where appropriate usage.c + gc: add and use a die_message_errno() config API: don't use vreportf(), make it static in usage.c common-main.c: call exit(), don't return usage.c: add a non-fatal bug() function to go with BUG() parse-options.[ch] API: use bug() to improve error output receive-pack: use bug() and BUG_if_bug() cache-tree.c: use bug() and BUG_if_bug() pack-objects: use BUG(...) not die("BUG: ...") strbuf.h: use BUG(...) not die("BUG: ...") usage API: create a new usage.h, move API docs there usage.[ch] API users: use report_fn, not hardcoded prototype usage.[ch] API: rename "warn" vars functions to "warning" usage.c: move usage routines around usage.c: move rename variables in usage routines around usage API: use C99 macros for {usage,usagef,die,error,warning,die}*() usage API: make the "{usage,fatal,error,warning,BUG}: " translatable usage API: add "core.usageAddSource" config to add <file>:<line> Documentation/CodingGuidelines | 3 + Documentation/config/core.txt | 7 + .../technical/api-error-handling.txt | 81 ------ Documentation/technical/api-trace2.txt | 4 +- apply.c | 8 +- apply.h | 6 +- banned.h | 5 - builtin/fast-import.c | 22 +- builtin/gc.c | 21 +- builtin/notes.c | 15 +- builtin/pack-objects.c | 2 +- builtin/receive-pack.c | 16 +- cache-tree.c | 7 +- common-main.c | 9 +- config.c | 22 +- config.h | 10 +- daemon.c | 3 +- git-compat-util.h | 59 +--- http-backend.c | 6 +- imap-send.c | 3 +- parse-options.c | 56 ++-- repo-settings.c | 11 + repository.h | 2 + run-command.c | 32 +-- strbuf.h | 2 +- t/helper/test-trace2.c | 27 +- t/t0210-trace2-normal.sh | 52 ++++ trace.c | 80 +----- trace.h | 133 ++++----- trace2.c | 45 +-- trace2.h | 28 -- usage.c | 270 +++++++++++------- usage.h | 180 ++++++++++++ 33 files changed, 636 insertions(+), 591 deletions(-) delete mode 100644 Documentation/technical/api-error-handling.txt create mode 100644 usage.h