Subject: Re: [PATCH v4 05/10] userdiff: add and use for_each_userdiff_driver()

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 25 Mar 2021 02:26:21 +0100

On Thu, Mar 25 2021, Jeff King wrote:

> On Thu, Mar 25, 2021 at 12:05:09AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> >   As an aside, it feels like this is something we ought to be able to
>> >   ask git-config about, rather than having a test-helper. This is
>> >   basically "baked-in" config, and if we represented it as such, and
>> >   parsed it into a struct just like regular config, then probably "git
>> >   config --list --source" could be used to find it (and differentiate it
>> >   from user-provided config). Possible downsides:
>> >
>> >     1. Would people find it confusing that "git config --list" suddenly
>> >        gets way bigger? Maybe we'd want an "--include-baked-in" option
>> >        or something.
>> >
>> >     2. Is the cost of parsing the config measurably bad? Obviously a
>> >        user could provide the same content and we'd have to parse it,
>> >        but there's a lot more rules here than most users would probably
>> >        provide.
>> 
>> Also:
>> 
>>  3. Only the PATTERNS() macro translates as-is to config syntax. We
>>     don't have a way to do what IPATTERN() does in the config syntax
>>     currently.
>> 
>>     We could add a ifuncname and xifuncname or whatever for it I guess,
>>     but currently the ICASE behavior in the C code is magic.
>
> Good point. IMHO that is something we should consider fixing
> independently. It was a mistake to add builtins that couldn't be
> replicated via the config (though I notice it happened quite a while
> ago, and nobody seems to have cared, so perhaps it isn't that
> important).

I'm conspiring to eventually optimistically replace these ERE patterns
with PCRE if we build with that at some point.

Then you could just prefix your pattern with (?i) here in this and other
things that want icase...

> I have also wondered if we should just ship a file which could be
> installed as /etc/gitconfig.filetypes and include it the stock
> /etc/gitconfig. That is effectively the same as "baked in", but
> hopefully makes it more clear to users how they can modify things.  But
> all of that is somewhat orthogonal to what you're doing here.

In theory I'm with you on that, in practice this is just the sort of
thing that requires opt-in effort from every person packaging git
(installing system-wide-config).

A good number of those people will decide "default config values? In
some ~200 line file included in /etc/gitconfig?? I don't need that! This
is a minimal install!".

And then their web UI that calls "git diff" under the hood won't do the
right thing with a .gitattributes config that expects these built-in
drivers anymore.

But yeah, there's no reason your earlier suggestion of injecting global
defaults into "git config -l" wouldn't work, and for our own C APIs such
as this one to rely on that happening.

It would even make git more discoverable, as all the "this is set to xyz
by default" docs in git-config(1) could become reflective, you could
just run "git config -l --show-origin | grep ^default:" or something to
see all default values.