Re: [PATCH v2 11/11] tests: add a "set -o pipefail" for a patched bash

Junio C Hamano [off-list ref] writes:

...
The above have already !MINGW

quoted

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 4f7e62d05c..7b5d92add5 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh

@@ -251,7 +251,10 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 		i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) &&
-	test_match_signal 13 "$OUT" &&
+	if ! test_have_prereq BASH_SET_O_PIPEFAIL
+	then
+		test_match_signal 13 "$OUT"
+	fi &&
 	test_path_is_missing .git/index.lock
 '

but this one does not.  Yet, we've been using test_match_signal on 13
without issues, it appears.

And somehow with the lazy prereq on SET_O_PIPEFAIL, this part starts
to break, like so:

  https://github.com/git/git/runs/1752687552?check_suite_focus=true#step:7:37042

The output captured in OUT is 0 as we can see on #37032 in the test
log.

Nah, it seems that t3600-rm's "match signal 13" is already broken
without O_PIPEFAIL patch on Windows.  For example:

https://github.com/git/git/runs/1753231308?check_suite_focus=true#step:7:36912

This was introduced by c15ffae5 (rm tests: actually test for SIGPIPE
in SIGPIPE test, 2021-01-16) in the same series.

I am not sure "actually testing for SIGPIPE" is more important than
"make sure 'git rm' choked should not die with cruft", so without
thinking too deeply about the issue, my gut reaction is that
reverting is better than using !MINGW as other tests.  That is, no
matter how "git rm" gets killed, it should not leave .git/index.lock
behind, and the original already tests that.  The patch tried to
make sure it dies with signal #13 (and fails the test on Windows)
before it even looks at the leftover index.lock file, which feels a
bit backwards.

On Sat, Jan 23 2021, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

...
The above have already !MINGW

quoted

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 4f7e62d05c..7b5d92add5 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh

@@ -251,7 +251,10 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 		i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) &&
-	test_match_signal 13 "$OUT" &&
+	if ! test_have_prereq BASH_SET_O_PIPEFAIL
+	then
+		test_match_signal 13 "$OUT"
+	fi &&
 	test_path_is_missing .git/index.lock
 '

but this one does not.  Yet, we've been using test_match_signal on 13
without issues, it appears.

And somehow with the lazy prereq on SET_O_PIPEFAIL, this part starts
to break, like so:

  https://github.com/git/git/runs/1752687552?check_suite_focus=true#step:7:37042

The output captured in OUT is 0 as we can see on #37032 in the test
log.

Nah, it seems that t3600-rm's "match signal 13" is already broken
without O_PIPEFAIL patch on Windows.  For example:

https://github.com/git/git/runs/1753231308?check_suite_focus=true#step:7:36912

This was introduced by c15ffae5 (rm tests: actually test for SIGPIPE
in SIGPIPE test, 2021-01-16) in the same series.

Yes, not adding !MINGW here is a stupid oversight on my part, I can
re-roll with that added, which seems to be like it'll work & be
better. I.e. we'll actually test for SIGPIPE ...(read on)....

I am not sure "actually testing for SIGPIPE" is more important than
"make sure 'git rm' choked should not die with cruft", so without
thinking too deeply about the issue, my gut reaction is that
reverting is better than using !MINGW as other tests.  That is, no
matter how "git rm" gets killed, it should not leave .git/index.lock
behind, and the original already tests that.

I don't get it. I understand why we'd do any of:

 1. Keep my patch with !MINGW added. I.e. the intent of your 0693f9ddad
    (Make sure lockfiles are unlocked when dying on SIGPIPE, 2008-12-18)
    which added the test is to explicitly stress SIGPIPE, but we never
    actually checked it explicitly...

 2. Just remove/rewrite that part of the test. We have 7559a1be8a
    (unblock and unignore SIGPIPE, 2014-09-18) (the other test whose
    pattern I copied) now.

    That along with 12e0437f23 (common-main: call
    restore_sigpipe_to_default(), 2016-07-01) means we do this
    everywhere, so why test "git rm" in particular in this one place but
    not other git commands?

 3. Remove the overly specific PIPE test added in 7559a1be8a in favor of
    this "git rm" test. After all if we want to test the SIGPIPE pattern
    but sometimes we get SIGPIPE, sometimes we don't (MINGW), but we
    don't really care because we assume on some platforms it's being
    tested.

But not why we'd keep the test as-is now that we've dug up this old code
and found that since it got added we have a reliable way to test for
actually-sigpipe.

Just to maintain the coverage on MINGW? Wouldn't it be better to have
two tests then, one without the prereq to run everywhere, and another
identical one with the "trap" on !MINGW?

I don't really care and can re-roll in whatever way you prefer, I just
don't understand what I'd put as a reason in the commit message(s),
depending on which route we go...

Refactor the cache-tree test file to use our current recommended
patterns. This makes a subsequent meaningful change easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t0090-cache-tree.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 5a633690bf..45e1cc82ed 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh

@@ -10,7 +10,8 @@ cache-tree extension.
 cmp_cache_tree () {
 	test-tool dump-cache-tree | sed -e '/#(ref)/d' >actual &&
 	sed "s/$OID_REGEX/SHA/" <actual >filtered &&
-	test_cmp "$1" filtered
+	test_cmp "$1" filtered &&
+	rm filtered
 }
 
 # We don't bother with actually checking the SHA1:

@@ -54,7 +55,7 @@ test_invalid_cache_tree () {
 }
 
 test_no_cache_tree () {
-	: >expect &&
+	>expect &&
 	cmp_cache_tree expect
 }
 

@@ -83,18 +84,6 @@ test_expect_success 'git-add in subdir invalidates cache-tree' '
 	test_invalid_cache_tree
 '
 
-cat >before <<\EOF
-SHA  (3 entries, 2 subtrees)
-SHA dir1/ (1 entries, 0 subtrees)
-SHA dir2/ (1 entries, 0 subtrees)
-EOF
-
-cat >expect <<\EOF
-invalid                                   (2 subtrees)
-invalid                                  dir1/ (0 subtrees)
-SHA dir2/ (1 entries, 0 subtrees)
-EOF
-
 test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '
 	git tag no-children &&
 	test_when_finished "git reset --hard no-children; git read-tree HEAD" &&

@@ -102,9 +91,20 @@ test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '
 	test_commit dir1/a &&
 	test_commit dir2/b &&
 	echo "I changed this file" >dir1/a &&
+	test_when_finished "rm before" &&
+	cat >before <<-\EOF &&
+	SHA  (3 entries, 2 subtrees)
+	SHA dir1/ (1 entries, 0 subtrees)
+	SHA dir2/ (1 entries, 0 subtrees)
+	EOF
 	cmp_cache_tree before &&
 	echo "I changed this file" >dir1/a &&
 	git add dir1/a &&
+	cat >expect <<-\EOF &&
+	invalid                                   (2 subtrees)
+	invalid                                  dir1/ (0 subtrees)
+	SHA dir2/ (1 entries, 0 subtrees)
+	EOF
 	cmp_cache_tree expect
 '
 

-- 
2.29.2.222.g5d2a92d10f8

This started as an attempt to add a bash "set -o pipefail" test mode,
but now comes without that. Junio suggested dropping it in
[off-list ref].

The "cache-tree tests" part is mostly rewritten. I'd removed the index
dependency of the tests, but the point of the tests is to test the
index. Now we do that again in a more readable way.

The "git rm" test at the end fixes the current CI failure in this
topic, and does some version of what I suggested in
[off-list ref]. Junio, I think that makes sense
as a fix while we're at it, but if you don't like it just drop it.

Jeff King (1):
  git-svn tests: rewrite brittle tests to use "--[no-]merges".

Ævar Arnfjörð Bjarmason (9):
  cache-tree tests: refactor for modern test style
  cache-tree tests: remove unused $2 parameter
  cache-tree tests: use a sub-shell with less indirection
  cache-tree tests: explicitly test HEAD and index differences
  git svn mergeinfo tests: modernize redirection & quoting style
  git svn mergeinfo tests: refactor "test -z" to use test_must_be_empty
  upload-pack tests: avoid a non-zero "grep" exit status
  archive tests: use a cheaper "zipinfo -h" invocation to get header
  rm tests: actually test for SIGPIPE in SIGPIPE test

 t/t0090-cache-tree.sh              | 82 +++++++++++++++---------------
 t/t3600-rm.sh                      | 16 +++++-
 t/t5004-archive-corner-cases.sh    |  3 +-
 t/t5703-upload-pack-ref-in-want.sh |  3 +-
 t/t9151-svn-mergeinfo.sh           | 43 ++++++++--------
 5 files changed, 80 insertions(+), 67 deletions(-)

Range-diff:
 -:  ---------- >  1:  b30499c4e4 cache-tree tests: refactor for modern test style
 1:  8e8e03fa3d =  2:  af0b25a048 cache-tree tests: remove unused $2 parameter
 2:  828d25533c !  3:  09959568de cache-tree tests: use a sub-shell with less indirection
    @@ Commit message
         We did actually recover correctly if we failed in this function since
         we were wrapped in a subshell one function call up. Let's just use the
         sub-shell at the point where we want to change the directory
    -    instead. This also allows us to get rid of the wrapper function.
    +    instead.
    +
    +    It's important that the "|| return 1" is outside the
    +    subshell. Normally, we `exit 1` from within subshells[1], but that
    +    wouldn't help us exit this loop early[1][2].
    +
    +    Since we can get rid of the wrapper function let's rename the main
    +    function to drop the "rec" (for "recursion") suffix[3].
    +
    +    1. https://lore.kernel.org/git/CAPig+cToj8nQmyBCqC1k7DXF2vXaonCEA-fCJ4x7JBZG2ixYBw@mail.gmail.com/
    +    2. https://lore.kernel.org/git/20150325052952.GE31924@peff.net/
    +    3. https://lore.kernel.org/git/YARsCsgXuiXr4uFX@coredump.intra.peff.net/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## t/t0090-cache-tree.sh ##
    +@@ t/t0090-cache-tree.sh: cmp_cache_tree () {
    + # We don't bother with actually checking the SHA1:
    + # test-tool dump-cache-tree already verifies that all existing data is
    + # correct.
    +-generate_expected_cache_tree_rec () {
    ++generate_expected_cache_tree () {
    + 	dir="$1${1:+/}" &&
    + 	# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
    + 	# We want to count only foo because it's the only direct child
     @@ t/t0090-cache-tree.sh: generate_expected_cache_tree_rec () {
      	printf "SHA $dir (%d entries, %d subtrees)\n" "$entries" "$subtree_count" &&
      	for subtree in $subtrees
    @@ t/t0090-cache-tree.sh: generate_expected_cache_tree_rec () {
     -		generate_expected_cache_tree_rec "$dir$subtree" || return 1
     -		cd ..
     +		(
    -+			cd "$subtree"
    -+			generate_expected_cache_tree_rec "$dir$subtree" || return 1
    -+		)
    ++			cd "$subtree" &&
    ++			generate_expected_cache_tree "$dir$subtree"
    ++		) || return 1
      	done
      }
      
    @@ t/t0090-cache-tree.sh: generate_expected_cache_tree_rec () {
     -}
     -
      test_cache_tree () {
    --	generate_expected_cache_tree >expect &&
    -+	generate_expected_cache_tree_rec >expect &&
    + 	generate_expected_cache_tree >expect &&
      	cmp_cache_tree expect
    - }
    - 
 3:  fefdc570a5 !  4:  697b0084fd cache-tree tests: refactor overly complex function
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## Commit message ##
    -    cache-tree tests: refactor overly complex function
    +    cache-tree tests: explicitly test HEAD and index differences
     
    -    Refactor overly complex code added in 9c4d6c0297 (cache-tree: Write
    -    updated cache-tree after commit, 2014-07-13).
    +    The test code added in 9c4d6c0297 (cache-tree: Write updated
    +    cache-tree after commit, 2014-07-13) used "ls-files" in lieu of
    +    "ls-tree" because it wanted to test the data in the index, since this
    +    test is testing the cache-tree extension.
     
    -    Interestingly, in the numerous commits[1][2][3] who fixed commits bugs
    -    in this code since its introduction it seems not to have been noticed
    -    that we didn't need to be doing some dance with grep/cut/uniq/awk to
    -    extract this information. It can be done in a much simpler way with
    -    just "ls-tree" and "wc -l".
    +    Change the test to instead use "ls-tree" for traversal, and then
    +    explicitly check how HEAD differs from the index. This is more easily
    +    understood, and less fragile as numerous past bug fixes[1][2][3] to
    +    the old code we're replacing demonstrate.
     
    -    I'm also removing the comment, because I think now that this code is
    -    trivial to understand it's not needed anymore.
    +    As an aside this would be a bit easier if empty pathspecs hadn't been
    +    made an error in d426430e6e (pathspec: warn on empty strings as
    +    pathspec, 2016-06-22) and 9e4e8a64c2 (pathspec: die on empty strings
    +    as pathspec, 2017-06-06).
    +
    +    If that was still allowed this code could be simplified slightly:
    +
    +            diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
    +            index 9bf66c9e68..0b02881f55 100755
    +            --- a/t/t0090-cache-tree.sh
    +            +++ b/t/t0090-cache-tree.sh
    +            @@ -18,19 +18,18 @@ cmp_cache_tree () {
    +             # test-tool dump-cache-tree already verifies that all existing data is
    +             # correct.
    +             generate_expected_cache_tree () {
    +            -       pathspec="$1" &&
    +            -       dir="$2${2:+/}" &&
    +            +       pathspec="$1${1:+/}" &&
    +                    git ls-tree --name-only HEAD -- "$pathspec" >files &&
    +                    git ls-tree --name-only -d HEAD -- "$pathspec" >subtrees &&
    +            -       printf "SHA %s (%d entries, %d subtrees)\n" "$dir" $(wc -l <files) $(wc -l <subtrees) &&
    +            +       printf "SHA %s (%d entries, %d subtrees)\n" "$pathspec" $(wc -l <files) $(wc -l <subtrees) &&
    +                    while read subtree
    +                    do
    +            -               generate_expected_cache_tree "$pathspec/$subtree/" "$subtree" || return 1
    +            +               generate_expected_cache_tree "$subtree" || return 1
    +                    done <subtrees
    +             }
    +
    +             test_cache_tree () {
    +            -       generate_expected_cache_tree "." >expect &&
    +            +       generate_expected_cache_tree >expect &&
    +                    cmp_cache_tree expect &&
    +                    rm expect actual files subtrees &&
    +                    git status --porcelain -- ':!status' ':!expected.status' >status &&
     
         1. c8db708d5d (t0090: avoid passing empty string to printf %d,
            2014-09-30)
    @@ Commit message
     
      ## t/t0090-cache-tree.sh ##
     @@ t/t0090-cache-tree.sh: cmp_cache_tree () {
    + # test-tool dump-cache-tree already verifies that all existing data is
      # correct.
    - generate_expected_cache_tree_rec () {
    - 	dir="$1${1:+/}" &&
    + generate_expected_cache_tree () {
    +-	dir="$1${1:+/}" &&
     -	# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
     -	# We want to count only foo because it's the only direct child
     -	git ls-files >files &&
    @@ t/t0090-cache-tree.sh: cmp_cache_tree () {
     -	entries=$(wc -l <files) &&
     -	printf "SHA $dir (%d entries, %d subtrees)\n" "$entries" "$subtree_count" &&
     -	for subtree in $subtrees
    -+	git ls-tree --name-only HEAD >files &&
    -+	git ls-tree --name-only -d HEAD >subtrees &&
    ++	pathspec="$1" &&
    ++	dir="$2${2:+/}" &&
    ++	git ls-tree --name-only HEAD -- "$pathspec" >files &&
    ++	git ls-tree --name-only -d HEAD -- "$pathspec" >subtrees &&
     +	printf "SHA %s (%d entries, %d subtrees)\n" "$dir" $(wc -l <files) $(wc -l <subtrees) &&
     +	while read subtree
      	do
    - 		(
    - 			cd "$subtree"
    --			generate_expected_cache_tree_rec "$dir$subtree" || return 1
    -+			generate_expected_cache_tree_rec "$subtree" || return 1
    - 		)
    +-		(
    +-			cd "$subtree" &&
    +-			generate_expected_cache_tree "$dir$subtree"
    +-		) || return 1
     -	done
    ++		generate_expected_cache_tree "$pathspec/$subtree/" "$subtree" || return 1
     +	done <subtrees
      }
      
      test_cache_tree () {
    +-	generate_expected_cache_tree >expect &&
    +-	cmp_cache_tree expect
    ++	generate_expected_cache_tree "." >expect &&
    ++	cmp_cache_tree expect &&
    ++	rm expect actual files subtrees &&
    ++	git status --porcelain -- ':!status' ':!expected.status' >status &&
    ++	if test -n "$1"
    ++	then
    ++		test_cmp "$1" status
    ++	else
    ++		test_must_be_empty status
    ++	fi
    + }
    + 
    + test_invalid_cache_tree () {
    +@@ t/t0090-cache-tree.sh: test_expect_success 'second commit has cache-tree' '
    + '
    + 
    + test_expect_success PERL 'commit --interactive gives cache-tree on partial commit' '
    ++	test_when_finished "git reset --hard" &&
    + 	cat <<-\EOT >foo.c &&
    + 	int foo()
    + 	{
    +@@ t/t0090-cache-tree.sh: test_expect_success PERL 'commit --interactive gives cache-tree on partial commi
    + 	EOT
    + 	test_write_lines p 1 "" s n y q |
    + 	git commit --interactive -m foo &&
    +-	test_cache_tree
    ++	cat <<-\EOF >expected.status &&
    ++	 M foo.c
    ++	EOF
    ++	test_cache_tree expected.status
    + '
    + 
    + test_expect_success PERL 'commit -p with shrinking cache-tree' '
    +@@ t/t0090-cache-tree.sh: test_expect_success 'partial commit gives cache-tree' '
    + 	git add one.t &&
    + 	echo "some other change" >two.t &&
    + 	git commit two.t -m partial &&
    +-	test_cache_tree
    ++	cat <<-\EOF >expected.status &&
    ++	M  one.t
    ++	EOF
    ++	test_cache_tree expected.status
    + '
    + 
    + test_expect_success 'no phantom error when switching trees' '
 4:  a16938e58d =  5:  5ede74a1ab git svn mergeinfo tests: modernize redirection & quoting style
 5:  b520656240 =  6:  c287f5a24a git svn mergeinfo tests: refactor "test -z" to use test_must_be_empty
 6:  f2e70ac911 !  7:  9bd6ad6e25 git-svn tests: rewrite brittle tests to use "--[no-]merges".
    @@ Commit message
         to figure out if a set of commits turned into merge commits or not.
     
         Signed-off-by: Jeff King [off-list ref]
    +    [ÆAB: wrote commit message]
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
    -    Commit-message-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## t/t9151-svn-mergeinfo.sh ##
     @@ t/t9151-svn-mergeinfo.sh: test_expect_success 'load svn dump' "
 7:  dcf001e165 <  -:  ---------- rm tests: actually test for SIGPIPE in SIGPIPE test
 8:  2212fa65eb !  8:  fd40e818a7 upload-pack tests: avoid a non-zero "grep" exit status
    @@ Commit message
         upload-pack tests: avoid a non-zero "grep" exit status
     
         Continue changing a test that 763b47bafa (t5703: stop losing return
    -    codes of git commands, 2019-11-27) already refactored. A follow-up
    -    commit will add support for testing under bash's "set -o pipefail",
    -    under that mode this test will fail because sometimes there's no
    -    commits in the "objs" output.
    +    codes of git commands, 2019-11-27) already refactored.
     
    -    It's easier to just fix this than to exempt these tests under a
    -    soon-to-be added "set -o pipefail" test mode. So let's do that.
    +    This was originally added as part of a series to add support for
    +    running under bash's "set -o pipefail", under that mode this test will
    +    fail because sometimes there's no commits in the "objs" output.
    +
    +    It's easier to fix that than exempt these tests under a hypothetical
    +    "set -o pipefail" test mode. It looks like we probably won't have
    +    that, but once we've dug this code up let's refactor it[2] so we don't
    +    hide a potential pipe failure.
    +
    +    1. https://lore.kernel.org/git/xmqqzh18o8o6.fsf@gitster.c.googlers.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
    @@ t/t5703-upload-pack-ref-in-want.sh: get_actual_commits () {
      	git index-pack o.pack &&
      	git verify-pack -v o.idx >objs &&
     -	grep commit objs | cut -d" " -f1 | sort >actual_commits
    -+	>actual_commits &&
    -+	if grep -q commit objs
    -+	then
    -+		grep commit objs | cut -d" " -f1 | sort >actual_commits
    -+	fi
    ++	sed -n -e 's/\([0-9a-f][0-9a-f]*\) commit .*/\1/p' objs >objs.sed &&
    ++	sort >actual_commits <objs.sed
      }
      
      check_output () {
 9:  8167c2e346 =  9:  5405062665 archive tests: use a cheaper "zipinfo -h" invocation to get header
10:  30c454ae7c <  -:  ---------- tests: split up bash detection library
11:  6f290f850c <  -:  ---------- tests: add a "set -o pipefail" for a patched bash
 -:  ---------- > 10:  b526b3cb24 rm tests: actually test for SIGPIPE in SIGPIPE test
-- 
2.29.2.222.g5d2a92d10f8

Remove the $2 paramater. This appears to have been some
work-in-progress code from an earlier version of
9c4d6c0297 (cache-tree: Write updated cache-tree after commit,
2014-07-13) which was left in the final version.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t0090-cache-tree.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 45e1cc82ed..7ff7f04719 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh

@@ -19,7 +19,6 @@ cmp_cache_tree () {
 # correct.
 generate_expected_cache_tree_rec () {
 	dir="$1${1:+/}" &&
-	parent="$2" &&
 	# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
 	# We want to count only foo because it's the only direct child
 	git ls-files >files &&

@@ -30,10 +29,9 @@ generate_expected_cache_tree_rec () {
 	for subtree in $subtrees
 	do
 		cd "$subtree"
-		generate_expected_cache_tree_rec "$dir$subtree" "$dir" || return 1
+		generate_expected_cache_tree_rec "$dir$subtree" || return 1
 		cd ..
-	done &&
-	dir=$parent
+	done
 }
 
 generate_expected_cache_tree () {

-- 
2.29.2.222.g5d2a92d10f8

Change a "cd xyz && work && cd .." pattern introduced in
9c4d6c0297 (cache-tree: Write updated cache-tree after commit,
2014-07-13) to use a sub-shell instead with less indirection.

We did actually recover correctly if we failed in this function since
we were wrapped in a subshell one function call up. Let's just use the
sub-shell at the point where we want to change the directory
instead.

It's important that the "|| return 1" is outside the
subshell. Normally, we `exit 1` from within subshells[1], but that
wouldn't help us exit this loop early[1][2].

Since we can get rid of the wrapper function let's rename the main
function to drop the "rec" (for "recursion") suffix[3].

1. https://lore.kernel.org/git/CAPig+cToj8nQmyBCqC1k7DXF2vXaonCEA-fCJ4x7JBZG2ixYBw@mail.gmail.com/
2. https://lore.kernel.org/git/20150325052952.GE31924@peff.net/
3. https://lore.kernel.org/git/YARsCsgXuiXr4uFX@coredump.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t0090-cache-tree.sh | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 7ff7f04719..5bb4f75443 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh

@@ -17,7 +17,7 @@ cmp_cache_tree () {
 # We don't bother with actually checking the SHA1:
 # test-tool dump-cache-tree already verifies that all existing data is
 # correct.
-generate_expected_cache_tree_rec () {
+generate_expected_cache_tree () {
 	dir="$1${1:+/}" &&
 	# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
 	# We want to count only foo because it's the only direct child

@@ -28,18 +28,13 @@ generate_expected_cache_tree_rec () {
 	printf "SHA $dir (%d entries, %d subtrees)\n" "$entries" "$subtree_count" &&
 	for subtree in $subtrees
 	do
-		cd "$subtree"
-		generate_expected_cache_tree_rec "$dir$subtree" || return 1
-		cd ..
+		(
+			cd "$subtree" &&
+			generate_expected_cache_tree "$dir$subtree"
+		) || return 1
 	done
 }
 
-generate_expected_cache_tree () {
-	(
-		generate_expected_cache_tree_rec
-	)
-}
-
 test_cache_tree () {
 	generate_expected_cache_tree >expect &&
 	cmp_cache_tree expect

-- 
2.29.2.222.g5d2a92d10f8

The test code added in 9c4d6c0297 (cache-tree: Write updated
cache-tree after commit, 2014-07-13) used "ls-files" in lieu of
"ls-tree" because it wanted to test the data in the index, since this
test is testing the cache-tree extension.

Change the test to instead use "ls-tree" for traversal, and then
explicitly check how HEAD differs from the index. This is more easily
understood, and less fragile as numerous past bug fixes[1][2][3] to
the old code we're replacing demonstrate.

As an aside this would be a bit easier if empty pathspecs hadn't been
made an error in d426430e6e (pathspec: warn on empty strings as
pathspec, 2016-06-22) and 9e4e8a64c2 (pathspec: die on empty strings
as pathspec, 2017-06-06).

If that was still allowed this code could be simplified slightly:

	diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
	index 9bf66c9e68..0b02881f55 100755
	--- a/t/t0090-cache-tree.sh
	+++ b/t/t0090-cache-tree.sh
	@@ -18,19 +18,18 @@ cmp_cache_tree () {
	 # test-tool dump-cache-tree already verifies that all existing data is
	 # correct.
	 generate_expected_cache_tree () {
	-       pathspec="$1" &&
	-       dir="$2${2:+/}" &&
	+       pathspec="$1${1:+/}" &&
	        git ls-tree --name-only HEAD -- "$pathspec" >files &&
	        git ls-tree --name-only -d HEAD -- "$pathspec" >subtrees &&
	-       printf "SHA %s (%d entries, %d subtrees)\n" "$dir" $(wc -l <files) $(wc -l <subtrees) &&
	+       printf "SHA %s (%d entries, %d subtrees)\n" "$pathspec" $(wc -l <files) $(wc -l <subtrees) &&
	        while read subtree
	        do
	-               generate_expected_cache_tree "$pathspec/$subtree/" "$subtree" || return 1
	+               generate_expected_cache_tree "$subtree" || return 1
	        done <subtrees
	 }

	 test_cache_tree () {
	-       generate_expected_cache_tree "." >expect &&
	+       generate_expected_cache_tree >expect &&
	        cmp_cache_tree expect &&
	        rm expect actual files subtrees &&
	        git status --porcelain -- ':!status' ':!expected.status' >status &&

1. c8db708d5d (t0090: avoid passing empty string to printf %d,
   2014-09-30)
2. d69360c6b1 (t0090: tweak awk statement for Solaris
   /usr/xpg4/bin/awk, 2014-12-22)
3. 9b5a9fa60a (t0090: stop losing return codes of git commands,
   2019-11-27)

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t0090-cache-tree.sh | 45 ++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 5bb4f75443..9bf66c9e68 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh

@@ -18,26 +18,28 @@ cmp_cache_tree () {
 # test-tool dump-cache-tree already verifies that all existing data is
 # correct.
 generate_expected_cache_tree () {
-	dir="$1${1:+/}" &&
-	# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
-	# We want to count only foo because it's the only direct child
-	git ls-files >files &&
-	subtrees=$(grep / files|cut -d / -f 1|uniq) &&
-	subtree_count=$(echo "$subtrees"|awk -v c=0 '$1 != "" {++c} END {print c}') &&
-	entries=$(wc -l <files) &&
-	printf "SHA $dir (%d entries, %d subtrees)\n" "$entries" "$subtree_count" &&
-	for subtree in $subtrees
+	pathspec="$1" &&
+	dir="$2${2:+/}" &&
+	git ls-tree --name-only HEAD -- "$pathspec" >files &&
+	git ls-tree --name-only -d HEAD -- "$pathspec" >subtrees &&
+	printf "SHA %s (%d entries, %d subtrees)\n" "$dir" $(wc -l <files) $(wc -l <subtrees) &&
+	while read subtree
 	do
-		(
-			cd "$subtree" &&
-			generate_expected_cache_tree "$dir$subtree"
-		) || return 1
-	done
+		generate_expected_cache_tree "$pathspec/$subtree/" "$subtree" || return 1
+	done <subtrees
 }
 
 test_cache_tree () {
-	generate_expected_cache_tree >expect &&
-	cmp_cache_tree expect
+	generate_expected_cache_tree "." >expect &&
+	cmp_cache_tree expect &&
+	rm expect actual files subtrees &&
+	git status --porcelain -- ':!status' ':!expected.status' >status &&
+	if test -n "$1"
+	then
+		test_cmp "$1" status
+	else
+		test_must_be_empty status
+	fi
 }
 
 test_invalid_cache_tree () {

@@ -126,6 +128,7 @@ test_expect_success 'second commit has cache-tree' '
 '
 
 test_expect_success PERL 'commit --interactive gives cache-tree on partial commit' '
+	test_when_finished "git reset --hard" &&
 	cat <<-\EOT >foo.c &&
 	int foo()
 	{

@@ -152,7 +155,10 @@ test_expect_success PERL 'commit --interactive gives cache-tree on partial commi
 	EOT
 	test_write_lines p 1 "" s n y q |
 	git commit --interactive -m foo &&
-	test_cache_tree
+	cat <<-\EOF >expected.status &&
+	 M foo.c
+	EOF
+	test_cache_tree expected.status
 '
 
 test_expect_success PERL 'commit -p with shrinking cache-tree' '

@@ -243,7 +249,10 @@ test_expect_success 'partial commit gives cache-tree' '
 	git add one.t &&
 	echo "some other change" >two.t &&
 	git commit two.t -m partial &&
-	test_cache_tree
+	cat <<-\EOF >expected.status &&
+	M  one.t
+	EOF
+	test_cache_tree expected.status
 '
 
 test_expect_success 'no phantom error when switching trees' '

-- 
2.29.2.222.g5d2a92d10f8

Use "<file" instead of "< file", and don't put the closing quote for
strings on an indented line. This makes a follow-up refactoring commit
easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t9151-svn-mergeinfo.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t9151-svn-mergeinfo.sh b/t/t9151-svn-mergeinfo.sh
index 4f6c06ecb2..59c5847c5f 100755
--- a/t/t9151-svn-mergeinfo.sh
+++ b/t/t9151-svn-mergeinfo.sh

@@ -9,37 +9,37 @@ test_description='git-svn svn mergeinfo properties'
 
 test_expect_success 'load svn dump' "
 	svnadmin load -q '$rawsvnrepo' \
-	  < '$TEST_DIRECTORY/t9151/svn-mergeinfo.dump' &&
+	  <'$TEST_DIRECTORY/t9151/svn-mergeinfo.dump' &&
 	git svn init --minimize-url -R svnmerge \
 	  --rewrite-root=http://svn.example.org \
 	  -T trunk -b branches '$svnrepo' &&
 	git svn fetch --all
-	"
+"
 
 test_expect_success 'all svn merges became git merge commits' '
 	unmarked=$(git rev-list --parents --all --grep=Merge |
 		grep -v " .* " | cut -f1 -d" ") &&
 	[ -z "$unmarked" ]
-	'
+'
 
 test_expect_success 'cherry picks did not become git merge commits' '
 	bad_cherries=$(git rev-list --parents --all --grep=Cherry |
 		grep " .* " | cut -f1 -d" ") &&
 	[ -z "$bad_cherries" ]
-	'
+'
 
 test_expect_success 'svn non-merge merge commits did not become git merge commits' '
 	bad_non_merges=$(git rev-list --parents --all --grep=non-merge |
 		grep " .* " | cut -f1 -d" ") &&
 	[ -z "$bad_non_merges" ]
-	'
+'
 
 test_expect_success 'commit made to merged branch is reachable from the merge' '
 	before_commit=$(git rev-list --all --grep="trunk commit before merging trunk to b2") &&
 	merge_commit=$(git rev-list --all --grep="Merge trunk to b2") &&
 	not_reachable=$(git rev-list -1 $before_commit --not $merge_commit) &&
 	[ -z "$not_reachable" ]
-	'
+'
 
 test_expect_success 'merging two branches in one commit is detected correctly' '
 	f1_commit=$(git rev-list --all --grep="make f1 branch from trunk") &&

@@ -47,11 +47,11 @@ test_expect_success 'merging two branches in one commit is detected correctly' '
 	merge_commit=$(git rev-list --all --grep="Merge f1 and f2 to trunk") &&
 	not_reachable=$(git rev-list -1 $f1_commit $f2_commit --not $merge_commit) &&
 	[ -z "$not_reachable" ]
-	'
+'
 
 test_expect_failure 'everything got merged in the end' '
 	unmerged=$(git rev-list --all --not master) &&
 	[ -z "$unmerged" ]
-	'
+'
 
 test_done

-- 
2.29.2.222.g5d2a92d10f8

Refactor some old-style test code to use test_must_be_empty instead of
"test -z". This makes a follow-up commit easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t9151-svn-mergeinfo.sh | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/t/t9151-svn-mergeinfo.sh b/t/t9151-svn-mergeinfo.sh
index 59c5847c5f..806eff4023 100755
--- a/t/t9151-svn-mergeinfo.sh
+++ b/t/t9151-svn-mergeinfo.sh

@@ -17,41 +17,42 @@ test_expect_success 'load svn dump' "
 "
 
 test_expect_success 'all svn merges became git merge commits' '
-	unmarked=$(git rev-list --parents --all --grep=Merge |
-		grep -v " .* " | cut -f1 -d" ") &&
-	[ -z "$unmarked" ]
+	git rev-list --parents --all --grep=Merge |
+		grep -v " .* " | cut -f1 -d" " >unmarked &&
+	test_must_be_empty unmarked
 '
 
+
 test_expect_success 'cherry picks did not become git merge commits' '
-	bad_cherries=$(git rev-list --parents --all --grep=Cherry |
-		grep " .* " | cut -f1 -d" ") &&
-	[ -z "$bad_cherries" ]
+	git rev-list --parents --all --grep=Cherry |
+		grep " .* " | cut -f1 -d" " >bad-cherries &&
+	test_must_be_empty bad-cherries
 '
 
 test_expect_success 'svn non-merge merge commits did not become git merge commits' '
-	bad_non_merges=$(git rev-list --parents --all --grep=non-merge |
-		grep " .* " | cut -f1 -d" ") &&
-	[ -z "$bad_non_merges" ]
+	git rev-list --parents --all --grep=non-merge |
+		grep " .* " | cut -f1 -d" " >bad-non-merges &&
+	test_must_be_empty bad-non-merges
 '
 
 test_expect_success 'commit made to merged branch is reachable from the merge' '
 	before_commit=$(git rev-list --all --grep="trunk commit before merging trunk to b2") &&
 	merge_commit=$(git rev-list --all --grep="Merge trunk to b2") &&
-	not_reachable=$(git rev-list -1 $before_commit --not $merge_commit) &&
-	[ -z "$not_reachable" ]
+	git rev-list -1 $before_commit --not $merge_commit >not-reachable &&
+	test_must_be_empty not-reachable
 '
 
 test_expect_success 'merging two branches in one commit is detected correctly' '
 	f1_commit=$(git rev-list --all --grep="make f1 branch from trunk") &&
 	f2_commit=$(git rev-list --all --grep="make f2 branch from trunk") &&
 	merge_commit=$(git rev-list --all --grep="Merge f1 and f2 to trunk") &&
-	not_reachable=$(git rev-list -1 $f1_commit $f2_commit --not $merge_commit) &&
-	[ -z "$not_reachable" ]
+	git rev-list -1 $f1_commit $f2_commit --not $merge_commit >not-reachable &&
+	test_must_be_empty not-reachable
 '
 
 test_expect_failure 'everything got merged in the end' '
-	unmerged=$(git rev-list --all --not master) &&
-	[ -z "$unmerged" ]
+	git rev-list --all --not master >unmerged &&
+	test_must_be_empty unmerged
 '
 
 test_done

-- 
2.29.2.222.g5d2a92d10f8

From: Jeff King <redacted>

Rewrite a brittle tests which used "rev-list" without "--[no-]merges"
to figure out if a set of commits turned into merge commits or not.

Signed-off-by: Jeff King <redacted>
[ÆAB: wrote commit message]
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t9151-svn-mergeinfo.sh | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/t/t9151-svn-mergeinfo.sh b/t/t9151-svn-mergeinfo.sh
index 806eff4023..c33bae91fb 100755
--- a/t/t9151-svn-mergeinfo.sh
+++ b/t/t9151-svn-mergeinfo.sh

@@ -17,21 +17,17 @@ test_expect_success 'load svn dump' "
 "
 
 test_expect_success 'all svn merges became git merge commits' '
-	git rev-list --parents --all --grep=Merge |
-		grep -v " .* " | cut -f1 -d" " >unmarked &&
+	git rev-list --all --no-merges --grep=Merge >unmarked &&
 	test_must_be_empty unmarked
 '
 
-
 test_expect_success 'cherry picks did not become git merge commits' '
-	git rev-list --parents --all --grep=Cherry |
-		grep " .* " | cut -f1 -d" " >bad-cherries &&
+	git rev-list --all --merges --grep=Cherry >bad-cherries &&
 	test_must_be_empty bad-cherries
 '
 
 test_expect_success 'svn non-merge merge commits did not become git merge commits' '
-	git rev-list --parents --all --grep=non-merge |
-		grep " .* " | cut -f1 -d" " >bad-non-merges &&
+	git rev-list --all --merges --grep=non-merge >bad-non-merges &&
 	test_must_be_empty bad-non-merges
 '
 

-- 
2.29.2.222.g5d2a92d10f8

Continue changing a test that 763b47bafa (t5703: stop losing return
codes of git commands, 2019-11-27) already refactored.

This was originally added as part of a series to add support for
running under bash's "set -o pipefail", under that mode this test will
fail because sometimes there's no commits in the "objs" output.

It's easier to fix that than exempt these tests under a hypothetical
"set -o pipefail" test mode. It looks like we probably won't have
that, but once we've dug this code up let's refactor it[2] so we don't
hide a potential pipe failure.

1. https://lore.kernel.org/git/xmqqzh18o8o6.fsf@gitster.c.googlers.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t5703-upload-pack-ref-in-want.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index eab966985b..5d825b1781 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh

@@ -16,7 +16,8 @@ get_actual_commits () {
 	test-tool pkt-line unpack-sideband <out >o.pack &&
 	git index-pack o.pack &&
 	git verify-pack -v o.idx >objs &&
-	grep commit objs | cut -d" " -f1 | sort >actual_commits
+	sed -n -e 's/\([0-9a-f][0-9a-f]*\) commit .*/\1/p' objs >objs.sed &&
+	sort >actual_commits <objs.sed
 }
 
 check_output () {

-- 
2.29.2.222.g5d2a92d10f8

Change a test initially added in 50cd31c652 (t3600: comment on
inducing SIGPIPE in `git rm`, 2019-11-27) to explicitly test for
SIGPIPE using a pattern initially established in 7559a1be8a (unblock
and unignore SIGPIPE, 2014-09-18).

The problem with using that pattern is that it requires us to skip the
test on MINGW[1]. If we kept the test with its initial semantics[2]
we'd get coverage there, at the cost of not checking whether we
actually had SIGPIPE outside of MinGW.

Arguably we should just remove this test. Between the test added in
7559a1be8a and the change made in 12e0437f23 (common-main: call
restore_sigpipe_to_default(), 2016-07-01) it's a bit arbitrary to only
check this for "git rm".

But in lieu of having wider test coverage for other "git" subcommands
let's refactor this to explicitly test for SIGPIPE outside of MinGW,
and then just that we remove the ".git/index.lock" (as before) on all
platforms.

1. https://lore.kernel.org/git/xmqq1rec5ckf.fsf@gitster.c.googlers.com/
2. 0693f9ddad (Make sure lockfiles are unlocked when dying on SIGPIPE,
   2008-12-18)

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t3600-rm.sh | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index efec8d13b6..185e39c390 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh

@@ -240,7 +240,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 	test_path_is_missing frotz/nitfol
 '
 
-test_expect_success 'choking "git rm" should not let it die with cruft' '
+choke_git_rm_setup() {
 	git reset -q --hard &&
 	test_when_finished "rm -f .git/index.lock && git reset -q --hard" &&
 	i=0 &&

@@ -249,12 +249,24 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 	do
 		echo "100644 $hash 0	some-file-$i"
 		i=$(( $i + 1 ))
-	done | git update-index --index-info &&
+	done | git update-index --index-info
+}
+
+test_expect_success 'choking "git rm" should not let it die with cruft (induce SIGPIPE)' '
+	choke_git_rm_setup &&
 	# git command is intentionally placed upstream of pipe to induce SIGPIPE
 	git rm -n "some-file-*" | : &&
 	test_path_is_missing .git/index.lock
 '
 
+
+test_expect_success !MINGW 'choking "git rm" should not let it die with cruft (induce and check SIGPIPE)' '
+	choke_git_rm_setup &&
+	OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) &&
+	test_match_signal 13 "$OUT" &&
+	test_path_is_missing .git/index.lock
+'
+
 test_expect_success 'Resolving by removal is not a warning-worthy event' '
 	git reset -q --hard &&
 	test_when_finished "rm -f .git/index.lock msg && git reset -q --hard" &&

-- 
2.29.2.222.g5d2a92d10f8

Change an invocation of zipinfo added in 19ee29401d (t5004: test ZIP
archives with many entries, 2015-08-22) to simply ask zipinfo for the
header info, rather than spewing out info about the entire archive and
race to kill it with SIGPIPE due to the downstream "head -2".

I ran across this because I'm adding a "set -o pipefail" test
mode. This won't be needed for the version of the mode that I'm
introducing (which currently relies on a patch to GNU bash), but I
think this is a good idea anyway.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t5004-archive-corner-cases.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
index 3e7b23cb32..2d32d0ed12 100755
--- a/t/t5004-archive-corner-cases.sh
+++ b/t/t5004-archive-corner-cases.sh

@@ -153,7 +153,8 @@ test_expect_success ZIPINFO 'zip archive with many entries' '
 
 	# check the number of entries in the ZIP file directory
 	expr 65536 + 256 >expect &&
-	"$ZIPINFO" many.zip | head -2 | sed -n "2s/.* //p" >actual &&
+	"$ZIPINFO" -h many.zip >zipinfo &&
+	sed -n "2s/.* //p" <zipinfo >actual &&
 	test_cmp expect actual
 '
 

-- 
2.29.2.222.g5d2a92d10f8