Re: [PATCH] credential: let empty credential specs reset helper list
- From
Jeff King <peff@peff.net>
- Date
- Feb 26, 2016, 22:37 UTC
- Message-ID
- <20160226223719.GA2429@sigill.intra.peff.net>
- In-Reply-To
- <xmqqk2lrjmff.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 26, 2016 at 11:34:12AM -0800, Junio C Hamano wrote:
Show 29 quoted lines
> >> - 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.
Show 8 quoted lines
> 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.
Show 5 quoted lines
> 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 callback3. 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