This started as an attempt to add a bash "set -o pipefail" test mode,
but now comes without that. Junio suggested dropping it in
<xmqq5z3o5n8c.fsf@xxxxxxxxxxxxxxxxxxxxxx>.
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
<87sg6s6lrs.fsf@xxxxxxxxxxxxxxxxxxx>. 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@xxxxxxxxxxxxxx/
+ 2. https://lore.kernel.org/git/20150325052952.GE31924@xxxxxxxx/
+ 3. https://lore.kernel.org/git/YARsCsgXuiXr4uFX@xxxxxxxxxxxxxxxxxxxxxxx/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
## 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 <avarab@xxxxxxxxx>
## 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 <peff@xxxxxxxx>
+ [ÆAB: wrote commit message]
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
- Commit-message-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
## 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@xxxxxxxxxxxxxxxxxxxxxx/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
@@ 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
