| Message ID | patch-v2-1.2-91402624777-20211201T200801Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD | expand |
On Wed, Dec 01, 2021 at 09:11:41PM +0100, Ævar Arnfjörð Bjarmason wrote: > Amend the tests checking whether stderr is empty added in > 4868b2ea17b (Subject: setup: officially support --work-tree without > --git-dir, 2011-01-19) work portably on all POSIX shells, instead > suppressing the trace output with "test_untraceable" on shells that > aren't bash. > > The tests that used the "try_repo" helper wanted to check whether git > commands invoked therein would emit anything on stderr. To do this > they invoked the function and redirected the stderr to a "message" > file. > > In 58275069288 (t1510-repo-setup: mark as untraceable with '-x', > 2018-02-24) these were made to use "test_untraceable" introduced in > 5fc98e79fc0 (t: add means to disable '-x' tracing for individual test > scripts, 2018-02-24). > > It is better to have the "try_repo" function itself start with a > "test_when_finished 'rm stderr'", and then redirect the stderr output > from git commands it invokes via its helpers to a "stderr" file. > > This means that if we have a failure it'll be closer to the source of > the problem, and most importantly isn't incompatible with "-x" on > shells that aren't "bash". > > We also need to split those tests that had two "try_repo" invocations > into different tests, which'll further help to narrow down any > potential failures. This wasn't strictly necessary (an artifact of the > use of "test_when_finished"), but the pattern enforces better test > hygiene. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/t1510-repo-setup.sh | 83 +++++++++++++++++++++---------------------- > 1 file changed, 40 insertions(+), 43 deletions(-) > > diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh > index 591505a39c0..f1748ac4a19 100755 > --- a/t/t1510-repo-setup.sh > +++ b/t/t1510-repo-setup.sh > @@ -40,9 +40,6 @@ A few rules for repo setup: > prefix is NULL. > " > > -# This test heavily relies on the standard error of nested function calls. > -test_untraceable=UnfortunatelyYes > - > TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > @@ -62,7 +59,7 @@ test_repo () { > export GIT_WORK_TREE > fi && > rm -f trace && > - GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null && > + GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr && I suspect that it's lines like this that make Peff argue for BASH_XTRACEFD :) While this is not a compound command, it does contain a command substitution, and the trace generated when executing the command in that command substitution goes to the command's stderr, and then, because of the redirection, to the 'stderr' file. I find it suspicious that this doesn't trigger a failure in a 'test_must_be_empty stderr' later on. > grep '^setup: ' trace >result && > test_cmp expected result > ) > @@ -72,7 +69,7 @@ maybe_config () { > file=$1 var=$2 value=$3 && > if test "$value" != unset > then > - git config --file="$file" "$var" "$value" > + git config --file="$file" "$var" "$value" 2>>stderr > fi > } > > @@ -80,7 +77,7 @@ setup_repo () { > name=$1 worktreecfg=$2 gitfile=$3 barecfg=$4 && > sane_unset GIT_DIR GIT_WORK_TREE && > > - git -c init.defaultBranch=initial init "$name" && > + git -c init.defaultBranch=initial init "$name" 2>>stderr && > maybe_config "$name/.git/config" core.worktree "$worktreecfg" && > maybe_config "$name/.git/config" core.bare "$barecfg" && > mkdir -p "$name/sub/sub" && > @@ -210,10 +207,12 @@ run_wt_tests () { > # (git dir) (work tree) (cwd) (prefix) <-- from subdir > try_repo () { > name=$1 worktreeenv=$2 gitdirenv=$3 && > + test_when_finished "rm stderr" && > setup_repo "$name" "$4" "$5" "$6" && > shift 6 && > try_case "$name" "$worktreeenv" "$gitdirenv" \ > "$1" "$2" "$3" "$4" && > + test_must_be_empty stderr && > shift 4 && > case "$gitdirenv" in > /* | ?:/* | unset) ;; > @@ -221,7 +220,8 @@ try_repo () { > gitdirenv=../$gitdirenv ;; > esac && > try_case "$name/sub" "$worktreeenv" "$gitdirenv" \ > - "$1" "$2" "$3" "$4" > + "$1" "$2" "$3" "$4" && > + test_must_be_empty stderr > } > > # Bit 0 = GIT_WORK_TREE > @@ -234,15 +234,13 @@ try_repo () { > test_expect_success '#0: nonbare repo, no explicit configuration' ' > try_repo 0 unset unset unset "" unset \ > .git "$here/0" "$here/0" "(null)" \ > - .git "$here/0" "$here/0" sub/ 2>message && > - test_must_be_empty message > + .git "$here/0" "$here/0" sub/ > ' > > test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' ' > try_repo 1 "$here" unset unset "" unset \ > "$here/1/.git" "$here" "$here" 1/ \ > - "$here/1/.git" "$here" "$here" 1/sub/ 2>message && > - test_must_be_empty message > + "$here/1/.git" "$here" "$here" 1/sub/ > ' > > test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' ' > @@ -268,19 +266,20 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' ' > mkdir -p 4/sub sub && > try_case 4 unset unset \ > .git "$here/4/sub" "$here/4" "(null)" \ > - "$here/4/.git" "$here/4/sub" "$here/4/sub" "(null)" 2>message && > - test_must_be_empty message > + "$here/4/.git" "$here/4/sub" "$here/4/sub" "(null)" > ' > > -test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' ' > +test_expect_success '#5.1: core.worktree + GIT_WORK_TREE is accepted' ' > # or: you cannot intimidate away the lack of GIT_DIR setting > try_repo 5 "$here" unset "$here/5" "" unset \ > "$here/5/.git" "$here" "$here" 5/ \ > - "$here/5/.git" "$here" "$here" 5/sub/ 2>message && > + "$here/5/.git" "$here" "$here" 5/sub/ > +' > + > +test_expect_success '#5.2: core.worktree + GIT_WORK_TREE is accepted' ' > try_repo 5a .. unset "$here/5a" "" unset \ > "$here/5a/.git" "$here" "$here" 5a/ \ > - "$here/5a/.git" "$here/5a" "$here/5a" sub/ && > - test_must_be_empty message > + "$here/5a/.git" "$here/5a" "$here/5a" sub/ > ' > > test_expect_success '#6: setting GIT_DIR brings core.worktree to life' ' > @@ -376,8 +375,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' ' > mkdir -p 9/wt && > try_repo 9 wt unset unset gitfile unset \ > "$here/9.git" "$here/9/wt" "$here/9" "(null)" \ > - "$here/9.git" "$here/9/sub/wt" "$here/9/sub" "(null)" 2>message && > - test_must_be_empty message > + "$here/9.git" "$here/9/sub/wt" "$here/9/sub" "(null)" > ' > > test_expect_success '#10: GIT_DIR can point to gitfile' ' > @@ -402,16 +400,14 @@ run_wt_tests 11 gitfile > test_expect_success '#12: core.worktree with gitfile is accepted' ' > try_repo 12 unset unset "$here/12" gitfile unset \ > "$here/12.git" "$here/12" "$here/12" "(null)" \ > - "$here/12.git" "$here/12" "$here/12" sub/ 2>message && > - test_must_be_empty message > + "$here/12.git" "$here/12" "$here/12" sub/ > ' > > test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' ' > # or: you cannot intimidate away the lack of GIT_DIR setting > try_repo 13 non-existent-too unset non-existent gitfile unset \ > "$here/13.git" "$here/13/non-existent-too" "$here/13" "(null)" \ > - "$here/13.git" "$here/13/sub/non-existent-too" "$here/13/sub" "(null)" 2>message && > - test_must_be_empty message > + "$here/13.git" "$here/13/sub/non-existent-too" "$here/13/sub" "(null)" > ' > > # case #14. > @@ -549,8 +545,7 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba > mkdir -p 17b/.git/wt/sub && > > try_case 17a/.git "$here/17a" unset \ > - "$here/17a/.git" "$here/17a" "$here/17a" .git/ \ > - 2>message && > + "$here/17a/.git" "$here/17a" "$here/17a" .git/ && > try_case 17a/.git/wt "$here/17a" unset \ > "$here/17a/.git" "$here/17a" "$here/17a" .git/wt/ && > try_case 17a/.git/wt/sub "$here/17a" unset \ > @@ -565,14 +560,16 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba > > try_repo 17c "$here/17c" unset unset "" true \ > .git "$here/17c" "$here/17c" "(null)" \ > - "$here/17c/.git" "$here/17c" "$here/17c" sub/ 2>message && > - test_must_be_empty message > + "$here/17c/.git" "$here/17c" "$here/17c" sub/ > ' > > -test_expect_success '#18: bare .git named by GIT_DIR has no worktree' ' > +test_expect_success '#18.1: bare .git named by GIT_DIR has no worktree' ' > try_repo 18 unset .git unset "" true \ > .git "(null)" "$here/18" "(null)" \ > - ../.git "(null)" "$here/18/sub" "(null)" && > + ../.git "(null)" "$here/18/sub" "(null)" > +' > + > +test_expect_success '#18.2: bare .git named by GIT_DIR has no worktree' ' > try_repo 18b unset "$here/18b/.git" unset "" true \ > "$here/18b/.git" "(null)" "$here/18b" "(null)" \ > "$here/18b/.git" "(null)" "$here/18b/sub" "(null)" > @@ -590,12 +587,11 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)' > setup_repo 20a "$here/20a" "" unset && > mkdir -p 20a/.git/wt/sub && > try_case 20a/.git unset unset \ > - "$here/20a/.git" "$here/20a" "$here/20a" .git/ 2>message && > + "$here/20a/.git" "$here/20a" "$here/20a" .git/ && > try_case 20a/.git/wt unset unset \ > "$here/20a/.git" "$here/20a" "$here/20a" .git/wt/ && > try_case 20a/.git/wt/sub unset unset \ > - "$here/20a/.git" "$here/20a" "$here/20a" .git/wt/sub/ && > - test_must_be_empty message > + "$here/20a/.git" "$here/20a" "$here/20a" .git/wt/sub/ > ' > > test_expect_success '#20b/c: core.worktree and core.bare conflict' ' > @@ -625,10 +621,9 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare > cd 21/.git && > GIT_WORK_TREE="$here/21" && > export GIT_WORK_TREE && > - git status >/dev/null > - ) 2>message && > - test_must_be_empty message > - > + git status 2>message && > + test_must_be_empty message > + ) > ' > run_wt_tests 21 > > @@ -742,14 +737,16 @@ test_expect_success '#24: bare repo has no worktree (gitfile case)' ' > test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile case)' ' > try_repo 25 "$here/25" unset unset gitfile true \ > "$here/25.git" "$here/25" "$here/25" "(null)" \ > - "$here/25.git" "$here/25" "$here/25" "sub/" 2>message && > - test_must_be_empty message > + "$here/25.git" "$here/25" "$here/25" "sub/" > ' > > -test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' ' > +test_expect_success '#26.1: bare repo has no worktree (GIT_DIR -> gitfile case)' ' > try_repo 26 unset "$here/26/.git" unset gitfile true \ > "$here/26.git" "(null)" "$here/26" "(null)" \ > - "$here/26.git" "(null)" "$here/26/sub" "(null)" && > + "$here/26.git" "(null)" "$here/26/sub" "(null)" > +' > + > +test_expect_success '#26.2: bare repo has no worktree (GIT_DIR -> gitfile case)' ' > try_repo 26b unset .git unset gitfile true \ > "$here/26b.git" "(null)" "$here/26b" "(null)" \ > "$here/26b.git" "(null)" "$here/26b/sub" "(null)" > @@ -779,9 +776,9 @@ test_expect_success '#29: setup' ' > cd 29 && > GIT_WORK_TREE="$here/29" && > export GIT_WORK_TREE && > - git status > - ) 2>message && > - test_must_be_empty message > + git status 2>message && > + test_must_be_empty message > + ) > ' > run_wt_tests 29 gitfile > > -- > 2.34.1.876.gdb91009a90c >
On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote: > On Wed, Dec 01, 2021 at 09:11:41PM +0100, Ævar Arnfjörð Bjarmason wrote: > > Amend the tests checking whether stderr is empty added in > > 4868b2ea17b (Subject: setup: officially support --work-tree without > > --git-dir, 2011-01-19) work portably on all POSIX shells, instead > > suppressing the trace output with "test_untraceable" on shells that > > aren't bash. > > > > The tests that used the "try_repo" helper wanted to check whether git > > commands invoked therein would emit anything on stderr. To do this > > they invoked the function and redirected the stderr to a "message" > > file. > > > > In 58275069288 (t1510-repo-setup: mark as untraceable with '-x', > > 2018-02-24) these were made to use "test_untraceable" introduced in > > 5fc98e79fc0 (t: add means to disable '-x' tracing for individual test > > scripts, 2018-02-24). > > > > It is better to have the "try_repo" function itself start with a > > "test_when_finished 'rm stderr'", and then redirect the stderr output > > from git commands it invokes via its helpers to a "stderr" file. > > > > This means that if we have a failure it'll be closer to the source of > > the problem, and most importantly isn't incompatible with "-x" on > > shells that aren't "bash". > > > > We also need to split those tests that had two "try_repo" invocations > > into different tests, which'll further help to narrow down any > > potential failures. This wasn't strictly necessary (an artifact of the > > use of "test_when_finished"), but the pattern enforces better test > > hygiene. > > > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > --- > > t/t1510-repo-setup.sh | 83 +++++++++++++++++++++---------------------- > > 1 file changed, 40 insertions(+), 43 deletions(-) > > > > diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh > > index 591505a39c0..f1748ac4a19 100755 > > --- a/t/t1510-repo-setup.sh > > +++ b/t/t1510-repo-setup.sh > > @@ -40,9 +40,6 @@ A few rules for repo setup: > > prefix is NULL. > > " > > > > -# This test heavily relies on the standard error of nested function calls. > > -test_untraceable=UnfortunatelyYes > > - > > TEST_PASSES_SANITIZE_LEAK=true > > . ./test-lib.sh > > > > @@ -62,7 +59,7 @@ test_repo () { > > export GIT_WORK_TREE > > fi && > > rm -f trace && > > - GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null && > > + GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr && > > I suspect that it's lines like this that make Peff argue for > BASH_XTRACEFD :) > > While this is not a compound command, it does contain a command > substitution, and the trace generated when executing the command in > that command substitution goes to the command's stderr, and then, > because of the redirection, to the 'stderr' file. > > I find it suspicious that this doesn't trigger a failure in a > 'test_must_be_empty stderr' later on. Ah, that's because this hunk is executed in a subshell that starts with 'cd "$1"', creating an extra 'stderr' file in a subdirectory: $ ./t1510-repo-setup.sh -r 1 -d [...] $ find trash\ directory.t1510-repo-setup/ |grep stderr trash directory.t1510-repo-setup/0/stderr trash directory.t1510-repo-setup/0/sub/stderr Changing that redirection to '2>>"$TRASH_DIRECTORY"/stderr' makes a bunch of tests fail with: + test_must_be_empty stderr 'stderr' is not empty, it contains: + pwd error: last command exited with $?=1 + rm stderr not ok 1 - #0: nonbare repo, no explicit configuration
On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote: > > @@ -62,7 +59,7 @@ test_repo () { > > export GIT_WORK_TREE > > fi && > > rm -f trace && > > - GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null && > > + GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr && > > I suspect that it's lines like this that make Peff argue for > BASH_XTRACEFD :) > > While this is not a compound command, it does contain a command > substitution, and the trace generated when executing the command in > that command substitution goes to the command's stderr, and then, > because of the redirection, to the 'stderr' file. Better still, the behavior varies between shells: $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr' ++ echo foo + FOO=foo + echo main + set +x stdout:main $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr' + FOO=foo echo main + set +x stdout:main stderr:+ echo foo -Peff
On Fri, Dec 10 2021, Jeff King wrote: > On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote: > >> > @@ -62,7 +59,7 @@ test_repo () { >> > export GIT_WORK_TREE >> > fi && >> > rm -f trace && >> > - GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null && >> > + GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr && >> >> I suspect that it's lines like this that make Peff argue for >> BASH_XTRACEFD :) >> >> While this is not a compound command, it does contain a command >> substitution, and the trace generated when executing the command in >> that command substitution goes to the command's stderr, and then, >> because of the redirection, to the 'stderr' file. > > Better still, the behavior varies between shells: > > $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr' > ++ echo foo > + FOO=foo > + echo main > + set +x > stdout:main > > $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr' > + FOO=foo echo main > + set +x > stdout:main > stderr:+ echo foo > > -Peff I just re-rolled a v3 with a fix for this and the general issue pointed out by SZEDER, thanks both. But as far as keeping test_untraceable goes, isn't this a good argument for dropping it? I.e. if bash was the odd shell out that included the $(...) command in the trace output it would be a good reason to keep it, then we could perhaps use it without worrying about our trace output being corrupted when we did a "test_cmp" on "stderr" later. But it's "dash" that's including it, so unless we're going to outright forbid it to have -x output (which is inconvienent) we'll need to deal with t either way (in my re-roll I just replaced $(pwd) with $PWD).
On Fri, Dec 10, 2021 at 04:47:23AM -0500, Jeff King wrote: > On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote: > > > > @@ -62,7 +59,7 @@ test_repo () { > > > export GIT_WORK_TREE > > > fi && > > > rm -f trace && > > > - GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null && > > > + GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr && > > > > I suspect that it's lines like this that make Peff argue for > > BASH_XTRACEFD :) > > > > While this is not a compound command, it does contain a command > > substitution, and the trace generated when executing the command in > > that command substitution goes to the command's stderr, and then, > > because of the redirection, to the 'stderr' file. > > Better still, the behavior varies between shells: Indeed, although POSIX seems to be quite clear about what should happen in this case: the specs for simple commands [1] state that redirections should be performed before variable assignments are expanded for, among other things, command substitution. > $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr' > ++ echo foo > + FOO=foo > + echo main > + set +x > stdout:main > > $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr' > + FOO=foo echo main > + set +x > stdout:main > stderr:+ echo foo So in case of these commands the shell should first redirect stdout and stderr, then expand the command substitution, thus it should write the trace for the 'echo foo' within to stderr _while_ stderr is redirected. It seems that dash, for once, does conform to POSIX. Although the standard-conform behavior is potentially more problematic for us, because it would cause test failure with tracing enabled when used like this: GIT_TRACE="$(pwd)/trace" git cmd --opts 2>actual.err && test_cmp expected.err actual.err because the "+ pwd" trace output from the command substitution would go to 'err.actual'; 9b2ac68f27 (t5526: use $TRASH_DIRECTORY to specify the path of GIT_TRACE log file, 2018-02-24) fixed a case like this. [1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh index 591505a39c0..f1748ac4a19 100755 --- a/t/t1510-repo-setup.sh +++ b/t/t1510-repo-setup.sh @@ -40,9 +40,6 @@ A few rules for repo setup: prefix is NULL. " -# This test heavily relies on the standard error of nested function calls. -test_untraceable=UnfortunatelyYes - TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh @@ -62,7 +59,7 @@ test_repo () { export GIT_WORK_TREE fi && rm -f trace && - GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null && + GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr && grep '^setup: ' trace >result && test_cmp expected result ) @@ -72,7 +69,7 @@ maybe_config () { file=$1 var=$2 value=$3 && if test "$value" != unset then - git config --file="$file" "$var" "$value" + git config --file="$file" "$var" "$value" 2>>stderr fi } @@ -80,7 +77,7 @@ setup_repo () { name=$1 worktreecfg=$2 gitfile=$3 barecfg=$4 && sane_unset GIT_DIR GIT_WORK_TREE && - git -c init.defaultBranch=initial init "$name" && + git -c init.defaultBranch=initial init "$name" 2>>stderr && maybe_config "$name/.git/config" core.worktree "$worktreecfg" && maybe_config "$name/.git/config" core.bare "$barecfg" && mkdir -p "$name/sub/sub" && @@ -210,10 +207,12 @@ run_wt_tests () { # (git dir) (work tree) (cwd) (prefix) <-- from subdir try_repo () { name=$1 worktreeenv=$2 gitdirenv=$3 && + test_when_finished "rm stderr" && setup_repo "$name" "$4" "$5" "$6" && shift 6 && try_case "$name" "$worktreeenv" "$gitdirenv" \ "$1" "$2" "$3" "$4" && + test_must_be_empty stderr && shift 4 && case "$gitdirenv" in /* | ?:/* | unset) ;; @@ -221,7 +220,8 @@ try_repo () { gitdirenv=../$gitdirenv ;; esac && try_case "$name/sub" "$worktreeenv" "$gitdirenv" \ - "$1" "$2" "$3" "$4" + "$1" "$2" "$3" "$4" && + test_must_be_empty stderr } # Bit 0 = GIT_WORK_TREE @@ -234,15 +234,13 @@ try_repo () { test_expect_success '#0: nonbare repo, no explicit configuration' ' try_repo 0 unset unset unset "" unset \ .git "$here/0" "$here/0" "(null)" \ - .git "$here/0" "$here/0" sub/ 2>message && - test_must_be_empty message + .git "$here/0" "$here/0" sub/ ' test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' ' try_repo 1 "$here" unset unset "" unset \ "$here/1/.git" "$here" "$here" 1/ \ - "$here/1/.git" "$here" "$here" 1/sub/ 2>message && - test_must_be_empty message + "$here/1/.git" "$here" "$here" 1/sub/ ' test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' ' @@ -268,19 +266,20 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' ' mkdir -p 4/sub sub && try_case 4 unset unset \ .git "$here/4/sub" "$here/4" "(null)" \ - "$here/4/.git" "$here/4/sub" "$here/4/sub" "(null)" 2>message && - test_must_be_empty message + "$here/4/.git" "$here/4/sub" "$here/4/sub" "(null)" ' -test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' ' +test_expect_success '#5.1: core.worktree + GIT_WORK_TREE is accepted' ' # or: you cannot intimidate away the lack of GIT_DIR setting try_repo 5 "$here" unset "$here/5" "" unset \ "$here/5/.git" "$here" "$here" 5/ \ - "$here/5/.git" "$here" "$here" 5/sub/ 2>message && + "$here/5/.git" "$here" "$here" 5/sub/ +' + +test_expect_success '#5.2: core.worktree + GIT_WORK_TREE is accepted' ' try_repo 5a .. unset "$here/5a" "" unset \ "$here/5a/.git" "$here" "$here" 5a/ \ - "$here/5a/.git" "$here/5a" "$here/5a" sub/ && - test_must_be_empty message + "$here/5a/.git" "$here/5a" "$here/5a" sub/ ' test_expect_success '#6: setting GIT_DIR brings core.worktree to life' ' @@ -376,8 +375,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' ' mkdir -p 9/wt && try_repo 9 wt unset unset gitfile unset \ "$here/9.git" "$here/9/wt" "$here/9" "(null)" \ - "$here/9.git" "$here/9/sub/wt" "$here/9/sub" "(null)" 2>message && - test_must_be_empty message + "$here/9.git" "$here/9/sub/wt" "$here/9/sub" "(null)" ' test_expect_success '#10: GIT_DIR can point to gitfile' ' @@ -402,16 +400,14 @@ run_wt_tests 11 gitfile test_expect_success '#12: core.worktree with gitfile is accepted' ' try_repo 12 unset unset "$here/12" gitfile unset \ "$here/12.git" "$here/12" "$here/12" "(null)" \ - "$here/12.git" "$here/12" "$here/12" sub/ 2>message && - test_must_be_empty message + "$here/12.git" "$here/12" "$here/12" sub/ ' test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' ' # or: you cannot intimidate away the lack of GIT_DIR setting try_repo 13 non-existent-too unset non-existent gitfile unset \ "$here/13.git" "$here/13/non-existent-too" "$here/13" "(null)" \ - "$here/13.git" "$here/13/sub/non-existent-too" "$here/13/sub" "(null)" 2>message && - test_must_be_empty message + "$here/13.git" "$here/13/sub/non-existent-too" "$here/13/sub" "(null)" ' # case #14. @@ -549,8 +545,7 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba mkdir -p 17b/.git/wt/sub && try_case 17a/.git "$here/17a" unset \ - "$here/17a/.git" "$here/17a" "$here/17a" .git/ \ - 2>message && + "$here/17a/.git" "$here/17a" "$here/17a" .git/ && try_case 17a/.git/wt "$here/17a" unset \ "$here/17a/.git" "$here/17a" "$here/17a" .git/wt/ && try_case 17a/.git/wt/sub "$here/17a" unset \ @@ -565,14 +560,16 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba try_repo 17c "$here/17c" unset unset "" true \ .git "$here/17c" "$here/17c" "(null)" \ - "$here/17c/.git" "$here/17c" "$here/17c" sub/ 2>message && - test_must_be_empty message + "$here/17c/.git" "$here/17c" "$here/17c" sub/ ' -test_expect_success '#18: bare .git named by GIT_DIR has no worktree' ' +test_expect_success '#18.1: bare .git named by GIT_DIR has no worktree' ' try_repo 18 unset .git unset "" true \ .git "(null)" "$here/18" "(null)" \ - ../.git "(null)" "$here/18/sub" "(null)" && + ../.git "(null)" "$here/18/sub" "(null)" +' + +test_expect_success '#18.2: bare .git named by GIT_DIR has no worktree' ' try_repo 18b unset "$here/18b/.git" unset "" true \ "$here/18b/.git" "(null)" "$here/18b" "(null)" \ "$here/18b/.git" "(null)" "$here/18b/sub" "(null)" @@ -590,12 +587,11 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)' setup_repo 20a "$here/20a" "" unset && mkdir -p 20a/.git/wt/sub && try_case 20a/.git unset unset \ - "$here/20a/.git" "$here/20a" "$here/20a" .git/ 2>message && + "$here/20a/.git" "$here/20a" "$here/20a" .git/ && try_case 20a/.git/wt unset unset \ "$here/20a/.git" "$here/20a" "$here/20a" .git/wt/ && try_case 20a/.git/wt/sub unset unset \ - "$here/20a/.git" "$here/20a" "$here/20a" .git/wt/sub/ && - test_must_be_empty message + "$here/20a/.git" "$here/20a" "$here/20a" .git/wt/sub/ ' test_expect_success '#20b/c: core.worktree and core.bare conflict' ' @@ -625,10 +621,9 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare cd 21/.git && GIT_WORK_TREE="$here/21" && export GIT_WORK_TREE && - git status >/dev/null - ) 2>message && - test_must_be_empty message - + git status 2>message && + test_must_be_empty message + ) ' run_wt_tests 21 @@ -742,14 +737,16 @@ test_expect_success '#24: bare repo has no worktree (gitfile case)' ' test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile case)' ' try_repo 25 "$here/25" unset unset gitfile true \ "$here/25.git" "$here/25" "$here/25" "(null)" \ - "$here/25.git" "$here/25" "$here/25" "sub/" 2>message && - test_must_be_empty message + "$here/25.git" "$here/25" "$here/25" "sub/" ' -test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' ' +test_expect_success '#26.1: bare repo has no worktree (GIT_DIR -> gitfile case)' ' try_repo 26 unset "$here/26/.git" unset gitfile true \ "$here/26.git" "(null)" "$here/26" "(null)" \ - "$here/26.git" "(null)" "$here/26/sub" "(null)" && + "$here/26.git" "(null)" "$here/26/sub" "(null)" +' + +test_expect_success '#26.2: bare repo has no worktree (GIT_DIR -> gitfile case)' ' try_repo 26b unset .git unset gitfile true \ "$here/26b.git" "(null)" "$here/26b" "(null)" \ "$here/26b.git" "(null)" "$here/26b/sub" "(null)" @@ -779,9 +776,9 @@ test_expect_success '#29: setup' ' cd 29 && GIT_WORK_TREE="$here/29" && export GIT_WORK_TREE && - git status - ) 2>message && - test_must_be_empty message + git status 2>message && + test_must_be_empty message + ) ' run_wt_tests 29 gitfile
Amend the tests checking whether stderr is empty added in 4868b2ea17b (Subject: setup: officially support --work-tree without --git-dir, 2011-01-19) work portably on all POSIX shells, instead suppressing the trace output with "test_untraceable" on shells that aren't bash. The tests that used the "try_repo" helper wanted to check whether git commands invoked therein would emit anything on stderr. To do this they invoked the function and redirected the stderr to a "message" file. In 58275069288 (t1510-repo-setup: mark as untraceable with '-x', 2018-02-24) these were made to use "test_untraceable" introduced in 5fc98e79fc0 (t: add means to disable '-x' tracing for individual test scripts, 2018-02-24). It is better to have the "try_repo" function itself start with a "test_when_finished 'rm stderr'", and then redirect the stderr output from git commands it invokes via its helpers to a "stderr" file. This means that if we have a failure it'll be closer to the source of the problem, and most importantly isn't incompatible with "-x" on shells that aren't "bash". We also need to split those tests that had two "try_repo" invocations into different tests, which'll further help to narrow down any potential failures. This wasn't strictly necessary (an artifact of the use of "test_when_finished"), but the pattern enforces better test hygiene. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t1510-repo-setup.sh | 83 +++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 43 deletions(-)