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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2014, 17:29 UTC
Message-ID: <xmqqa964bvd6.fsf@gitster.dls.corp.google.com>
In-Reply-To: <vpqegvhfe5p.fsf@anie.imag.fr>

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

Show 9 quoted lines

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