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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.