patchAdd test case for git-config-set

... includes the mean tests I mentioned on the list.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

 t1300-config-set.sh |  180 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 180 insertions(+), 0 deletions(-)
 create mode 100644 t1300-config-set.sh

applies-to: de5d2a42ebd69e3608e0f99f615fbcf55a88c101 3fa048df0d691d3423ec81f8dba947e2c05fa996

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I am not so sure about this case.  Shouldn't this remove both?

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 '^'

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.

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

Maybe we could have the shell-level interface like this:

	git-config-set [--remove rx] section.key [value...]

When --remove rx is specified, the command first removes existing multivalue for the given section.key that match rx, and then insert given value(s); not giving any values amounts to --unset. Not giving --remove rx is the same as giving a regexp that matches all multivalues. So the simplest:

	git-config-set section.key value

becomes a single-value assignment (insert-or-replace),

	git-config-set section.key

confusingly enough is --unset (we probably would want to require an explicit command line noise-word "--unset" in this case). And "replacing with two values regardless of whatever there are currently" naturally becomes:

	git-config-set pull.twohead recursive resolve

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.

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

Junio C Hamano <junkio@cox.net> writes:

This is too confusing, so I think we should require --unset. I think the C-interface I suggested in the previous message can stay the same, but from the command line perspective, we should have a bit easier syntax.

A revised suggestion is:

	;# remove all
	git-config-set --unset section.key

	;# remove values that match rx and then append zero or more values
	git-config-set --remove rx section.key [value...]

	;# append one or more values (equivalent to specifying --remove
        ;# with rx that never matches anything).  To reduce
        ;# confusion, we always require at least one value here.
	git-config-set section.key value [value...]

	;# Additionally, purely as a syntax sugar, replace the
	;# entire multivalue with one or more values
	;# (equivalent to saying --remove '^').
	git-config-set --replace section.key value [value...]

I think (aside from "*-set" now becomes confusing), showing the value of the specified key to stdout with

	git-config-set section.key

would be a nice addition to complete the suite; has anybody noticed that git-var is cumbersome to use for this?

Hi,

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

As mentioned in my previous mail, this will go.

No. Just remove one unique entry. If people insist it is a good idea to throw away all multivars, I'll think about it. But I am not convinced. Your use case of a multivar does not fit what I think multivars are useful for.

IMHO, multiple key/value pairs are only sensible when they provide a sort of subkey, like Linus did in his example:

	[proxy]
		command=ssh for kernel.org
		command=rsh for myprivate.machine

See, there is a subkey, namely "for kernel.org", which can nicely be expressed as a regex.

IMOHO, your example

	[diff]
		twohead = resolve
		twohead = recursive

would look much better in this way

	[diff]
		twohead = recursive resolve

Several reasons:

	- it is easier to read
	- it does the job
	- it is clear from the beginning, which one precedes the other
	  (for me it was not at all clear that the last entry wins...)

As I said. *If*, then these must be two operations, for your security. But I still don't see a sensible use case of removing all key/value pairs for a certain key (and possibly a regex for the values).

Rather, if you have a sensible setup, you'll want to set/replace/unset exactly one entry.

I think it is less error prone to add them one by one, else you have to check the values for uniqueness, too.

Good point! This will be in my next version.

Ciao, Dscho

Hi,

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

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.

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

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.

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

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

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

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

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