Re: [PATCH] Add test case for git-config-set

Hi,

On Thu, 17 Nov 2005, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > +test_expect_failure 'ambiguous unset' \
> > +	'git-config-set --unset nextsection.nonewline'
> 
> I am not so sure about this case.  Shouldn't this remove both?

No. The common case should be a variable with a unique key/value pair. And 
in that case, it should be treated as an error if there are already more 
than one pair.

> For example, if a Porcelain wants to force pull.twohead to be
> resolve and nothing else, and it wants to do it unconditionally,
> it would first want to empty whatever multivalue there are
> currently, and then insert its own, and I'd imagine the way to
> say that would be like this:
> 
> 	git-config-set --unset pull.twohead '^'
>         git-config-set pull.twohead resolve
> 
> More simply (I do not think you have a test case for this):
> 
>         git-config-set pull.twohead resolve '^'

This is a slippery slope. Giving a wrong regex by accident may destroy 
*all* your settings. Not what I want.

> I think it is the easiest to explain and understand the
> semantics of config_set_multivalue if it were to first remove all
> existing key-value for matching ones, and then insert what was
> provided by the user.

I'd be very, very careful about that. For sure, I'd want to make the 
removing of multiple values a single operation, which leaves a backup of 
.git/config behind.

> Extending that multivalue example a bit more, I think it is a
> bit cumbersome for a Porcelain to set pull.twohead to recursive
> and then resolve, with your interface.  Even if you had the
> emptying behaviour I suggested above, you would have to say
> something awkard like this:
> 
> 	git-config-set --unset pull.twohead '^'
>         git-config-set pull.twohead recursive
>         git-config-set pull.twohead resolve no-such-value-should-be-there

Really, I don't see the point in making twohead a multivar:

	[diff]
		twohead = resolve recursive blabla

This looks much nicer to me, and should be easy to parse (even for human 
eyes, who do not particularly like to find one value at the top of 
.git/config, and the second on the bottom, which is perfectly possible in 
your setup).

> Maybe we could have the shell-level interface like this:
> 
> 	git-config-set [--remove rx] section.key [value...]

As I said, I'd be *very* careful to remove multiple values. So, I would 
like to *require* two steps for this.

> 	git-config-set section.key
> 
> confusingly enough is --unset

Okay. That was my original code before introducing multivars, where I did 
not have --unset. This will go.

> 	git-config-set pull.twohead recursive resolve

Oh yes, yes. Just quote "recursive resolve"! Note that it is somewhat 
uncommon to make the behaviour of a variable in an ini file depend on its 
position... (this is meant as one more point in favor of just one pair).

> The C-level interface would become something like:
> 
> 	git_config_set_multivar(const char *key,
>         			const char *remove_value_regex,
>                                 const char **values)
> 
> where values is a NULL terminated list of values.

The normal use case (for things like "proxy.command", which will hopefully 
not become very common) will be to set/replace/unset exactly one value. 
For what you want, your proposal might be good, but I deem a unique 
key/value pair with a space separated list of methods a better solution.

> BTW, do we want to remove the section after removing the last
> key and making it empty?

No, because you might have comments in there. I am not willing to go as 
far as removing something like

	[diff]
		;might want to use that someday
		;twohead = recursive megacool

Ciao,
Dscho

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html