Re: [PATCH v2 2/2] make config --add behave correctly for empty and NULL values

[Junio C Hamano <gitster@pobox.com>]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> +const char CONFIG_REGEX_NONE[] = "a^";
>
> I have a slight preference for this version (no magic (void *)1 value,
> and belts-and-suspenders solution since someone actually using the regex
> should still get a correct behavior.
>
> But I'm fine with both Junio/Peff's version or this one.

I do not care too deeply either way, to be honest.

But if we were to redo this in the right way, I suspect that the
best solution may be to correct the root cause, which is the design
mistake in the git_config_set_multivar_in_file API.  The function
takes a regexp (possibly NULL) and a multi_replace bit, and with
that expresses these three combinations:

    - a non-NULL regexp means only the existing ones that match are
      subject to replacement;

    - NULL regexp means all of the existing ones that match are
      subject to replacement;

    - multi-replace bit controls which ones among the replacement
      candidates are replaced (either the first one or all).

But we actually want to express three, not two, different handling
for the existing entries.  Either (1) use the regexp to decide which
ones are subject to replacement, (2) declare all of them are subject
to replacement, or (3) declare none of them are to be replaced.  The
last one cannot be expressed without coming up with a trick to say
"I am giving a regexp that hopefully will not match anything as a
workaround because otherwise you will replace all of them but what I
really want to say is I do not want you to replace anything", and
this thread discusses a fix to the bug in the implementation that
failed to come up with a "hopefully will not match anything"
pattern.  And we are still discussing to fix a better workaround.

Instead of polishing the workaround, wouldn't it be better to make
it unnecessary to work it around?  For exaple, we could turn the
last parameter to the function into an "unsigned flag" with two
bits, CONFIG_SET_USE_REGEXP_TO_FILTER (if set, use the regexp to
filter which of the existing entries to be replaced) and
CONFIG_SET_REPLACE_MULTI (if set, replace all the eligible ones),
and the result would be conceptually a lot cleaner, no?

Some notes:

 - Because most callers expect "replace" behaviour, instead of
   adding CONFIG_SET_USE_REGEXP_TO_FILTER to the majority of
   existing callers, a new flag CONFIG_SET_JUST_APPEND (which is
   exactly the negation of the USE_REGEXP_TO_FILTER) would be a more
   practical thing to introduce.

 - We can keep using value_regexp==NULL (under !JUST_APPEND) to mean
   value_regexp=".*", i.e. matches anything, as a short-hand.

Hmm?