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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 18, 2005, 10:48 UTC
Message-ID: <Pine.LNX.4.63.0511181136400.8037@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <7vd5kypjv2.fsf@assigned-by-dhcp.cox.net>

Hi,

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

Show 6 quoted lines

> 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.

Show 12 quoted lines

> 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.

Show 9 quoted lines

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

Show 7 quoted lines

> 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