[PATCH 1/2] t/README: document test_config

[PATCH 1/2] t/README: document test_config

[Firmin Martin]

test_config is used over one thousand times in the test suite, yet not
documented. Give it a place in the "Test harness library" section.

Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 t/README | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/README b/t/README
index 8eb9e46b1d..f69aa05c61 100644
--- a/t/README
+++ b/t/README
@@ -1046,6 +1046,21 @@ library for your script to use.
    Abort the test script if either the value of the variable or the
    default are not valid bool values.
 
+ - test_config <config-option> [<value>]
+
+   Set the configuration option <config-option> to <value>, and unset it at the
+   end of the current test. For a similar purpose, test_config_global for
+   global configuration is also available. Note, however, that test_config_*
+   should not be used under a subshell.
+  
+   Example:
+
+	test_config format.coverLetter auto
+
+   Is a concise way to write:
+	test_when_finished "git config --unset format.coverLetter" &&
+	git config format.coverLetter auto
+
 
 Prerequisites
 -------------
-- 
2.31.1.443.g67a420f573

[PATCH 2/2] t: use test_config whenever possible

[Firmin Martin]

Replace patterns of the form
1.
        git config <config-option> <value> &&
        ...
        git config --unset <config-option> &&
2.
        test_when_finished "git config --unset <config-option>" &&
        git config <config-option> <value> &&
        ...

to the concise

        test_config <config-option> <value>

In t5526, two tests have been further simplified as the output file is
written before "git config --global --unset".

Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 t/t3200-branch.sh                |  9 +++------
 t/t3900-i18n-commit.sh           |  3 +--
 t/t4027-diff-submodule.sh        |  3 +--
 t/t4041-diff-submodule-option.sh |  3 +--
 t/t4205-log-pretty-formats.sh    |  8 +++-----
 t/t5505-remote.sh                |  3 +--
 t/t5526-fetch-submodules.sh      | 16 ++++------------
 t/t6006-rev-list-format.sh       |  5 ++---
 t/t7401-submodule-summary.sh     |  3 +--
 t/t7610-mergetool.sh             |  2 +-
 t/t7900-maintenance.sh           |  9 +++------
 11 files changed, 21 insertions(+), 43 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cc4b10236e..0b0119bbe2 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -365,11 +365,9 @@ EOF
 '
 
 test_expect_success 'git branch with column.*' '
-	git config column.ui column &&
-	git config column.branch "dense" &&
+	test_config column.ui column &&
+	test_config column.branch "dense" &&
 	COLUMNS=80 git branch >actual &&
-	git config --unset column.branch &&
-	git config --unset column.ui &&
 	cat >expect <<\EOF &&
   a/b/c   bam   foo   l   * main   n     o/p   r
   abc     bar   j/k   m/m   mb     o/o   q     topic
@@ -382,9 +380,8 @@ test_expect_success 'git branch --column -v should fail' '
 '
 
 test_expect_success 'git branch -v with column.ui ignored' '
-	git config column.ui column &&
+	test_config column.ui column &&
 	COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
-	git config --unset column.ui &&
 	cat >expect <<\EOF &&
   a/b/c
   abc
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index bfab245eb3..c16c0f7fba 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -179,7 +179,7 @@ test_commit_autosquash_flags () {
 	H=$1
 	flag=$2
 	test_expect_success "commit --$flag with $H encoding" '
-		git config i18n.commitencoding $H &&
+		test_config i18n.commitencoding $H &&
 		git checkout -b $H-$flag C0 &&
 		echo $H >>F &&
 		git commit -a -F "$TEST_DIRECTORY"/t3900/$H.txt &&
@@ -193,7 +193,6 @@ test_commit_autosquash_flags () {
 		E=$(git cat-file commit '$H-$flag' |
 			sed -ne "s/^encoding //p") &&
 		test "z$E" = "z$H" &&
-		git config --unset-all i18n.commitencoding &&
 		git rebase --autosquash -i HEAD^^^ &&
 		git log --oneline >actual &&
 		test_line_count = 3 actual
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 94ef77e1df..a6eb2416ed 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -123,7 +123,7 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'
 '
 
 test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '
-	git config diff.ignoreSubmodules dirty &&
+	test_config diff.ignoreSubmodules dirty &&
 	git diff HEAD >actual &&
 	test_must_be_empty actual &&
 	git config --add -f .gitmodules submodule.subname.ignore none &&
@@ -152,7 +152,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)
 	test_cmp expect.body actual.body &&
 	git config --remove-section submodule.subname &&
 	git config --remove-section -f .gitmodules submodule.subname &&
-	git config --unset diff.ignoreSubmodules &&
 	rm .gitmodules
 '
 
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 0c1502d4b0..782424c2d0 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -58,13 +58,12 @@ test_expect_success 'added submodule' '
 '
 
 test_expect_success 'added submodule, set diff.submodule' '
-	git config diff.submodule log &&
+	test_config diff.submodule log &&
 	git add sm1 &&
 	git diff --cached >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 0000000...$head1 (new submodule)
 	EOF
-	git config --unset diff.submodule &&
 	test_cmp expected actual
 '
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index cabdf7d57a..6ad232cb40 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -30,12 +30,11 @@ test_expect_success 'set up basic repos' '
 	>bar &&
 	git add foo &&
 	test_tick &&
-	git config i18n.commitEncoding $test_encoding &&
+	test_config i18n.commitEncoding $test_encoding &&
 	commit_msg $test_encoding | git commit -F - &&
 	git add bar &&
 	test_tick &&
-	git commit -m "add bar" &&
-	git config --unset i18n.commitEncoding
+	git commit -m "add bar"
 '
 
 test_expect_success 'alias builtin format' '
@@ -60,10 +59,9 @@ test_expect_success 'alias user-defined format' '
 '
 
 test_expect_success 'alias user-defined tformat with %s (ISO8859-1 encoding)' '
-	git config i18n.logOutputEncoding $test_encoding &&
+	test_config i18n.logOutputEncoding $test_encoding &&
 	git log --oneline >expected-s &&
 	git log --pretty="tformat:%h %s" >actual-s &&
-	git config --unset i18n.logOutputEncoding &&
 	test_cmp expected-s actual-s
 '
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c7b392794b..bed8e6633a 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -836,8 +836,7 @@ test_expect_success 'rename a remote with name prefix of other remote' '
 
 test_expect_success 'rename succeeds with existing remote.<target>.prune' '
 	git clone one four.four &&
-	test_when_finished git config --global --unset remote.upstream.prune &&
-	git config --global remote.upstream.prune true &&
+	test_config_global remote.upstream.prune true &&
 	git -C four.four remote rename origin upstream
 '
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index ed11569d8d..ff18263171 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -418,7 +418,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
 		git fetch --recurse-submodules
 	) &&
 	add_upstream_commit &&
-	git config --global fetch.recurseSubmodules false &&
+	test_config_global fetch.recurseSubmodules false &&
 	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
@@ -429,11 +429,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
 	(
 		cd downstream &&
 		git config fetch.recurseSubmodules on-demand &&
-		git fetch >../actual.out 2>../actual.err
-	) &&
-	git config --global --unset fetch.recurseSubmodules &&
-	(
-		cd downstream &&
+		git fetch >../actual.out 2>../actual.err &&
 		git config --unset fetch.recurseSubmodules
 	) &&
 	test_must_be_empty actual.out &&
@@ -446,7 +442,7 @@ test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
 		git fetch --recurse-submodules
 	) &&
 	add_upstream_commit &&
-	git config fetch.recurseSubmodules false &&
+	test_config fetch.recurseSubmodules false &&
 	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
@@ -457,11 +453,7 @@ test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
 	(
 		cd downstream &&
 		git config submodule.submodule.fetchRecurseSubmodules on-demand &&
-		git fetch >../actual.out 2>../actual.err
-	) &&
-	git config --unset fetch.recurseSubmodules &&
-	(
-		cd downstream &&
+		git fetch >../actual.out 2>../actual.err &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
 	test_must_be_empty actual.out &&
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 35a2f62392..9ba06ec5ae 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -37,7 +37,7 @@ truncate_count=20
 test_expect_success 'setup' '
 	: >foo &&
 	git add foo &&
-	git config i18n.commitEncoding $test_encoding &&
+	test_config i18n.commitEncoding $test_encoding &&
 	echo "$added_iso88591" | git commit -F - &&
 	head1=$(git rev-parse --verify HEAD) &&
 	head1_short=$(git rev-parse --verify --short $head1) &&
@@ -48,8 +48,7 @@ test_expect_success 'setup' '
 	head2=$(git rev-parse --verify HEAD) &&
 	head2_short=$(git rev-parse --verify --short $head2) &&
 	tree2=$(git rev-parse --verify HEAD:) &&
-	tree2_short=$(git rev-parse --verify --short $tree2) &&
-	git config --unset i18n.commitEncoding
+	tree2_short=$(git rev-parse --verify --short $tree2)
 '
 
 # usage: test_format name format_string [failure] <expected_output
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 9c3cc4cf40..4963f3290d 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -116,7 +116,7 @@ test_expect_success 'no ignore=all setting has any effect' "
 	git config -f .gitmodules submodule.sm1.path sm1 &&
 	git config -f .gitmodules submodule.sm1.ignore all &&
 	git config submodule.sm1.ignore all &&
-	git config diff.ignoreSubmodules all &&
+	test_config diff.ignoreSubmodules all &&
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
 	* sm1 $head1...$head2 (1):
@@ -124,7 +124,6 @@ test_expect_success 'no ignore=all setting has any effect' "
 
 	EOF
 	test_cmp expected actual &&
-	git config --unset diff.ignoreSubmodules &&
 	git config --remove-section submodule.sm1 &&
 	git config -f .gitmodules --remove-section submodule.sm1
 "
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 8cc64729ad..2ff00f6c46 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -803,7 +803,7 @@ test_expect_success 'diff.orderFile configuration is honored' '
 test_expect_success 'mergetool -Oorder-file is honored' '
 	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count order-file-side2 &&
-	test_config diff.orderFile order-file &&
+	git config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
 	echo b >order-file &&
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index b93ae014ee..7f12619feb 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -155,8 +155,7 @@ test_expect_success 'prefetch multiple remotes' '
 	git log --oneline --decorate --all >log &&
 	! grep "prefetch" log &&
 
-	test_when_finished git config --unset remote.remote1.skipFetchAll &&
-	git config remote.remote1.skipFetchAll true &&
+	test_config remote.remote1.skipFetchAll true &&
 	GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
 	test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
 	test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
@@ -407,8 +406,7 @@ test_expect_success 'maintenance.strategy inheritance' '
 		git config --unset maintenance.$task.schedule || return 1
 	done &&
 
-	test_when_finished git config --unset maintenance.strategy &&
-	git config maintenance.strategy incremental &&
+	test_config maintenance.strategy incremental &&
 
 	GIT_TRACE2_EVENT="$(pwd)/incremental-hourly.txt" \
 		git maintenance run --schedule=hourly --quiet &&
@@ -465,8 +463,7 @@ test_expect_success 'maintenance.strategy inheritance' '
 '
 
 test_expect_success 'register and unregister' '
-	test_when_finished git config --global --unset-all maintenance.repo &&
-	git config --global --add maintenance.repo /existing1 &&
+	test_config_global maintenance.repo /existing1 &&
 	git config --global --add maintenance.repo /existing2 &&
 	git config --global --get-all maintenance.repo >before &&
 
-- 
2.31.1.443.g67a420f573

RE: [PATCH 2/2] t: use test_config whenever possible

[Felipe Contreras]

Firmin Martin wrote:
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index ed11569d8d..ff18263171 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -418,7 +418,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
>  		git fetch --recurse-submodules
>  	) &&
>  	add_upstream_commit &&
> -	git config --global fetch.recurseSubmodules false &&
> +	test_config_global fetch.recurseSubmodules false &&
>  	head1=$(git rev-parse --short HEAD) &&
>  	git add submodule &&
>  	git commit -m "new submodule" &&
> @@ -429,11 +429,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
>  	(
>  		cd downstream &&
>  		git config fetch.recurseSubmodules on-demand &&

Uhm:

  test_config fetch.recurseSubmodules on-demand &&

> -		git fetch >../actual.out 2>../actual.err
> -	) &&
> -	git config --global --unset fetch.recurseSubmodules &&
> -	(
> -		cd downstream &&
> +		git fetch >../actual.out 2>../actual.err &&
>  		git config --unset fetch.recurseSubmodules


Then the above line can be removed too.

>  	) &&
>  	test_must_be_empty actual.out &&
> @@ -446,7 +442,7 @@ test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
>  		git fetch --recurse-submodules
>  	) &&
>  	add_upstream_commit &&
> -	git config fetch.recurseSubmodules false &&
> +	test_config fetch.recurseSubmodules false &&
>  	head1=$(git rev-parse --short HEAD) &&
>  	git add submodule &&
>  	git commit -m "new submodule" &&
> @@ -457,11 +453,7 @@ test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
>  	(
>  		cd downstream &&
>  		git config submodule.submodule.fetchRecurseSubmodules on-demand &&

Ditto.

Very nice cleanup.

-- 
Felipe Contreras

RE: [PATCH 2/2] t: use test_config whenever possible

[Felipe Contreras]

Felipe Contreras wrote:
> Firmin Martin wrote:
> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> > index ed11569d8d..ff18263171 100755
> > --- a/t/t5526-fetch-submodules.sh
> > +++ b/t/t5526-fetch-submodules.sh
> > @@ -418,7 +418,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
> >  		git fetch --recurse-submodules
> >  	) &&
> >  	add_upstream_commit &&
> > -	git config --global fetch.recurseSubmodules false &&
> > +	test_config_global fetch.recurseSubmodules false &&
> >  	head1=$(git rev-parse --short HEAD) &&
> >  	git add submodule &&
> >  	git commit -m "new submodule" &&
> > @@ -429,11 +429,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
> >  	(
> >  		cd downstream &&
> >  		git config fetch.recurseSubmodules on-demand &&
> 
> Uhm:
> 
>   test_config fetch.recurseSubmodules on-demand &&

Ahh, I just read the comment about not using it in a subshell.

Then outside:

  test_config -C downstream fetch.recurseSubmodules on-demand &&

-- 
Felipe Contreras

RE: [PATCH 2/2] t: use test_config whenever possible

[Firmin Martin]

>
> Ahh, I just read the comment about not using it in a subshell.
>
> Then outside:
>
>   test_config -C downstream fetch.recurseSubmodules on-demand &&
True. This makes me realize that I should also briefly document the -C option.

Thanks,

Firmin

Re: [PATCH 1/2] t/README: document test_config

[Eric Sunshine]

On Fri, May 14, 2021 at 2:55 AM Firmin Martin <firminmartin24@gmail.com> wrote:
> test_config is used over one thousand times in the test suite, yet not
> documented. Give it a place in the "Test harness library" section.
>
> Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
> ---
> diff --git a/t/README b/t/README
> @@ -1046,6 +1046,21 @@ library for your script to use.
> + - test_config <config-option> [<value>]
> +
> +   Set the configuration option <config-option> to <value>, and unset it at the
> +   end of the current test. For a similar purpose, test_config_global for
> +   global configuration is also available. Note, however, that test_config_*
> +   should not be used under a subshell.

"should" is perhaps too weak of a word. test_config() will not
function correctly at all (just as test_when_finished() will not
function correctly) within a subshell. So, perhaps say "must not be
used in a subshell."

Re: [PATCH 1/2] t/README: document test_config

[Firmin Martin]

Hi Eric,

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, May 14, 2021 at 2:55 AM Firmin Martin <firminmartin24@gmail.com> wrote:
>> test_config is used over one thousand times in the test suite, yet not
>> documented. Give it a place in the "Test harness library" section.
>>
>> Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
>> ---
>> diff --git a/t/README b/t/README
>> @@ -1046,6 +1046,21 @@ library for your script to use.
>> + - test_config <config-option> [<value>]
>> +
>> +   Set the configuration option <config-option> to <value>, and unset it at the
>> +   end of the current test. For a similar purpose, test_config_global for
>> +   global configuration is also available. Note, however, that test_config_*
>> +   should not be used under a subshell.
>
> "should" is perhaps too weak of a word. test_config() will not
> function correctly at all (just as test_when_finished() will not
> function correctly) within a subshell. So, perhaps say "must not be
> used in a subshell."
Indeed, will correct this.

Thanks,

Firmin

[PATCH v2 0/2] document test_config & use it whenever possible

[Firmin Martin]

* Fix wording.

---
Firmin Martin (2):
  t/README: document test_config
  t: use test_config whenever possible

 t/README                         | 15 +++++++++++++++
 t/t3200-branch.sh                |  9 +++------
 t/t3900-i18n-commit.sh           |  3 +--
 t/t4027-diff-submodule.sh        |  3 +--
 t/t4041-diff-submodule-option.sh |  3 +--
 t/t4205-log-pretty-formats.sh    |  8 +++-----
 t/t5505-remote.sh                |  3 +--
 t/t5526-fetch-submodules.sh      | 16 ++++------------
 t/t6006-rev-list-format.sh       |  5 ++---
 t/t7401-submodule-summary.sh     |  3 +--
 t/t7610-mergetool.sh             |  2 +-
 t/t7900-maintenance.sh           |  9 +++------
 12 files changed, 36 insertions(+), 43 deletions(-)

Range-diff against v1:
1:  ee35266247 ! 1:  07ca310549 t/README: document test_config
    @@ Commit message
         test_config is used over one thousand times in the test suite, yet not
         documented. Give it a place in the "Test harness library" section.
     
    +    Thanks-to: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
     
      ## t/README ##
    @@ t/README: library for your script to use.
     +   Set the configuration option <config-option> to <value>, and unset it at the
     +   end of the current test. For a similar purpose, test_config_global for
     +   global configuration is also available. Note, however, that test_config_*
    -+   should not be used under a subshell.
    ++   must not be used in a subshell.
     +  
     +   Example:
     +
2:  8a878755ef = 2:  279b5b3701 t: use test_config whenever possible
-- 
2.31.1.443.g67a420f573

[PATCH v2 1/2] t/README: document test_config

[Firmin Martin]

test_config is used over one thousand times in the test suite, yet not
documented. Give it a place in the "Test harness library" section.

Thanks-to: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 t/README | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/README b/t/README
index 8eb9e46b1d..aaf1c9cadc 100644
--- a/t/README
+++ b/t/README
@@ -1046,6 +1046,21 @@ library for your script to use.
    Abort the test script if either the value of the variable or the
    default are not valid bool values.
 
+ - test_config <config-option> [<value>]
+
+   Set the configuration option <config-option> to <value>, and unset it at the
+   end of the current test. For a similar purpose, test_config_global for
+   global configuration is also available. Note, however, that test_config_*
+   must not be used in a subshell.
+  
+   Example:
+
+	test_config format.coverLetter auto
+
+   Is a concise way to write:
+	test_when_finished "git config --unset format.coverLetter" &&
+	git config format.coverLetter auto
+
 
 Prerequisites
 -------------
-- 
2.31.1.443.g67a420f573

Re: [PATCH v2 1/2] t/README: document test_config

[Bagas Sanjaya]

On 15/05/21 22.27, Firmin Martin wrote:
>   
> + - test_config <config-option> [<value>]
> +
> +   Set the configuration option <config-option> to <value>, and unset it at the
> +   end of the current test. For a similar purpose, test_config_global for
> +   global configuration is also available. Note, however, that test_config_*
> +   must not be used in a subshell.
> +

 From the syntax above, when I omit <value>, default value for <config-option>
will be set, right? You forgot to mention it.

> +   Example:
> +
> +	test_config format.coverLetter auto
> +
> +   Is a concise way to write:
> +	test_when_finished "git config --unset format.coverLetter" &&
> +	git config format.coverLetter auto
> +
>   

The description said "set the config, then unset it". But the expanded version
said "unsetting the config is deferred to the end of test and set the config".

The documentation for test_when_finished said:

>    Prepend <script> to a list of commands to run to clean up
>    at the end of the current test.  If some clean-up command
>    fails, the test will not pass.

Is my interpretation above correct?

Oh yeah, maybe it's better to say:
"For example: <blah> have the same effect as <blah>."

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v2 1/2] t/README: document test_config

[Firmin Martin]

Hi Bagas,

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On 15/05/21 22.27, Firmin Martin wrote:
>>   
>> + - test_config <config-option> [<value>]
>> +
>> +   Set the configuration option <config-option> to <value>, and unset it at the
>> +   end of the current test. For a similar purpose, test_config_global for
>> +   global configuration is also available. Note, however, that test_config_*
>> +   must not be used in a subshell.
>> +

>  From the syntax above, when I omit <value>, default value for <config-option>
> will be set, right? 
Good remark. I thought the same as you, but no, it will "git config
<config-option>" (query the value of <config-option>) and then "git
config --unset <config-option>" which is dangerous. Doing so is more
likely an error and should be forbidden, as in this case, it is
equivalent to test_unconfig <config-option>. Will reflect this in v3.
> You forgot to mention it.
I'll thus drop square brackets.

>> +   Example:
>> +
>> +	test_config format.coverLetter auto
>> +
>> +   Is a concise way to write:
>> +	test_when_finished "git config --unset format.coverLetter" &&
>> +	git config format.coverLetter auto
>> +
>>   
>
> The description said "set the config, then unset it". But the expanded version
> said "unsetting the config is deferred to the end of test and set the config".
>
> The documentation for test_when_finished said:
>
>>    Prepend <script> to a list of commands to run to clean up
>>    at the end of the current test.  If some clean-up command
>>    fails, the test will not pass.
>
> Is my interpretation above correct?
Yes.

> Oh yeah, maybe it's better to say:
> "For example: <blah> have the same effect as <blah>."
I agree, because the two lines have not exactly the same behaviour as
test_config. Will fix wording.

Thanks,

Firmin

[PATCH v2 2/2] t: use test_config whenever possible

[Firmin Martin]

Replace patterns of the form
1.
        git config <config-option> <value> &&
        ...
        git config --unset <config-option> &&
2.
        test_when_finished "git config --unset <config-option>" &&
        git config <config-option> <value> &&
        ...

to the concise

        test_config <config-option> <value>

In t5526, two tests have been further simplified as the output file is
written before "git config --global --unset".

Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 t/t3200-branch.sh                |  9 +++------
 t/t3900-i18n-commit.sh           |  3 +--
 t/t4027-diff-submodule.sh        |  3 +--
 t/t4041-diff-submodule-option.sh |  3 +--
 t/t4205-log-pretty-formats.sh    |  8 +++-----
 t/t5505-remote.sh                |  3 +--
 t/t5526-fetch-submodules.sh      | 16 ++++------------
 t/t6006-rev-list-format.sh       |  5 ++---
 t/t7401-submodule-summary.sh     |  3 +--
 t/t7610-mergetool.sh             |  2 +-
 t/t7900-maintenance.sh           |  9 +++------
 11 files changed, 21 insertions(+), 43 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cc4b10236e..0b0119bbe2 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -365,11 +365,9 @@ EOF
 '
 
 test_expect_success 'git branch with column.*' '
-	git config column.ui column &&
-	git config column.branch "dense" &&
+	test_config column.ui column &&
+	test_config column.branch "dense" &&
 	COLUMNS=80 git branch >actual &&
-	git config --unset column.branch &&
-	git config --unset column.ui &&
 	cat >expect <<\EOF &&
   a/b/c   bam   foo   l   * main   n     o/p   r
   abc     bar   j/k   m/m   mb     o/o   q     topic
@@ -382,9 +380,8 @@ test_expect_success 'git branch --column -v should fail' '
 '
 
 test_expect_success 'git branch -v with column.ui ignored' '
-	git config column.ui column &&
+	test_config column.ui column &&
 	COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
-	git config --unset column.ui &&
 	cat >expect <<\EOF &&
   a/b/c
   abc
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index bfab245eb3..c16c0f7fba 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -179,7 +179,7 @@ test_commit_autosquash_flags () {
 	H=$1
 	flag=$2
 	test_expect_success "commit --$flag with $H encoding" '
-		git config i18n.commitencoding $H &&
+		test_config i18n.commitencoding $H &&
 		git checkout -b $H-$flag C0 &&
 		echo $H >>F &&
 		git commit -a -F "$TEST_DIRECTORY"/t3900/$H.txt &&
@@ -193,7 +193,6 @@ test_commit_autosquash_flags () {
 		E=$(git cat-file commit '$H-$flag' |
 			sed -ne "s/^encoding //p") &&
 		test "z$E" = "z$H" &&
-		git config --unset-all i18n.commitencoding &&
 		git rebase --autosquash -i HEAD^^^ &&
 		git log --oneline >actual &&
 		test_line_count = 3 actual
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 94ef77e1df..a6eb2416ed 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -123,7 +123,7 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'
 '
 
 test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '
-	git config diff.ignoreSubmodules dirty &&
+	test_config diff.ignoreSubmodules dirty &&
 	git diff HEAD >actual &&
 	test_must_be_empty actual &&
 	git config --add -f .gitmodules submodule.subname.ignore none &&
@@ -152,7 +152,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)
 	test_cmp expect.body actual.body &&
 	git config --remove-section submodule.subname &&
 	git config --remove-section -f .gitmodules submodule.subname &&
-	git config --unset diff.ignoreSubmodules &&
 	rm .gitmodules
 '
 
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 0c1502d4b0..782424c2d0 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -58,13 +58,12 @@ test_expect_success 'added submodule' '
 '
 
 test_expect_success 'added submodule, set diff.submodule' '
-	git config diff.submodule log &&
+	test_config diff.submodule log &&
 	git add sm1 &&
 	git diff --cached >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 0000000...$head1 (new submodule)
 	EOF
-	git config --unset diff.submodule &&
 	test_cmp expected actual
 '
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 8272d94ce6..fbe8ae5378 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -30,12 +30,11 @@ test_expect_success 'set up basic repos' '
 	>bar &&
 	git add foo &&
 	test_tick &&
-	git config i18n.commitEncoding $test_encoding &&
+	test_config i18n.commitEncoding $test_encoding &&
 	commit_msg $test_encoding | git commit -F - &&
 	git add bar &&
 	test_tick &&
-	git commit -m "add bar" &&
-	git config --unset i18n.commitEncoding
+	git commit -m "add bar"
 '
 
 test_expect_success 'alias builtin format' '
@@ -60,10 +59,9 @@ test_expect_success 'alias user-defined format' '
 '
 
 test_expect_success 'alias user-defined tformat with %s (ISO8859-1 encoding)' '
-	git config i18n.logOutputEncoding $test_encoding &&
+	test_config i18n.logOutputEncoding $test_encoding &&
 	git log --oneline >expected-s &&
 	git log --pretty="tformat:%h %s" >actual-s &&
-	git config --unset i18n.logOutputEncoding &&
 	test_cmp expected-s actual-s
 '
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c7b392794b..bed8e6633a 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -836,8 +836,7 @@ test_expect_success 'rename a remote with name prefix of other remote' '
 
 test_expect_success 'rename succeeds with existing remote.<target>.prune' '
 	git clone one four.four &&
-	test_when_finished git config --global --unset remote.upstream.prune &&
-	git config --global remote.upstream.prune true &&
+	test_config_global remote.upstream.prune true &&
 	git -C four.four remote rename origin upstream
 '
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index ed11569d8d..ff18263171 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -418,7 +418,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
 		git fetch --recurse-submodules
 	) &&
 	add_upstream_commit &&
-	git config --global fetch.recurseSubmodules false &&
+	test_config_global fetch.recurseSubmodules false &&
 	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
@@ -429,11 +429,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
 	(
 		cd downstream &&
 		git config fetch.recurseSubmodules on-demand &&
-		git fetch >../actual.out 2>../actual.err
-	) &&
-	git config --global --unset fetch.recurseSubmodules &&
-	(
-		cd downstream &&
+		git fetch >../actual.out 2>../actual.err &&
 		git config --unset fetch.recurseSubmodules
 	) &&
 	test_must_be_empty actual.out &&
@@ -446,7 +442,7 @@ test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
 		git fetch --recurse-submodules
 	) &&
 	add_upstream_commit &&
-	git config fetch.recurseSubmodules false &&
+	test_config fetch.recurseSubmodules false &&
 	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
@@ -457,11 +453,7 @@ test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
 	(
 		cd downstream &&
 		git config submodule.submodule.fetchRecurseSubmodules on-demand &&
-		git fetch >../actual.out 2>../actual.err
-	) &&
-	git config --unset fetch.recurseSubmodules &&
-	(
-		cd downstream &&
+		git fetch >../actual.out 2>../actual.err &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
 	test_must_be_empty actual.out &&
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 35a2f62392..9ba06ec5ae 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -37,7 +37,7 @@ truncate_count=20
 test_expect_success 'setup' '
 	: >foo &&
 	git add foo &&
-	git config i18n.commitEncoding $test_encoding &&
+	test_config i18n.commitEncoding $test_encoding &&
 	echo "$added_iso88591" | git commit -F - &&
 	head1=$(git rev-parse --verify HEAD) &&
 	head1_short=$(git rev-parse --verify --short $head1) &&
@@ -48,8 +48,7 @@ test_expect_success 'setup' '
 	head2=$(git rev-parse --verify HEAD) &&
 	head2_short=$(git rev-parse --verify --short $head2) &&
 	tree2=$(git rev-parse --verify HEAD:) &&
-	tree2_short=$(git rev-parse --verify --short $tree2) &&
-	git config --unset i18n.commitEncoding
+	tree2_short=$(git rev-parse --verify --short $tree2)
 '
 
 # usage: test_format name format_string [failure] <expected_output
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 9c3cc4cf40..4963f3290d 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -116,7 +116,7 @@ test_expect_success 'no ignore=all setting has any effect' "
 	git config -f .gitmodules submodule.sm1.path sm1 &&
 	git config -f .gitmodules submodule.sm1.ignore all &&
 	git config submodule.sm1.ignore all &&
-	git config diff.ignoreSubmodules all &&
+	test_config diff.ignoreSubmodules all &&
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
 	* sm1 $head1...$head2 (1):
@@ -124,7 +124,6 @@ test_expect_success 'no ignore=all setting has any effect' "
 
 	EOF
 	test_cmp expected actual &&
-	git config --unset diff.ignoreSubmodules &&
 	git config --remove-section submodule.sm1 &&
 	git config -f .gitmodules --remove-section submodule.sm1
 "
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 8cc64729ad..2ff00f6c46 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -803,7 +803,7 @@ test_expect_success 'diff.orderFile configuration is honored' '
 test_expect_success 'mergetool -Oorder-file is honored' '
 	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count order-file-side2 &&
-	test_config diff.orderFile order-file &&
+	git config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
 	echo b >order-file &&
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index b93ae014ee..7f12619feb 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -155,8 +155,7 @@ test_expect_success 'prefetch multiple remotes' '
 	git log --oneline --decorate --all >log &&
 	! grep "prefetch" log &&
 
-	test_when_finished git config --unset remote.remote1.skipFetchAll &&
-	git config remote.remote1.skipFetchAll true &&
+	test_config remote.remote1.skipFetchAll true &&
 	GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
 	test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
 	test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
@@ -407,8 +406,7 @@ test_expect_success 'maintenance.strategy inheritance' '
 		git config --unset maintenance.$task.schedule || return 1
 	done &&
 
-	test_when_finished git config --unset maintenance.strategy &&
-	git config maintenance.strategy incremental &&
+	test_config maintenance.strategy incremental &&
 
 	GIT_TRACE2_EVENT="$(pwd)/incremental-hourly.txt" \
 		git maintenance run --schedule=hourly --quiet &&
@@ -465,8 +463,7 @@ test_expect_success 'maintenance.strategy inheritance' '
 '
 
 test_expect_success 'register and unregister' '
-	test_when_finished git config --global --unset-all maintenance.repo &&
-	git config --global --add maintenance.repo /existing1 &&
+	test_config_global maintenance.repo /existing1 &&
 	git config --global --add maintenance.repo /existing2 &&
 	git config --global --get-all maintenance.repo >before &&
 
-- 
2.31.1.443.g67a420f573

Re: [PATCH v2 2/2] t: use test_config whenever possible

[Junio C Hamano]

Firmin Martin <firminmartin24@gmail.com> writes:

"whenever possible" in the subject is not wrong per-se but because
we do not want to accept a patch that uses it where it is impossible
to be used anyway, it does not mean all that much.

Use of test_config to replace #1 below is a correctness improvement,
and use of test_config to replace #2 below is a readability thing.

    t: use more test_config for correctness and readability

perhaps?

> Replace patterns of the form
> 1.
>         git config <config-option> <value> &&
>         ...
>         git config --unset <config-option> &&

Adding "because this form fails to unset the config if an earlier
step fails." would be helpful to the readers to clarify that this is
a correctness "fix".

> 2.
>         test_when_finished "git config --unset <config-option>" &&
>         git config <config-option> <value> &&
>         ...
>
> to the concise
>
>         test_config <config-option> <value>

Nice.

> In t5526, two tests have been further simplified as the output file is
> written before "git config --global --unset".

> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index ed11569d8d..ff18263171 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> ...
@@ -429,11 +429,7 @@ test_expect_success "'fetch.recurseSu
>  	(
>  		cd downstream &&
>  		git config fetch.recurseSubmodules on-demand &&
> -		git fetch >../actual.out 2>../actual.err
> -	) &&
> -	git config --global --unset fetch.recurseSubmodules &&
> -	(
> -		cd downstream &&
> +		git fetch >../actual.out 2>../actual.err &&
>  		git config --unset fetch.recurseSubmodules
>  	) &&
>  	test_must_be_empty actual.out &&

The original seems to be making sure that the config set in
"downstream" repository is used successfully before it is unset from
the global configuration and then the same fetch in the downstream
will work even when the configuration is removed from the global
configuration.  The squashing of the two tests into one would change
what is being tested, wouldn't it?

Surely, the actual.out and actual.err in the first invocation *was*
overwritten without being looked at, but the exit status of that
fetch (i.e. fetch before global config gets unset) was still checked
in the original.  So I'd call this a "simplification"

>  	(
>  		cd downstream &&
>  		git config fetch.recurseSubmodules on-demand &&
> -		git fetch >../actual.out 2>../actual.err
> +		git fetch
> 	) &&
> 	git config --global --unset fetch.recurseSubmodules &&

I.e. these redirected output files were not used and the next fetch
will overwrite them.

The patch I am responding to claims that 

 - "make sure it succeeds with global set, and then without global
   set, make sure it still works" is not worth testing.

 - it is OK to replace it with a single "it succeeds without
   unsetting the global config".

It is not clear if either is true.  Same for the other hunk that
squashes two tests into one for this path.

Re: [PATCH v2 2/2] t: use test_config whenever possible

[Firmin Martin]

Hi Junio,

Junio C Hamano <gitster@pobox.com> writes:

> Use of test_config to replace #1 below is a correctness improvement,
> and use of test_config to replace #2 below is a readability thing.
>
>     t: use more test_config for correctness and readability
>
> perhaps?
OK.

> Adding "because this form fails to unset the config if an earlier
> step fails." would be helpful to the readers to clarify that this is
> a correctness "fix".
Will do.

To be honest, I'm completely lost in the remaining of your reply (in
fact, I suspect that you misread the diff, cf. the end of the email). 
I copy here the full test with line number to make sure that we are in
the same page.

>  415 test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config" '
>  416     (
>  417         cd downstream &&
>  418         git fetch --recurse-submodules
>  419     ) &&
>  420     add_upstream_commit &&
>  421     git config --global fetch.recurseSubmodules false &&
>  422     head1=$(git rev-parse --short HEAD) &&
>  423     git add submodule &&
>  424     git commit -m "new submodule" &&
>  425     head2=$(git rev-parse --short HEAD) &&
>  426     echo "From $pwd/." > expect.err.2 &&
>  427     echo "   $head1..$head2  super      -> origin/super" >>expect.err.2 &&
>  428     head -3 expect.err >> expect.err.2 &&
>  429     (
>  430         cd downstream &&
>  431         git config fetch.recurseSubmodules on-demand &&
>  432         git fetch >../actual.out 2>../actual.err
>  433     ) &&
>  434     git config --global --unset fetch.recurseSubmodules &&
>  435     (
>  436         cd downstream &&
>  437         git config --unset fetch.recurseSubmodules
>  438     ) &&
>  439     test_must_be_empty actual.out &&
>  440     test_cmp expect.err.2 actual.err


> The original seems to be making sure that the config set in
> "downstream" repository is used successfully before it is unset from
> the global configuration
OK. 
> and then the same fetch in the downstream will work even when the
What do you mean by "the same fetch"? the one of line 432 or any
instance of "git -C downstream fetch" in later tests? 
> configuration is removed from the global configuration.

> The squashing of the two tests into one
Same question as above, I don't understand "the second test" you are
referring to (specifically, which lines?).

> would change what is being tested, wouldn't it?
I agree that the semantic isn't the same (as test_config_global will
unset global config after unsetting the "downstream" config), but since
there is *not* another git-fetch after the global unset and before the
"downstream" unset I thought that it is fine.

> Surely, the actual.out and actual.err in the first invocation *was*
> overwritten without being looked at, but the exit status of that
Aren't they considered in the end of the test ? (lines 439-440).
> fetch (i.e. fetch before global config gets unset) was still checked
> in the original.  So I'd call this a "simplification"
>
>>  	(
>>  		cd downstream &&
>>  		git config fetch.recurseSubmodules on-demand &&
>> -		git fetch >../actual.out 2>../actual.err
>> +		git fetch
>> 	) &&
>> 	git config --global --unset fetch.recurseSubmodules &&

> I.e. these redirected output files were not used and the next fetch
> will overwrite them.
Same question as above.

> The patch I am responding to claims that 
>
>  - "make sure it succeeds with global set, and then without global
>    set, make sure it still works" is not worth testing.
In this test, there is no "make sure it still works without global set"
(i.e. I didn't see any git-fetch invocation after the global
configuration unset). Are you referring later tests?
>
>  - it is OK to replace it with a single "it succeeds without
>    unsetting the global config".
At this point, I have the feeling that you have misread the diff
(do correct me If I'm wrong) and saw ...
>  (
>      cd downstream &&
>      git config fetch.recurseSubmodules on-demand &&
>      git fetch >../actual.out 2>../actual.err
>  ) &&
>  git config --global --unset fetch.recurseSubmodules &&
>  (
>      cd downstream &&
>      git fetch >../actual.out 2>../actual.err
>      git config --unset fetch.recurseSubmodules
>  ) &&
(note the second extra git-fetch)

... instead of

>  429     (
>  430         cd downstream &&
>  431         git config fetch.recurseSubmodules on-demand &&
>  432         git fetch >../actual.out 2>../actual.err
>  433     ) &&
>  434     git config --global --unset fetch.recurseSubmodules &&
>  435     (
>  436         cd downstream &&
>  437         git config --unset fetch.recurseSubmodules
>  438     ) &&

In this case, what you said (e.g. simplification etc.) makes perfect sense.

Thanks,

Firmin

Re: [PATCH v2 2/2] t: use test_config whenever possible

[Junio C Hamano]

Firmin Martin <firminmartin24@gmail.com> writes:

> fact, I suspect that you misread the diff, cf. the end of the email). 
> I copy here the full test with line number to make sure that we are in
> the same page.

The following is the original test before your patch, right?

>>  415 test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config" '
>>  416     (
>>  417         cd downstream &&
>>  418         git fetch --recurse-submodules
>>  419     ) &&
>>  420     add_upstream_commit &&
>>  421     git config --global fetch.recurseSubmodules false &&
>>  422     head1=$(git rev-parse --short HEAD) &&
>>  423     git add submodule &&
>>  424     git commit -m "new submodule" &&
>>  425     head2=$(git rev-parse --short HEAD) &&
>>  426     echo "From $pwd/." > expect.err.2 &&
>>  427     echo "   $head1..$head2  super      -> origin/super" >>expect.err.2 &&
>>  428     head -3 expect.err >> expect.err.2 &&
>>  429     (
>>  430         cd downstream &&
>>  431         git config fetch.recurseSubmodules on-demand &&
>>  432         git fetch >../actual.out 2>../actual.err

Here we run one "fetch" inside downstream, with a config specific to
the downstream repository; the expectation is that this on-demand
setting is honored, overriding the "global" setting that was set on
line 421.

>>  433     ) &&
>>  434     git config --global --unset fetch.recurseSubmodules &&

And then we discard the "global" setting.

>>  435     (
>>  436         cd downstream &&
>>  437         git config --unset fetch.recurseSubmodules

Oh, I thought I saw there was another fetch before this unset
between 436 and 437, that writes to the same actual.out and
actual.err.

> At this point, I have the feeling that you have misread the diff
> (do correct me If I'm wrong) and saw ...
>>  (
>>      cd downstream &&
>>      git config fetch.recurseSubmodules on-demand &&
>>      git fetch >../actual.out 2>../actual.err
>>  ) &&
>>  git config --global --unset fetch.recurseSubmodules &&
>>  (
>>      cd downstream &&
>>      git fetch >../actual.out 2>../actual.err
>>      git config --unset fetch.recurseSubmodules
>>  ) &&

Exactly.  Thanks and sorry for the noise.