patchcredential: let empty credential specs reset helper list

Sine the credential.helper key is a multi-valued config list, there's no way to "unset" a helper once it's been set. So if your system /etc/gitconfig sets one, you can never avoid running it, but only add your own helpers on top.

Since an empty value for credential.helper is nonsensical (it would just try to run "git-credential-"), we can assume nobody is using it. Let's define it to reset the helper list, letting you override lower-priority instances which have come before.

Signed-off-by: Jeff King <peff@peff.net>
---
This doesn't solve the multi-value config problem in the general sense,
but it's simple and easy. And even if we do solve it later, it's not a
big deal to keep supporting this scheme for compatibility.

 Documentation/config.txt         |  5 +++--
 Documentation/gitcredentials.txt |  5 +++++
 credential.c                     |  9 ++++++---
 t/t0300-credentials.sh           | 11 +++++++++++
 4 files changed, 25 insertions(+), 5 deletions(-)

Jeff King <peff@peff.net> writes:

s/Sine/Since/;

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 ;-)

Junio C Hamano <gitster@pobox.com> writes:

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?

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?

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.

On Fri, Feb 26, 2016 at 11:34 AM, Junio C Hamano <gitster@pobox.com> wrote:

For most cases, I think the previous approach of setting to empty string makes sense and is more easilly intuitive, but would only work for those values which the empty string has no valid meaning.

I would think either approach here is fine, and i think *most* cases of multivariable configuration don't need to be cleared from within a configuration file but only on the command line would be acceptable.

If we go this route I would prefer "-c !<config>" vs "-c <namespace>.!<config>"

Thanks, Jake

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

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.

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.

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

Jeff King <peff@peff.net> writes:

The callback API needs to be extended to include this "reset" bit, not just "var" and "value", obviously. And then of course the users of the API need to be updated to act on that "reset" thing.

That is what I meant by "multi-value things need custom code". The single-valued ones can do "last one wins", and "reset" can be turned into "the initial state" assignment for the variable; multi-value cumulative ones already have custom code to accumulate things into list, and they need to learn to empty the list upon seeing the "reset" bit.

But that is not as fundamental as "new syntax breaks old clients". If we want our new code to do useful things, obviously we would need to work on making it do so ;-)

I agree with you that that is a kludge and not the right way forward.