From: Phillip Wood Date: Mon, 07 Feb 2022 14:01:26 GMT Subject: Re: git-checkout doesn't seem to respect config from include.path Message-ID: In-Reply-To: Hi Junio On 03/02/2022 18:07, Junio C Hamano wrote: > Phillip Wood 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);