Re: Disabling credential helper?

[Jeff King <peff@peff.net>]

On Tue, Dec 02, 2014 at 08:36:07PM -0500, Jeff King wrote:

> But to answer your question: you can't currently. I would be happy to
> have a config syntax that means "reset this multi-value config option
> list to nothing", but it does not yet exist. It would be useful for more
> than just credential-helper config.

This is something I've wanted for a while, so I took another peek at it.
It turns out to be rather complicated.

This is the initial patch I came up with (don't bother reading it too
closely):

diff --git a/credential.c b/credential.c
index 1886ea5..9e84575 100644
--- a/credential.c
+++ b/credential.c
@@ -43,9 +43,6 @@ static int credential_config_callback(const char *var, const char *value,
 	if (!skip_prefix(var, "credential.", &key))
 		return 0;
 
-	if (!value)
-		return config_error_nonbool(var);
-
 	dot = strrchr(key, '.');
 	if (dot) {
 		struct credential want = CREDENTIAL_INIT;
@@ -63,8 +60,13 @@ static int credential_config_callback(const char *var, const char *value,
 		key = dot + 1;
 	}
 
-	if (!strcmp(key, "helper"))
-		string_list_append(&c->helpers, value);
+	if (!strcmp(key, "helper")) {
+		if (value)
+			string_list_append(&c->helpers, value);
+		else
+			string_list_clear(&c->helpers, 0);
+	} else if (!value)
+		return config_error_nonbool(var);
 	else if (!strcmp(key, "username")) {
 		if (!c->username)
 			c->username = xstrdup(value);


It allows you to do:

  git -c credential.helper [-c credential.helper=foo] fetch ...

The first one "resets" the list to empty, and then you can further
append to the list after that. There are a bunch of shortcomings,
though:

  1. I chose the value-less boolean as a token to reset the list (since
     it is otherwise an unmeaningful error). The example above shows its
     use with "-c", but you could also do:

       [credential]
       helper
       helper = foo

     in a config file itself.  This is probably rather unintuitive.

     But whatever syntax is chosen would need to have some mechanism for
     specifying it in the config file itself, as well as in the
     command-line (and therefore in the semi-public
     GIT_CONFIG_PARAMETERS environment variable which is passed around).
     So this is at least backwards-compatible, "just works" with
     existing config code, and won't stomp on any existing working
     values.

     If we can accept stomping on an unlikely-used token, something
     like:

       git -c credential.helper=RESET fetch ...

     is more sensible (and we can argue about the exact token used).
     If we can accept new syntax and new config code, something like:

       git -c '!credential.helper' fetch ...

     is probably workable.

  2. Notice how we didn't touch the config code at all here. That's
     because it doesn't know anything about whether a config item is a
     multi-valued list, or that the string list exists. It is up to each
     individual consumer of the config callbacks to implement this
     list-clearing mechanism (and obviously we should make them all
     agree on the token used). So we'd probably want to make similar
     changes everywhere that multi-values are used (which, to be fair,
     really isn't that many, I don't think).

  3. Running `git config credential.helper` doesn't know about this
     mechanism either. You can either get the "last one wins" behavior,
     or you can use --get-all to get all instances. We'd probably have
     to teach it a new `--get-list` that recognized the magic reset
     token.

None of those problems is insurmountable. It just takes a little more
code than what I wrote above. But for credential helpers specifically,
it's a bit more challenging. Because we're _not_ constructing just a
list of just credential.helper here. We're constructing a list of all
matching credential.*.helper config keys. So in something like:

  [credential "http://example.com"]
  helper = foo
  [credential]
  helper = bar

if you run "git fetch http://example.com", you'll get both helpers, in
sequence.

What should adding:

  [credential]
  helper = RESET

do on top of that?  Intuitively, I'd say that it would only touch the
credential.helper variables. But that's not what the user probably
wants. They probably want to reset _any_ helpers that have matched. And
that's actually what the code I posted above would do. So that's good, I
guess. But I wonder if this approach is introducing more confusion than
it is worth.

Having a separate --no-respect-credential-helpers is conceptually much
simpler. But it also does not allow you to reset the list and add in a
new helper.

-Peff