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

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

[Johannes Schindelin]

... 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
diff --git a/t1300-config-set.sh b/t1300-config-set.sh
new file mode 100644
index 0000000..df89216
--- /dev/null
+++ b/t1300-config-set.sh
@@ -0,0 +1,180 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Johannes Schindelin
+#
+
+test_description='Test git-config-set in different settings'
+
+. ./test-lib.sh
+
+test -f .git/config && rm .git/config
+
+git-config-set core.penguin "little blue"
+
+cat > expect << EOF
+#
+# This is the config file
+#
+
+[core]
+	penguin = little blue
+EOF
+
+test_expect_success 'initial' 'cmp .git/config expect'
+
+git-config-set Core.Movie BadPhysics
+
+cat > expect << EOF
+#
+# This is the config file
+#
+
+[core]
+	penguin = little blue
+	Movie = BadPhysics
+EOF
+
+test_expect_success 'mixed case' 'cmp .git/config expect'
+
+git-config-set Cores.WhatEver Second
+
+cat > expect << EOF
+#
+# This is the config file
+#
+
+[core]
+	penguin = little blue
+	Movie = BadPhysics
+[Cores]
+	WhatEver = Second
+EOF
+
+test_expect_success 'similar section' 'cmp .git/config expect'
+
+git-config-set CORE.UPPERCASE true
+
+cat > expect << EOF
+#
+# This is the config file
+#
+
+[core]
+	penguin = little blue
+	Movie = BadPhysics
+	UPPERCASE = true
+[Cores]
+	WhatEver = Second
+EOF
+
+test_expect_success 'similar section' 'cmp .git/config expect'
+
+cat > .git/config << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; 'nother silly comment
+
+# empty line
+		; comment
+		haha   ="beta" # last silly comment
+[nextSection] noNewline = ouch
+EOF
+
+git-config-set beta.haha alpha
+
+cat > expect << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; 'nother silly comment
+
+# empty line
+		; comment
+	haha = alpha
+[nextSection] noNewline = ouch
+EOF
+
+test_expect_success 'really mean test' 'cmp .git/config expect'
+
+git-config-set nextsection.nonewline wow
+
+cat > expect << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; 'nother silly comment
+
+# empty line
+		; comment
+	haha = alpha
+[nextSection]
+	nonewline = wow
+EOF
+
+test_expect_success 'really really mean test' 'cmp .git/config expect'
+
+git-config-set beta.haha
+
+cat > expect << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; 'nother silly comment
+
+# empty line
+		; comment
+[nextSection]
+	nonewline = wow
+EOF
+
+test_expect_success 'unset' 'cmp .git/config expect'
+
+git-config-set nextsection.NoNewLine "wow2 for me" "for me$"
+
+cat > expect << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; 'nother silly comment
+
+# empty line
+		; comment
+[nextSection]
+	nonewline = wow
+	NoNewLine = wow2 for me
+EOF
+
+test_expect_success 'multivar' 'cmp .git/config expect'
+
+git-config-set nextsection.nonewline "wow3" "wow$"
+
+cat > expect << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; 'nother silly comment
+
+# empty line
+		; comment
+[nextSection]
+	nonewline = wow3
+	NoNewLine = wow2 for me
+EOF
+
+test_expect_success 'multivar replace' 'cmp .git/config expect'
+
+test_expect_failure 'ambiguous unset' \
+	'git-config-set --unset nextsection.nonewline'
+
+test_expect_failure 'invalid unset' \
+	'git-config-set --unset somesection.nonewline'
+
+git-config-set --unset nextsection.nonewline "wow3$"
+
+cat > expect << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; 'nother silly comment
+
+# empty line
+		; comment
+[nextSection]
+	NoNewLine = wow2 for me
+EOF
+
+test_expect_success 'multivar unset' 'cmp .git/config expect'
+
+test_expect_failure 'invalid key' 'git-config-set inval.2key blabla'
+
+test_expect_success 'correct key' 'git-config-set 123456.a123 987'
+
+test_done
+
---
0.99.9.GIT

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

[Junio C Hamano]

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?

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?

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

[Junio C Hamano]

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

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

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?

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

Hi,

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

> Junio C Hamano <junkio@cox.net> writes:
> 
> > 	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).

As mentioned in my previous mail, this will go.

> A revised suggestion is:
> 
> 	;# remove all
> 	git-config-set --unset section.key

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

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

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.

> 	;# 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...]

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

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

Good point! This will be in my next version.

Ciao,
Dscho