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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html