Re: [PATCH] clone: handle empty config values in -c

From: Jeff King <peff@peff.net>
Date: May 2, 2017, 02:56 UTC
Message-ID: <20170502025623.jhs4hyiy6gc5sv7y@sigill.intra.peff.net>
In-Reply-To: <20170502000515.GU28740@aiede.svl.corp.google.com>

On Mon, May 01, 2017 at 05:05:15PM -0700, Jonathan Nieder wrote:

Show 11 quoted lines

> "git clone --config" uses the following incantation to add an item to
> a config file, instead of replacing an existing value:
> 
> 	git_config_set_multivar_gently(key, value, "^$", 0)
> 
> As long as no existing value matches the regex ^$, that works as
> intended and adds to the config.  When a value is empty, though, it
> replaces the existing value.
> [...]
> ---
> Thoughts?

Yeah, I think this is the exact reason we introduced CONFIG_REGEX_NONE in the first place. At the time we fixed "config --add", but "clone -c" needs the same treatment.

Grepping around, it looks like we should probably be using this in other places, too:

  - writing fetch refspecs in clone

  - throughout remote.c for urls and refspecs

I don't think an empty variable has meaning in those places, so probably nobody really cares. But passing CONFIG_REGEX_NONE seems to better match the original intent. And in the long run I think we probably ought to make an empty "remote.foo.url" mean "reset the url list to empty", as we've started to do with other multivars (like credential helpers).

So your patch looks fine, but I'd be pleased if you wanted to take it further and eradicate this "^$" anti-pattern through the code base.

-Peff

PS I notice that the documentation for "config --add" explicitly
   mentions that it behaves like "^$". This isn't accurate anymore. We
   should probably update that. I wondered if callers of git-config
   would need some way to specify "--replace-all --no-value" or
   something, but that is precisely what "--add" is. So I think it's
   just a documentation problem.