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

> 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?

> 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

> 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`

> 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.

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

> 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);

>> 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);