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

[Phillip Wood <phillip.wood123@gmail.com>]

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