Re: Fwd: git clone does not respect command line options

[Jeff King <peff@peff.net>]

On Fri, Feb 26, 2016 at 03:34:53PM +0700, Duy Nguyen wrote:

> On Fri, Feb 26, 2016 at 3:24 PM, Jeff King <peff@peff.net> wrote:
> > As an alternative, it would be nice to have some config syntax for
> > "clear the list". Maybe something like an empty string, which I think
> > has no meaning for the current multi-valued variables (at least not for
> > credential helpers or refspecs). That would allow something like:
> >
> >   git -c credential.helper= clone ...
> >
> > to do what you'd expect.
> 
> I've been thinking of -= instead. It's unambiguous. And you can use
> wildcards on both sides. "credential.helper -= *" means delete that
> key, "credential.* -= *" deletes all credential.* keys.
> credential.helper -= abc only deletes it if the previous value is abc.

But there you're inventing new syntax, so you'd need to invent new
syntax inside the config file, too. And you'd need to somehow
communicate to the consumers of the config values that the value is
"unset". So for config callbacks inside of git, they need to take more
than just the key/value pair (or we'd have to read all of the config and
pre-process it). Ditto for git-config. How do we show in the output of
--get-all that the list was reset? Or again, we could pre-process
completely in git-config (which would probably mean using a new option,
--get-list or something, instead of --get-all).

By contrast, I think my suggestion can be implemented as:

diff --git a/credential.c b/credential.c
index 7d6501d..aa99666 100644
--- a/credential.c
+++ b/credential.c
@@ -63,9 +63,12 @@ static int credential_config_callback(const char *var, const char *value,
 		key = dot + 1;
 	}
 
-	if (!strcmp(key, "helper"))
-		string_list_append(&c->helpers, value);
-	else if (!strcmp(key, "username")) {
+	if (!strcmp(key, "helper")) {
+		if (*value)
+			string_list_append(&c->helpers, value);
+		else
+			string_list_clear(&c->helpers, 0);
+	} else if (!strcmp(key, "username")) {
 		if (!c->username)
 			c->username = xstrdup(value);
 	}

The big downside is that each consumer of the value needs to learn this
trick. But as I said, I think there aren't very many.

Don't get me wrong; I think your suggestion is a little cleaner. If we
were designing the config system from scratch, I'd probably favor a
single query-able tree rather than the callback system, and do things
like list-processing centrally. But given the history, I'm not sure if
it's worth it now.

-Peff