Re: [PATCH] credential: let empty credential specs reset helper list

[Jeff King <peff@peff.net>]

On Fri, Feb 26, 2016 at 11:34:12AM -0800, Junio C Hamano wrote:

> >> -	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")) {
> >
> > I wondered why neither the existing code nor the updated one has a
> > check for !value, but this callback assumes no credential
> > configuration variable will ever be a boolean and rejects it
> > upfront, so this code before or after the change is safe.
> >
> > Not pointing out anything that needs to be changed; demonstrating
> > that I did read this sufficiently well to say that I have reviewed
> > it ;-)
> 
> This reminds me of one thing.  The only reason why we are hesitant
> to introduce a new syntax like
> 
> 	[credential]
>         	!helper ;# clear
>                 helper = ...
> 
> to allow explicit clearing of accumulated values so far IIRC is
> because such a _file_ will not be readable by existing versions of
> Git.  Am I correct?

I think there is another reason, which is that the interface we expose
to config callbacks (and via "config --get-all") is to sequentially pass
in all values. How does that interact with this "reset"? For example,
what is the output of:

  git config foo.bar one
  git -c '!foo.bar' config --get-all foo.bar

?

Do we continue to output the "reset" values, or do we quietly munge the
list on behalf of the caller? If the former, how do we represent that in
the output? I can see arguments both ways.

Implementation-wise (both for git-config and for internal callbacks), it
means we cannot parse the config as a single pass anymore. That's
probably OK; we've already moved partially toward that with the
configset stuff. If we _just_ support this via command-line options, we
could do an initial pass over those, looking for negatives, and then
simply skip all negatives while parsing the config files.

> If that is the case, then that reasoning will still not prevent us
> from adding corresponding support for a command-line overide, i.e.
> either one or both of these:
> 
> 	$ git -c credential.!helper cmd
> 	$ git -c !credential.helper cmd
> 
> no?

Yes, that would work, though to me it really feels like a
half-implemented feature. You cannot override a bad /etc/gitconfig line
via your ~/.gitconfig or repo-specific .git/config. Those things are
useful.

One other thing that occurred to me is that Apple Git hard-codes the
osxkeychain helper (rather than putting it into the system-wide
gitconfig <sigh>). No config-based system can "undo" that, but my patch
does. I admit that's probably not the best argument; hitting Apple with
a clue-stick is a cleaner approach.

> Of course, the code in the configuration subsystem for updated
> version of Git needs to become aware of the new syntax, and those
> that deal with the multi-value variables need custom code, which is
> similar to the way you special cased an empty value in the above
> patch, so I am not sure how much this would help.

I think you could get away without changing the users of the multi-value
variables, using the "negative" approach I mentioned above. Basically:

  1. parse GIT_CONFIG_PARAMETERS looking for negatives; stick them in a
     string-list or whatever.

  2. parse the files; look up each key in the string-list, and if it
     matches, don't even send it to the callback

  3. clear the string-list

  4. parse GIT_CONFIG_PARAMETERS again, ignoring any negatives

But like I said, that does feel somewhat half-implemented to me, since
it treats the command-line specially.

-Peff