git-checkout doesn't seem to respect config from include.path

git-checkout doesn't seem to respect config from include.path

[Greg Hurrell]

Hi,

Not sure if this is confined specifically to `git-checkout`, but that's
the command I noticed the issue with:

With the release of the v2.35.0 and the "zdiff3" setting for
"merge.conflictStyle", I find myself wanting to use "zdiff3" on machines
running the new version, and falling back to "diff3" on machines with an
older version.

To this end, I have a ~/.gitconfig that contains:

    [merge]
    	conflictStyle = zdiff3
    [include]
    	path = ~/.gitconfig.local

The idea is that I can use the same `~/.gitconfig` on every machine I
use, but on machines that only have an older Git version, I drop in a
~/.gitconfig.local with overrides like this:

    [merge]
    	conflictStyle = diff3

`git config --get merge.conflictStyle` correctly reports that my setting is
"diff3" on such machines, and `git config --get-all merge.conflictStyle`
shows:

    diff3
    zdiff3

In other words, it knows that I have multiple values set, but it uses
a last-one-wins policy.

However, when I try to run a command like `git checkout -b something`,
Git dies with:

    fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'

So, it looks like something in `git-checkout`'s option processing is
causing it to disregard the override set via "include.path". In fact, it
even disregards a value passed in with `-c` like this:

    git -c merge.conflictStyle=diff3 checkout -b something

Does this sound like a bug, or are my expectations off? I'd be happy to
look into fixing this, but first would like to know whether it is
expected behavior.

Cheers,
Greg

Re: git-checkout doesn't seem to respect config from include.path

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

On 2022-02-02 at 16:04:26, Greg Hurrell wrote:
> However, when I try to run a command like `git checkout -b something`,
> Git dies with:
> 
>     fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'
> 
> So, it looks like something in `git-checkout`'s option processing is
> causing it to disregard the override set via "include.path". In fact, it
> even disregards a value passed in with `-c` like this:
> 
>     git -c merge.conflictStyle=diff3 checkout -b something
> 
> Does this sound like a bug, or are my expectations off? I'd be happy to
> look into fixing this, but first would like to know whether it is
> expected behavior.

This definitely doesn't sound like the expected behavior here.  It's not
clear to me why this is happening, but it probably shouldn't be.  It
doesn't appear that we fail to call the config callback, since we've
been doing that since 2010.

What version of Git are you using that's not working?
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: git-checkout doesn't seem to respect config from include.path

[Greg Hurrell]

On Wed, Feb 2, 2022, at 11:57 PM, brian m. carlson wrote:
> This definitely doesn't sound like the expected behavior here.  It's not
> clear to me why this is happening, but it probably shouldn't be.  It
> doesn't appear that we fail to call the config callback, since we've
> been doing that since 2010.
> 
> What version of Git are you using that's not working?

Initially noticed on v2.30.2, but you can reproduce it to similar
effect on v2.35.0 (ie. add an invalid "merge.conflictStyle" value
like "fizzbuzz" to ~/.gitconfig, and a valid one like "diff3" to
a file pulled in via "include.path", and you'll see the same
behavior).

Cheers,
Greg

Re: git-checkout doesn't seem to respect config from include.path

[Phillip Wood]

Hi Greg

On 02/02/2022 16:04, Greg Hurrell wrote:
> Hi,
> 
> Not sure if this is confined specifically to `git-checkout`, but that's
> the command I noticed the issue with:
> 
> With the release of the v2.35.0 and the "zdiff3" setting for
> "merge.conflictStyle", I find myself wanting to use "zdiff3" on machines
> running the new version, and falling back to "diff3" on machines with an
> older version.
> 
> To this end, I have a ~/.gitconfig that contains:
> 
>      [merge]
>      	conflictStyle = zdiff3
>      [include]
>      	path = ~/.gitconfig.local
> 
> The idea is that I can use the same `~/.gitconfig` on every machine I
> use, but on machines that only have an older Git version, I drop in a
> ~/.gitconfig.local with overrides like this:
> 
>      [merge]
>      	conflictStyle = diff3
> 
> `git config --get merge.conflictStyle` correctly reports that my setting is
> "diff3" on such machines, and `git config --get-all merge.conflictStyle`
> shows:
> 
>      diff3
>      zdiff3
> 
> In other words, it knows that I have multiple values set, but it uses
> a last-one-wins policy.
> 
> However, when I try to run a command like `git checkout -b something`,
> Git dies with:
> 
>      fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'

I think what is happening is that git parses each line of the config 
file as it reads it so the old version of git sees "zdiff3" and errors 
out before it reads the include line. I'm afraid I don't have any useful 
suggestions for avoiding this other than switching the include around so 
that it contains zdiff3 and is only included by newer versions of git.

> So, it looks like something in `git-checkout`'s option processing is
> causing it to disregard the override set via "include.path". In fact, it
> even disregards a value passed in with `-c` like this:
> 
>      git -c merge.conflictStyle=diff3 checkout -b something

I think the values passed with -c are parsed after all the config files 
so the override works. What we really want in this case is to store the 
string value for each config option as we read each config source and 
then parse those values at the end, unfortunately I think that would 
break multi-valued config keys.

Best Wishes

Phillip

> Does this sound like a bug, or are my expectations off? I'd be happy to
> look into fixing this, but first would like to know whether it is
> expected behavior.
> 
> Cheers,
> Greg

Re: git-checkout doesn't seem to respect config from include.path

[Greg Hurrell]

On Thu, Feb 3, 2022, at 4:54 PM, Phillip Wood wrote:
> Hi Greg
> 
> On 02/02/2022 16:04, Greg Hurrell wrote:
> 
> > `git config --get merge.conflictStyle` correctly reports that my setting is
> > "diff3" on such machines, and `git config --get-all merge.conflictStyle`
> > shows:
> > 
> >      diff3
> >      zdiff3
> > 
> > In other words, it knows that I have multiple values set, but it uses
> > a last-one-wins policy.
> > 
> > However, when I try to run a command like `git checkout -b something`,
> > Git dies with:
> > 
> >      fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'
> 
> I think what is happening is that git parses each line of the config 
> file as it reads it so the old version of git sees "zdiff3" and errors 
> out before it reads the include line.

That gave me the idea of moving the `include.path` setting higher up in
the file, to see if `git checkout` would consult that value first, but
it doesn't work; `git config merge.conflictStyle` shows the value from
the file indicated in `include.path`, but a command like `git checkout`
still dies based on the value in ~/.gitconfig.

Overall this points to the general problem that it is not only hard to
make a single config that works on different machines, but it's hard to
make a _combination_ of files that works on different machines.

For now, I think my workaround is going to be templating out
machine-specific files.

Greg

Re: git-checkout doesn't seem to respect config from include.path

[Greg Hurrell]

Minor correction to what I said here:

On Thu, Feb 3, 2022, at 6:39 PM, Greg Hurrell wrote:
> 
> That gave me the idea of moving the `include.path` setting higher up in
> the file, to see if `git checkout` would consult that value first, but
> it doesn't work; `git config merge.conflictStyle` shows the value from
> the file indicated in `include.path`

It only did that because I forgot to remove the original `include.path`
from the bottom of the config. Once I removed that, `git config` showed
the value from ~/.gitconfig. `git checkout` behaved the same either way.

Greg

Re: git-checkout doesn't seem to respect config from include.path

[Phillip Wood]

Hi Greg

On 03/02/2022 17:39, Greg Hurrell wrote:
> On Thu, Feb 3, 2022, at 4:54 PM, Phillip Wood wrote:
>> Hi Greg
>>
>> On 02/02/2022 16:04, Greg Hurrell wrote:
>>
>>> `git config --get merge.conflictStyle` correctly reports that my setting is
>>> "diff3" on such machines, and `git config --get-all merge.conflictStyle`
>>> shows:
>>>
>>>       diff3
>>>       zdiff3
>>>
>>> In other words, it knows that I have multiple values set, but it uses
>>> a last-one-wins policy.
>>>
>>> However, when I try to run a command like `git checkout -b something`,
>>> Git dies with:
>>>
>>>       fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'
>>
>> I think what is happening is that git parses each line of the config
>> file as it reads it so the old version of git sees "zdiff3" and errors
>> out before it reads the include line.
> 
> That gave me the idea of moving the `include.path` setting higher up in
> the file, to see if `git checkout` would consult that value first, but
> it doesn't work; `git config merge.conflictStyle` shows the value from
> the file indicated in `include.path`, but a command like `git checkout`
> still dies based on the value in ~/.gitconfig.

Yes, you need to move the "zdiff3" setting into an include file that is 
only read by recent versions of git.

> Overall this points to the general problem that it is not only hard to
> make a single config that works on different machines, but it's hard to
> make a _combination_ of files that works on different machines.

If you make sure that you are only including files containing recent 
config keys on machines running recent git versions then it should work 
but arranging for that to happen is not necessarily easy

Best Wishes

Phillip

> For now, I think my workaround is going to be templating out
> machine-specific files.
> 
> Greg
> 

Re: git-checkout doesn't seem to respect config from include.path

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> ... What we really want in this case is to
> store the string value for each config option as we read each config
> source and then parse those values at the end, unfortunately I think
> that would break multi-valued config keys.

Thanks for raising, and looking into, the issue.

While the original "callback functions are called for each and every
configuration item defined in the files and it is the responsibility
for these callback functions to implement the semantics like the
last one wins" design that uses git_config() makes it harder, but I
think we are already halfway there, with the more recent API update
in 2014 (!) that allows config_get_value() to go directly get a
value given a key without writing callback functions.

I think builtin/add.c predates the configset API work (of course, it
is natural that we can "git add" way before 2014), and mostly uses
git_config(add_config) callback as a way to parse its configuration,
because it needs to tell other subsystems (like diff, merge, etc.)
that are even older to pay attention to the configuration variables
they care about.

So it may be a major surgery to switch to the newer
config_get_value() API.

For a "last one wins" variable, config_get_value() will only look at
the last item, so any garbage value Git does not recognize would not
trigger a fatal error.

Such an update is both good and bad.  Surely it makes the scenario
that triggered this discussion more pleasant by not dying, but it
makes it too pleasant by not even giving the user a chance to notice
a possible typo.

A incremental improvement that we can immediately make is probably
to teach the current xdiff-interface.c::git_xmerge_config() parser
to react to an unknown value differently.  It should not die() but
just ignore the unknown value, and issue a warning.  This should be
doable with minimum impact to the code.

Completely untested.  The first test that would be interesting to
run is how many tests this changes breaks to gauge how good test
coverage we have ;-)

 xdiff-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git c/xdiff-interface.c w/xdiff-interface.c
index 2e3a5a2943..523b04960a 100644
--- c/xdiff-interface.c
+++ w/xdiff-interface.c
@@ -322,8 +322,8 @@ int git_xmerge_config(const char *var, const char *value, void *cb)
 		 * git-completion.bash when you add new merge config
 		 */
 		else
-			die("unknown style '%s' given for '%s'",
-			    value, var);
+			warning("ignored unknown style '%s' given for '%s'",
+				value, var);
 		return 0;
 	}
 	return git_default_config(var, value, cb);

Re: git-checkout doesn't seem to respect config from include.path

[Phillip Wood]

Hi Junio

On 03/02/2022 18:07, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> ... What we really want in this case is to
>> store the string value for each config option as we read each config
>> source and then parse those values at the end, unfortunately I think
>> that would break multi-valued config keys.
> 
> Thanks for raising, and looking into, the issue.
> 
> While the original "callback functions are called for each and every
> configuration item defined in the files and it is the responsibility
> for these callback functions to implement the semantics like the
> last one wins" design that uses git_config() makes it harder, but I
> think we are already halfway there, with the more recent API update
> in 2014 (!) that allows config_get_value() to go directly get a
> value given a key without writing callback functions.
> 
> I think builtin/add.c predates the configset API work (of course, it
> is natural that we can "git add" way before 2014), and mostly uses
> git_config(add_config) callback as a way to parse its configuration,
> because it needs to tell other subsystems (like diff, merge, etc.)
> that are even older to pay attention to the configuration variables
> they care about.
> 
> So it may be a major surgery to switch to the newer
> config_get_value() API.
> 
> For a "last one wins" variable, config_get_value() will only look at
> the last item, so any garbage value Git does not recognize would not
> trigger a fatal error.
> 
> Such an update is both good and bad.  Surely it makes the scenario
> that triggered this discussion more pleasant by not dying, but it
> makes it too pleasant by not even giving the user a chance to notice
> a possible typo.
> 
> A incremental improvement that we can immediately make is probably
> to teach the current xdiff-interface.c::git_xmerge_config() parser
> to react to an unknown value differently.  It should not die() but
> just ignore the unknown value, and issue a warning.  This should be
> doable with minimum impact to the code.

I think that would be worthwhile, the warning is potentially confusing 
though if a bad value is followed by a good value then we will warn 
about the bad value but use the good one.

Best Wishes

Phillip

> Completely untested.  The first test that would be interesting to
> run is how many tests this changes breaks to gauge how good test
> coverage we have ;-)
> 
>   xdiff-interface.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git c/xdiff-interface.c w/xdiff-interface.c
> index 2e3a5a2943..523b04960a 100644
> --- c/xdiff-interface.c
> +++ w/xdiff-interface.c
> @@ -322,8 +322,8 @@ int git_xmerge_config(const char *var, const char *value, void *cb)
>   		 * git-completion.bash when you add new merge config
>   		 */
>   		else
> -			die("unknown style '%s' given for '%s'",
> -			    value, var);
> +			warning("ignored unknown style '%s' given for '%s'",
> +				value, var);
>   		return 0;
>   	}
>   	return git_default_config(var, value, cb);

Re: git-checkout doesn't seem to respect config from include.path

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I think that would be worthwhile, the warning is potentially confusing
> though if a bad value is followed by a good value then we will warn 
> about the bad value but use the good one.

I dunno.  That is exactly why the new message is crafted to convey:
"you have an entry with an unsupported value in your configuration
file, which you may want to inspect and possibly correct it; in the
meantime we've ignored that entry".  "ignored" is the key word.

If we say "we later found this good value so we'd use it", it may
become confusing, as we'd never issue such a notice for a
last-one-wins variable that do not use any unsupported values, but
we are not doing that, so I think there is no room for confusion.


>> Completely untested.  The first test that would be interesting to
>> run is how many tests this changes breaks to gauge how good test
>> coverage we have ;-)
>>   xdiff-interface.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git c/xdiff-interface.c w/xdiff-interface.c
>> index 2e3a5a2943..523b04960a 100644
>> --- c/xdiff-interface.c
>> +++ w/xdiff-interface.c
>> @@ -322,8 +322,8 @@ int git_xmerge_config(const char *var, const char *value, void *cb)
>>   		 * git-completion.bash when you add new merge config
>>   		 */
>>   		else
>> -			die("unknown style '%s' given for '%s'",
>> -			    value, var);
>> +			warning("ignored unknown style '%s' given for '%s'",
>> +				value, var);
>>   		return 0;
>>   	}
>>   	return git_default_config(var, value, cb);