Hi Junio
On 03/02/2022 18:07, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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);
