[PATCH] Documentation: update [section.subsection] to reflect what git does

[PATCH] Documentation: update [section.subsection] to reflect what git does

[Carlos Martín Nieto]

Using the [section.subsection] syntax, the subsection is transformed
to lower-case and is matched case sensitively. Say so in the
documentation and mention that you shouldn't be using it anyway.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

This bit me recently when I was creating a parser. See Jeff's
explanation here:
http://thread.gmane.org/gmane.comp.version-control.git/179569/focus=180290

 Documentation/config.txt |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0658ffb..1212c47 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -45,9 +45,10 @@ lines.  Variables may belong directly to a section or to a given subsection.
 You can have `[section]` if you have `[section "subsection"]`, but you
 don't need to.
 
-There is also a case insensitive alternative `[section.subsection]` syntax.
-In this syntax, subsection names follow the same restrictions as for section
-names.
+There is also a deprecated `[section.subsection]` syntax. With this
+syntax, the subsection name is converted to lower-case and is also
+compared case sensitively. These subsection names follow the same
+restrictions as section names.
 
 All the other lines (and the remainder of the line after the section
 header) are recognized as setting variables, in the form
-- 
1.7.6.557.gcee4

Re: [PATCH] Documentation: update [section.subsection] to reflect what git does

[Jeff King]

On Wed, Oct 12, 2011 at 05:52:06PM +0200, Carlos Martín Nieto wrote:

> -There is also a case insensitive alternative `[section.subsection]` syntax.
> -In this syntax, subsection names follow the same restrictions as for section
> -names.
> +There is also a deprecated `[section.subsection]` syntax. With this
> +syntax, the subsection name is converted to lower-case and is also
> +compared case sensitively. These subsection names follow the same
> +restrictions as section names.

Hmm. While technically more correct, I think it is a little more
confusing to read. The lower-case canonicalization thing is actually
used for the other case-insensitive parts, too. So maybe it makes sense
to describe that in detail, and then just note that
"[section.subsection]" uses the same mechanism.

The patch below does this, and then the original text in the section you
tweaked above hopefully makes more sense in the new context.

The explanation matches what we do now, but it did end up a bit longer
than I had hoped. We could make it a lot shorter by:

  1. Canonicalizing the section and key names that the caller gives to
     git-config.

  2. Not mentioning the "section.foo" syntax. We can't canonicalize the
     subsection in (1) because of this syntax. But we can at least gloss
     over the detail, and then maybe just mention it much later in the
     file format. Or even deprecate it.

-- >8 --
Subject: [PATCH] docs/config: explain case-insensitive matching

We generally think of key matching as case-insensitive, but
it's not exactly. It's about canonicalizing one side, and
comparing it byte-wise with the canonical key name given to
git-config.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-config.txt |   50 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 49 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e7ecf5d..e92aee9 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -28,7 +28,7 @@ DESCRIPTION
 -----------
 You can query/set/replace/unset options with this command. The name is
 actually the section and the key separated by a dot, and the value will be
-escaped.
+escaped. See the section on name matching below.
 
 Multiple lines can be added to an option by using the '--add' option.
 If you want to update or unset an option which can occur on multiple
@@ -178,6 +178,54 @@ See also <<FILES>>.
 	Opens an editor to modify the specified config file; either
 	'--system', '--global', or repository (default).
 
+
+NAME MATCHING
+-------------
+
+Configuration key names are matched using an algorithm that allows for
+partial case sensitivity. Section and key names are read from the config
+files, canonicalized according to the rules below, and then compared
+case-sensitively with the input given to git-config. Therefore any
+callers to git-config should request the canonicalized version of the
+name. This typically means lowercasing the section and key names, and
+leaving the subsection (if any) intact. For example, ask for
+`git config core.eol`, not `git config CoRe.EOL`.
+
+The canonicalization rules are:
+
+1. Lowercase the section and key names.
+
+2. If a literal subsection (like `[section "foo"]`) is used, leave it
+   intact.
+
+3. If a non-literal subsection (like `[section.foo]`) is used, lowercase
+   the subsection.
+
+4. Concatenate the resulting section, subsection, and key, separated by
+   a dot ('.').
+
+For example, this configuration file:
+
+-----------------------------------------------
+[CORE]
+eol = true
+
+[branch "Foo"]
+REMOTE = origin
+
+[color.DIFF]
+new = blue
+-----------------------------------------------
+
+would yield the following three canonicalized names:
+
+-----------------------------------------------
+core.eol
+branch.Foo.remote
+color.diff.new
+-----------------------------------------------
+
+
 [[FILES]]
 FILES
 -----
-- 
1.7.7.rc2.21.gb9948

Re: [PATCH] Documentation: update [section.subsection] to reflect what git does

[Jeff King]

On Wed, Oct 12, 2011 at 12:29:39PM -0400, Jeff King wrote:

> The explanation matches what we do now, but it did end up a bit longer
> than I had hoped. We could make it a lot shorter by:
> 
>   1. Canonicalizing the section and key names that the caller gives to
>      git-config.

Hmm. Scratch that. We seem to do this already in my tests. I'll look
further and try to make a better documentation patch.

-Peff

Re: [PATCH] Documentation: update [section.subsection] to reflect what git does

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> This bit me recently when I was creating a parser. See Jeff's
> explanation here:
> http://thread.gmane.org/gmane.comp.version-control.git/179569/focus=180290

I think you meant to focus on 180123 but anyway I think the updated text reads
much better.

Thanks.

>  Documentation/config.txt |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0658ffb..1212c47 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -45,9 +45,10 @@ lines.  Variables may belong directly to a section or to a given subsection.
>  You can have `[section]` if you have `[section "subsection"]`, but you
>  don't need to.
>  
> -There is also a case insensitive alternative `[section.subsection]` syntax.
> -In this syntax, subsection names follow the same restrictions as for section
> -names.
> +There is also a deprecated `[section.subsection]` syntax. With this
> +syntax, the subsection name is converted to lower-case and is also
> +compared case sensitively. These subsection names follow the same
> +restrictions as section names.
>  
>  All the other lines (and the remainder of the line after the section
>  header) are recognized as setting variables, in the form

Re: [PATCH] Documentation: update [section.subsection] to reflect what git does

[Jeff King]

On Wed, Oct 12, 2011 at 01:46:43PM -0400, Jeff King wrote:

> On Wed, Oct 12, 2011 at 12:29:39PM -0400, Jeff King wrote:
> 
> > The explanation matches what we do now, but it did end up a bit longer
> > than I had hoped. We could make it a lot shorter by:
> > 
> >   1. Canonicalizing the section and key names that the caller gives to
> >      git-config.
> 
> Hmm. Scratch that. We seem to do this already in my tests. I'll look
> further and try to make a better documentation patch.

OK, I was all set to do a patch to git-config for this, but it seems the
code is already there.  It's only the subsections which are the sticking
point, and those can't be canonicalized, because in most cases we need
to match them exactly.

In the process, I did some cleanup and added some new tests to t1300,
which I think are probably worth applying anyway.

  [1/2]: t1300: put git invocations inside test function
  [2/2]: t1300: test mixed-case variable retrieval

-Peff

[PATCH 1/2] t1300: put git invocations inside test function

[Jeff King]

This is a very old script, and did a lot of:

  echo whatever >expect
  git config foo bar
  test_expect_success 'cmp .git/config expect'

which meant that we didn't actually check that the call to
git-config succeeded. Fix this, and while we're at it,
modernize the style to use test_cmp.

Signed-off-by: Jeff King <peff@peff.net>
---
There are still a few 'cp' and 'rm' calls outside of the test functions,
but we can generally expect those to work (as we do with the 'cat'
calls).

 t/t1300-repo-config.sh |  176 +++++++++++++++++++++++++-----------------------
 1 files changed, 93 insertions(+), 83 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3e140c1..cf508af 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -7,28 +7,28 @@ test_description='Test git config in different settings'
 
 . ./test-lib.sh
 
-test -f .git/config && rm .git/config
-
-git config core.penguin "little blue"
+test_expect_success 'clear default config' '
+	rm -f .git/config
+'
 
 cat > expect << EOF
 [core]
 	penguin = little blue
 EOF
-
-test_expect_success 'initial' 'cmp .git/config expect'
-
-git config Core.Movie BadPhysics
+test_expect_success 'initial' '
+	git config core.penguin "little blue" &&
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [core]
 	penguin = little blue
 	Movie = BadPhysics
 EOF
-
-test_expect_success 'mixed case' 'cmp .git/config expect'
-
-git config Cores.WhatEver Second
+test_expect_success 'mixed case' '
+	git config Core.Movie BadPhysics &&
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [core]
@@ -37,10 +37,10 @@ cat > expect << EOF
 [Cores]
 	WhatEver = Second
 EOF
-
-test_expect_success 'similar section' 'cmp .git/config expect'
-
-git config CORE.UPPERCASE true
+test_expect_success 'similar section' '
+	git config Cores.WhatEver Second
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [core]
@@ -50,8 +50,10 @@ cat > expect << EOF
 [Cores]
 	WhatEver = Second
 EOF
-
-test_expect_success 'similar section' 'cmp .git/config expect'
+test_expect_success 'uppercase section' '
+	git config CORE.UPPERCASE true &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'replace with non-match' \
 	'git config core.penguin kingpin !blue'
@@ -69,7 +71,7 @@ cat > expect << EOF
 	WhatEver = Second
 EOF
 
-test_expect_success 'non-match result' 'cmp .git/config expect'
+test_expect_success 'non-match result' 'test_cmp expect .git/config'
 
 cat > .git/config <<\EOF
 [alpha]
@@ -88,7 +90,7 @@ bar = foo
 [beta]
 EOF
 
-test_expect_success 'unset with cont. lines is correct' 'cmp .git/config expect'
+test_expect_success 'unset with cont. lines is correct' 'test_cmp expect .git/config'
 
 cat > .git/config << EOF
 [beta] ; silly comment # another comment
@@ -116,7 +118,7 @@ noIndent= sillyValue ; 'nother silly comment
 [nextSection] noNewline = ouch
 EOF
 
-test_expect_success 'multiple unset is correct' 'cmp .git/config expect'
+test_expect_success 'multiple unset is correct' 'test_cmp expect .git/config'
 
 cp .git/config2 .git/config
 
@@ -140,9 +142,7 @@ noIndent= sillyValue ; 'nother silly comment
 [nextSection] noNewline = ouch
 EOF
 
-test_expect_success 'all replaced' 'cmp .git/config expect'
-
-git config beta.haha alpha
+test_expect_success 'all replaced' 'test_cmp expect .git/config'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -153,10 +153,10 @@ noIndent= sillyValue ; 'nother silly comment
 	haha = alpha
 [nextSection] noNewline = ouch
 EOF
-
-test_expect_success 'really mean test' 'cmp .git/config expect'
-
-git config nextsection.nonewline wow
+test_expect_success 'really mean test' '
+	git config beta.haha alpha &&
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -168,11 +168,12 @@ noIndent= sillyValue ; 'nother silly comment
 [nextSection]
 	nonewline = wow
 EOF
-
-test_expect_success 'really really mean test' 'cmp .git/config expect'
+test_expect_success 'really really mean test' '
+	git config nextsection.nonewline wow &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'get value' 'test alpha = $(git config beta.haha)'
-git config --unset beta.haha
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -183,10 +184,10 @@ noIndent= sillyValue ; 'nother silly comment
 [nextSection]
 	nonewline = wow
 EOF
-
-test_expect_success 'unset' 'cmp .git/config expect'
-
-git config nextsection.NoNewLine "wow2 for me" "for me$"
+test_expect_success 'unset' '
+	git config --unset beta.haha &&
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -198,8 +199,10 @@ noIndent= sillyValue ; 'nother silly comment
 	nonewline = wow
 	NoNewLine = wow2 for me
 EOF
-
-test_expect_success 'multivar' 'cmp .git/config expect'
+test_expect_success 'multivar' '
+	git config nextsection.NoNewLine "wow2 for me" "for me$" &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'non-match' \
 	'git config --get nextsection.nonewline !for'
@@ -214,8 +217,6 @@ test_expect_success 'ambiguous get' '
 test_expect_success 'get multivar' \
 	'git config --get-all nextsection.nonewline'
 
-git config nextsection.nonewline "wow3" "wow$"
-
 cat > expect << EOF
 [beta] ; silly comment # another comment
 noIndent= sillyValue ; 'nother silly comment
@@ -226,8 +227,10 @@ noIndent= sillyValue ; 'nother silly comment
 	nonewline = wow3
 	NoNewLine = wow2 for me
 EOF
-
-test_expect_success 'multivar replace' 'cmp .git/config expect'
+test_expect_success 'multivar replace' '
+	git config nextsection.nonewline "wow3" "wow$" &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'ambiguous value' '
 	test_must_fail git config nextsection.nonewline
@@ -241,8 +244,6 @@ test_expect_success 'invalid unset' '
 	test_must_fail git config --unset somesection.nonewline
 '
 
-git config --unset nextsection.nonewline "wow3$"
-
 cat > expect << EOF
 [beta] ; silly comment # another comment
 noIndent= sillyValue ; 'nother silly comment
@@ -253,7 +254,10 @@ noIndent= sillyValue ; 'nother silly comment
 	NoNewLine = wow2 for me
 EOF
 
-test_expect_success 'multivar unset' 'cmp .git/config expect'
+test_expect_success 'multivar unset' '
+	git config --unset nextsection.nonewline "wow3$" &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'invalid key' 'test_must_fail git config inval.2key blabla'
 
@@ -276,7 +280,7 @@ noIndent= sillyValue ; 'nother silly comment
 	Alpha = beta
 EOF
 
-test_expect_success 'hierarchical section value' 'cmp .git/config expect'
+test_expect_success 'hierarchical section value' 'test_cmp expect .git/config'
 
 cat > expect << EOF
 beta.noindent=sillyValue
@@ -304,15 +308,16 @@ EOF
 test_expect_success '--get-regexp' \
 	'git config --get-regexp in > output && cmp output expect'
 
-git config --add nextsection.nonewline "wow4 for you"
-
 cat > expect << EOF
 wow2 for me
 wow4 for you
 EOF
 
-test_expect_success '--add' \
-	'git config --get-all nextsection.nonewline > output && cmp output expect'
+test_expect_success '--add' '
+	git config --add nextsection.nonewline "wow4 for you" &&
+	git config --get-all nextsection.nonewline > output &&
+	test_cmp expect output
+'
 
 cat > .git/config << EOF
 [novalue]
@@ -361,8 +366,6 @@ cat > .git/config << EOF
 	c = d
 EOF
 
-git config a.x y
-
 cat > expect << EOF
 [a.b]
 	c = d
@@ -370,10 +373,10 @@ cat > expect << EOF
 	x = y
 EOF
 
-test_expect_success 'new section is partial match of another' 'cmp .git/config expect'
-
-git config b.x y
-git config a.b c
+test_expect_success 'new section is partial match of another' '
+	git config a.x y &&
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [a.b]
@@ -385,7 +388,11 @@ cat > expect << EOF
 	x = y
 EOF
 
-test_expect_success 'new variable inserts into proper section' 'cmp .git/config expect'
+test_expect_success 'new variable inserts into proper section' '
+	git config b.x y &&
+	git config a.b c &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' \
 	'test_must_fail git config --file non-existing-config -l'
@@ -399,9 +406,10 @@ cat > expect << EOF
 ein.bahn=strasse
 EOF
 
-GIT_CONFIG=other-config git config -l > output
-
-test_expect_success 'alternative GIT_CONFIG' 'cmp output expect'
+test_expect_success 'alternative GIT_CONFIG' '
+	GIT_CONFIG=other-config git config -l >output &&
+	test_cmp expect output
+'
 
 test_expect_success 'alternative GIT_CONFIG (--file)' \
 	'git config --file other-config -l > output && cmp output expect'
@@ -417,8 +425,6 @@ test_expect_success 'refer config from subdirectory' '
 
 '
 
-GIT_CONFIG=other-config git config anwohner.park ausweis
-
 cat > expect << EOF
 [ein]
 	bahn = strasse
@@ -426,7 +432,10 @@ cat > expect << EOF
 	park = ausweis
 EOF
 
-test_expect_success '--set in alternative GIT_CONFIG' 'cmp other-config expect'
+test_expect_success '--set in alternative GIT_CONFIG' '
+	GIT_CONFIG=other-config git config anwohner.park ausweis &&
+	test_cmp expect other-config
+'
 
 cat > .git/config << EOF
 # Hallo
@@ -531,7 +540,7 @@ test_expect_success 'section ending' '
 	git config gitcvs.enabled true &&
 	git config gitcvs.ext.dbname %Ggitcvs1.%a.%m.sqlite &&
 	git config gitcvs.dbname %Ggitcvs2.%a.%m.sqlite &&
-	cmp .git/config expect
+	test_cmp expect .git/config
 
 '
 
@@ -750,13 +759,6 @@ test_expect_success NOT_MINGW 'get --path copes with unset $HOME' '
 	test_cmp expect result
 '
 
-rm .git/config
-
-git config quote.leading " test"
-git config quote.ending "test "
-git config quote.semicolon "test;test"
-git config quote.hash "test#test"
-
 cat > expect << EOF
 [quote]
 	leading = " test"
@@ -764,8 +766,14 @@ cat > expect << EOF
 	semicolon = "test;test"
 	hash = "test#test"
 EOF
-
-test_expect_success 'quoting' 'cmp .git/config expect'
+test_expect_success 'quoting' '
+	rm .git/config &&
+	git config quote.leading " test" &&
+	git config quote.ending "test " &&
+	git config quote.semicolon "test;test" &&
+	git config quote.hash "test#test" &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'key with newline' '
 	test_must_fail git config "key.with
@@ -790,9 +798,10 @@ section.noncont=not continued
 section.quotecont=cont;inued
 EOF
 
-git config --list > result
-
-test_expect_success 'value continued on next line' 'cmp result expect'
+test_expect_success 'value continued on next line' '
+	git config --list > result &&
+	cmp result expect
+'
 
 cat > .git/config <<\EOF
 [section "sub=section"]
@@ -813,16 +822,17 @@ barQsection.sub=section.val3
 Qsection.sub=section.val4
 Qsection.sub=section.val5Q
 EOF
+test_expect_success '--null --list' '
+	git config --null --list | nul_to_q >result &&
+	echo >>result &&
+	test_cmp expect result
+'
 
-git config --null --list | perl -pe 'y/\000/Q/' > result
-echo >>result
-
-test_expect_success '--null --list' 'cmp result expect'
-
-git config --null --get-regexp 'val[0-9]' | perl -pe 'y/\000/Q/' > result
-echo >>result
-
-test_expect_success '--null --get-regexp' 'cmp result expect'
+test_expect_success '--null --get-regexp' '
+	git config --null --get-regexp "val[0-9]" | nul_to_q >result &&
+	echo >>result &&
+	test_cmp expect result
+'
 
 test_expect_success 'inner whitespace kept verbatim' '
 	git config section.val "foo 	  bar" &&
-- 
1.7.7.rc2.21.gb9948

[PATCH 2/2] t1300: test mixed-case variable retrieval

[Jeff King]

We should be able to ask for a config value both by its
canonical all-lowercase name (as git does internally), as
well as by random mixed-case (which will be canonicalized by
git-config for us).

Subsections are a tricky point, though. Since we have both

  [section "Foo"]

and

  [section.Foo]

you might want git-config to canonicalize the subsection or
not, depending on which you are expecting. But there's no
way to communicate this; git-config sees only the key, and
doesn't know which type of section name will be in the
config file.

So it must leave the subsection intact, and it is up to the
caller to provide a canonical version of the subsection if
they want to match the latter form.

Signed-off-by: Jeff King <peff@peff.net>
---
I was surprised this wasn't tested anywhere, but I couldn't find any
such place. I think it makes sense to document the desired behavior in
the form of a few tests.

 t/t1300-repo-config.sh |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index cf508af..8a37f96 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -73,6 +73,33 @@ EOF
 
 test_expect_success 'non-match result' 'test_cmp expect .git/config'
 
+test_expect_success 'find mixed-case key by canonical name' '
+	echo Second >expect &&
+	git config cores.whatever >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find mixed-case key by non-canonical name' '
+	echo Second >expect &&
+	git config CoReS.WhAtEvEr >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'subsections are not canonicalized by git-config' '
+	cat >>.git/config <<-\EOF &&
+	[section.SubSection]
+	key = one
+	[section "SubSection"]
+	key = two
+	EOF
+	echo one >expect &&
+	git config section.subsection.key >actual &&
+	test_cmp expect actual &&
+	echo two >expect &&
+	git config section.SubSection.key >actual &&
+	test_cmp expect actual
+'
+
 cat > .git/config <<\EOF
 [alpha]
 bar = foo
-- 
1.7.7.rc2.21.gb9948

Re: [PATCH] Documentation: update [section.subsection] to reflect what git does

[Jeff King]

On Wed, Oct 12, 2011 at 05:52:06PM +0200, Carlos Martín Nieto wrote:

> -There is also a case insensitive alternative `[section.subsection]` syntax.
> -In this syntax, subsection names follow the same restrictions as for section
> -names.
> +There is also a deprecated `[section.subsection]` syntax. With this
> +syntax, the subsection name is converted to lower-case and is also
> +compared case sensitively. These subsection names follow the same
> +restrictions as section names.

OK, now having looked thoroughly at the problem again, I agree that your
documentation update is much better than the one I posted earlier
(technically we could still talk about canonicalizing the subsection
earlier in git-config(1), but section.foo names are the deprecated
minority, so it's probably not worth cluttering the page with
explanations).

I do think the "compared case sensitively" bit is a little confusing
here, because it's not clear what is being compared here (as we're quite
deep into a discussion of the file format, and away from the git-config
usage). It might be more clear to say:

  With this syntax, the subsection name is converted to lower-case, and
  the result is compared case sensitively against the subsection name
  provided to git-config.

or something like that.

-Peff

Re: [PATCH 2/2] t1300: test mixed-case variable retrieval

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I was surprised this wasn't tested anywhere, but I couldn't find any
> such place. I think it makes sense to document the desired behavior in
> the form of a few tests.

Thanks. The patch looks good.

But oh boy the original test in the old style was ugly....

Re: [PATCH 1/2] t1300: put git invocations inside test function

[Johannes Sixt]

Am 10/12/2011 20:29, schrieb Jeff King:
> @@ -750,13 +759,6 @@ test_expect_success NOT_MINGW 'get --path copes with unset $HOME' '
>  	test_cmp expect result
>  '
>  
> -rm .git/config
> -
> -git config quote.leading " test"
> -git config quote.ending "test "
> -git config quote.semicolon "test;test"
> -git config quote.hash "test#test"
> -
>  cat > expect << EOF
>  [quote]
>  	leading = " test"
> @@ -764,8 +766,14 @@ cat > expect << EOF
>  	semicolon = "test;test"
>  	hash = "test#test"
>  EOF
> -
> -test_expect_success 'quoting' 'cmp .git/config expect'
> +test_expect_success 'quoting' '
> +	rm .git/config &&
> +	git config quote.leading " test" &&
> +	git config quote.ending "test " &&
> +	git config quote.semicolon "test;test" &&
> +	git config quote.hash "test#test" &&
> +	test_cmp expect .git/config
> +'

This innocently looking hunk fails on Windows, because the preceding tests
are skipped, and .git/config does not exist. I was tempted to just change
this to 'rm -f'. But there are a few other instances of 'rm .git/config'
in this file that were *not* moved inside the test function.

How would you like this solved?

- Move this one out again
- Add -f to just this one
- Add -f everywhere
- a combination of the above?

-- Hannes

Re: [PATCH 1/2] t1300: put git invocations inside test function

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> This innocently looking hunk fails on Windows, because the preceding tests
> are skipped, and .git/config does not exist. I was tempted to just change
> this to 'rm -f'. But there are a few other instances of 'rm .git/config'
> in this file that were *not* moved inside the test function.
>
> How would you like this solved?
>
> - Move this one out again
> - Add -f to just this one
> - Add -f everywhere
> - a combination of the above?

Probably "rm -f .git/config" to all tests would be the most appropriate,
as the desire to start from an empty config file is a sign that each test
wants to be as independent as possible from previous tests.

An alternative approach to achieve the independence would be to make each
test responsible for cleaning up after itself, with test_when_finished,
but that won't work for the first one, as the test framework would give
you the original one. Besides, that's trying to be cooperative when each
test can fail in an unexpected way (after all the purpose of tests is to
fail when things go wrong). In order to achieve isolation, each test
protecting themselves from others, as long as it can be easily and
reasonably done (e.g. a single "rm -f"), is probably far more preferrable
than each test trying to clean after itself, risking possible failure to
do so.

[PATCH] t1300: attempting to remove a non-existent .git/config is not an error

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

Since some tests before test number 79 ("quoting") are skipped, .git/config
does not exist and 'rm .git/config' fails. Fix this particular case.

While at it, move other instance of 'rm .git/config' that occur in this
file inside the test function to document that the test cases want to
protect themselves from remnants of earlier tests.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 You may think that the justification of this change would be the
 other way round: protect test cases, then incidentally this fixes
 a failure on Windows. But for me it is as described: The motivation
 is the fix for Windows, and without that, the "protect test cases"
 part would not have happened. :-)

 t/t1300-repo-config.sh |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index fba5ae0..51caff0 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -558,8 +558,6 @@ EOF
 test_expect_success "section was removed properly" \
 	"test_cmp expect .git/config"
 
-rm .git/config
-
 cat > expect << EOF
 [gitcvs]
 	enabled = true
@@ -570,6 +568,7 @@ EOF
 
 test_expect_success 'section ending' '
 
+	rm -f .git/config &&
 	git config gitcvs.enabled true &&
 	git config gitcvs.ext.dbname %Ggitcvs1.%a.%m.sqlite &&
 	git config gitcvs.dbname %Ggitcvs2.%a.%m.sqlite &&
@@ -642,8 +641,6 @@ test_expect_success 'invalid bool (set)' '
 
 	test_must_fail git config --bool bool.nobool foobar'
 
-rm .git/config
-
 cat > expect <<\EOF
 [bool]
 	true1 = true
@@ -658,6 +655,7 @@ EOF
 
 test_expect_success 'set --bool' '
 
+	rm -f .git/config &&
 	git config --bool bool.true1 01 &&
 	git config --bool bool.true2 -1 &&
 	git config --bool bool.true3 YeS &&
@@ -668,8 +666,6 @@ test_expect_success 'set --bool' '
 	git config --bool bool.false4 FALSE &&
 	cmp expect .git/config'
 
-rm .git/config
-
 cat > expect <<\EOF
 [int]
 	val1 = 1
@@ -679,13 +675,12 @@ EOF
 
 test_expect_success 'set --int' '
 
+	rm -f .git/config &&
 	git config --int int.val1 01 &&
 	git config --int int.val2 -1 &&
 	git config --int int.val3 5m &&
 	cmp expect .git/config'
 
-rm .git/config
-
 cat >expect <<\EOF
 [bool]
 	true1 = true
@@ -699,6 +694,7 @@ cat >expect <<\EOF
 EOF
 
 test_expect_success 'get --bool-or-int' '
+	rm -f .git/config &&
 	(
 		echo "[bool]"
 		echo true1
@@ -718,7 +714,6 @@ test_expect_success 'get --bool-or-int' '
 
 '
 
-rm .git/config
 cat >expect <<\EOF
 [bool]
 	true1 = true
@@ -732,6 +727,7 @@ cat >expect <<\EOF
 EOF
 
 test_expect_success 'set --bool-or-int' '
+	rm -f .git/config &&
 	git config --bool-or-int bool.true1 true &&
 	git config --bool-or-int bool.false1 false &&
 	git config --bool-or-int bool.true2 yes &&
@@ -742,8 +738,6 @@ test_expect_success 'set --bool-or-int' '
 	test_cmp expect .git/config
 '
 
-rm .git/config
-
 cat >expect <<\EOF
 [path]
 	home = ~/
@@ -752,6 +746,7 @@ cat >expect <<\EOF
 EOF
 
 test_expect_success NOT_MINGW 'set --path' '
+	rm -f .git/config &&
 	git config --path path.home "~/" &&
 	git config --path path.normal "/dev/null" &&
 	git config --path path.trailingtilde "foo~" &&
@@ -800,7 +795,7 @@ cat > expect << EOF
 	hash = "test#test"
 EOF
 test_expect_success 'quoting' '
-	rm .git/config &&
+	rm -f .git/config &&
 	git config quote.leading " test" &&
 	git config quote.ending "test " &&
 	git config quote.semicolon "test;test" &&
-- 
1.7.7.1507.g94722

Re: [PATCH] t1300: attempting to remove a non-existent .git/config is not an error

[Jeff King]

On Wed, Oct 19, 2011 at 09:37:06AM +0200, Johannes Sixt wrote:

> From: Johannes Sixt <j6t@kdbg.org>
> 
> Since some tests before test number 79 ("quoting") are skipped, .git/config
> does not exist and 'rm .git/config' fails. Fix this particular case.
> 
> While at it, move other instance of 'rm .git/config' that occur in this
> file inside the test function to document that the test cases want to
> protect themselves from remnants of earlier tests.

Thanks, this fix looks good to me.

-Peff