[v3,06/30] subtree: t7900: use 'test' for string equality

Commit Message

Luke Shumaker April 27, 2021, 9:17 p.m. UTC

From: Luke Shumaker <[e-mail]>

t7900-subtree.sh defines its own `check_equal A B` function, instead of
just using `test A = B` like all of the other tests.  Don't be special,
get rid of `check_equal` in favor of `test`.

Signed-off-by: Luke Shumaker <[e-mail]>
---
 contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++------------------
 1 file changed, 24 insertions(+), 36 deletions(-)

Comments

Ævar Arnfjörð Bjarmason April 30, 2021, 9:55 a.m. UTC | #1

On Tue, Apr 27 2021, Luke Shumaker wrote:

> From: Luke Shumaker <[e-mail]>
>
> t7900-subtree.sh defines its own `check_equal A B` function, instead of
> just using `test A = B` like all of the other tests.  Don't be special,
> get rid of `check_equal` in favor of `test`.
>
> Signed-off-by: Luke Shumaker <[e-mail]>
> ---
>  contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++------------------
>  1 file changed, 24 insertions(+), 36 deletions(-)
>
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 12b8cb03c7..76183153c9 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -28,18 +28,6 @@ create () {
>  	git add "$1"
>  }
>  
> -check_equal () {
> -	test_debug 'echo'
> -	test_debug "echo \"check a:\" \"{$1}\""
> -	test_debug "echo \"      b:\" \"{$2}\""
> -	if test "$1" = "$2"
> -	then
> -		return 0
> -	else
> -		return 1
> -	fi
> -}

It looks like the reason this was used because when this fails just
having the "test" makes for bad debugging. I.e. if the values don't
match the $1 and $2 are not aligned, so it's hard to eyeball what went
wrong.

These days this is more idiomatic:

    echo "Add [...]" >expected
    last_commit_message >actual &&
    test_cmp expected actual

So I think in this case a better narrower improvement would be to keep
the check_equal function. I wonder if we shouldn't just in general in
t/test-lib.sh have a test_cmp_str for this use-case. I.e. a trivial
wrapper that echos the two strings to a file for you, before running
diff(1).

Luke Shumaker April 30, 2021, 4:33 p.m. UTC | #2

On Fri, 30 Apr 2021 03:55:04 -0600,
Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Tue, Apr 27 2021, Luke Shumaker wrote:
> 
> > From: Luke Shumaker <[e-mail]>
> >
> > t7900-subtree.sh defines its own `check_equal A B` function, instead of
> > just using `test A = B` like all of the other tests.  Don't be special,
> > get rid of `check_equal` in favor of `test`.
> >
> > Signed-off-by: Luke Shumaker <[e-mail]>
> > ---
> >  contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++------------------
> >  1 file changed, 24 insertions(+), 36 deletions(-)
> >
> > diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> > index 12b8cb03c7..76183153c9 100755
> > --- a/contrib/subtree/t/t7900-subtree.sh
> > +++ b/contrib/subtree/t/t7900-subtree.sh
> > @@ -28,18 +28,6 @@ create () {
> >  	git add "$1"
> >  }
> >  
> > -check_equal () {
> > -	test_debug 'echo'
> > -	test_debug "echo \"check a:\" \"{$1}\""
> > -	test_debug "echo \"      b:\" \"{$2}\""
> > -	if test "$1" = "$2"
> > -	then
> > -		return 0
> > -	else
> > -		return 1
> > -	fi
> > -}
> 
> It looks like the reason this was used because when this fails just
> having the "test" makes for bad debugging. I.e. if the values don't
> match the $1 and $2 are not aligned, so it's hard to eyeball what went
> wrong.

It's easy to make that assumption, but looking at the history it seems
the "actual" reason it exists is that it's vestigial from before the
subtree tests used test-lib.sh, and echoing it like that was the only
way you'd get feedback.

> These days this is more idiomatic:
> 
>     echo "Add [...]" >expected
>     last_commit_message >actual &&
>     test_cmp expected actual
> 
> So I think in this case a better narrower improvement would be to keep
> the check_equal function. I wonder if we shouldn't just in general in
> t/test-lib.sh have a test_cmp_str for this use-case. I.e. a trivial
> wrapper that echos the two strings to a file for you, before running
> diff(1).

But it's been my experience that the tests are already impossible to
debug without passing `-x`, so everything from `check_equal` ends up
being just noise.

And also I figured if `test` is good enough for t9350-fast-export.sh,
then it's good enough for t7900-subtree.sh... after working with
subtree, I'd accidentally written a couple of the checks in one of my
fast-export patchsets using `check_equal`.  Being special makes things
harder to hack on.

Patch

diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 12b8cb03c7..76183153c9 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -28,18 +28,6 @@  create () {
 	git add "$1"
 }
 
-check_equal () {
-	test_debug 'echo'
-	test_debug "echo \"check a:\" \"{$1}\""
-	test_debug "echo \"      b:\" \"{$2}\""
-	if test "$1" = "$2"
-	then
-		return 0
-	else
-		return 1
-	fi
-}
-
 undo () {
 	git reset --hard HEAD~
 }
@@ -123,7 +111,7 @@  test_expect_success 'add subproj as subtree into sub dir/ with --prefix' '
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree add --prefix="sub dir" FETCH_HEAD &&
-		check_equal "$(last_commit_message)" "Add '\''sub dir/'\'' from commit '\''$(git rev-parse FETCH_HEAD)'\''"
+		test "$(last_commit_message)" = "Add '\''sub dir/'\'' from commit '\''$(git rev-parse FETCH_HEAD)'\''"
 	)
 '
 
@@ -136,7 +124,7 @@  test_expect_success 'add subproj as subtree into sub dir/ with --prefix and --me
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree add --prefix="sub dir" --message="Added subproject" FETCH_HEAD &&
-		check_equal "$(last_commit_message)" "Added subproject"
+		test "$(last_commit_message)" = "Added subproject"
 	)
 '
 
@@ -149,7 +137,7 @@  test_expect_success 'add subproj as subtree into sub dir/ with --prefix as -P an
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree add -P "sub dir" -m "Added subproject" FETCH_HEAD &&
-		check_equal "$(last_commit_message)" "Added subproject"
+		test "$(last_commit_message)" = "Added subproject"
 	)
 '
 
@@ -162,7 +150,7 @@  test_expect_success 'add subproj as subtree into sub dir/ with --squash and --pr
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree add --prefix="sub dir" --message="Added subproject with squash" --squash FETCH_HEAD &&
-		check_equal "$(last_commit_message)" "Added subproject with squash"
+		test "$(last_commit_message)" = "Added subproject with squash"
 	)
 '
 
@@ -185,7 +173,7 @@  test_expect_success 'merge new subproj history into sub dir/ with --prefix' '
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree merge --prefix="sub dir" FETCH_HEAD &&
-		check_equal "$(last_commit_message)" "Merge commit '\''$(git rev-parse FETCH_HEAD)'\''"
+		test "$(last_commit_message)" = "Merge commit '\''$(git rev-parse FETCH_HEAD)'\''"
 	)
 '
 
@@ -204,7 +192,7 @@  test_expect_success 'merge new subproj history into sub dir/ with --prefix and -
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree merge --prefix="sub dir" --message="Merged changes from subproject" FETCH_HEAD &&
-		check_equal "$(last_commit_message)" "Merged changes from subproject"
+		test "$(last_commit_message)" = "Merged changes from subproject"
 	)
 '
 
@@ -223,7 +211,7 @@  test_expect_success 'merge new subproj history into sub dir/ with --squash and -
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree merge --prefix="sub dir" --message="Merged changes from subproject using squash" --squash FETCH_HEAD &&
-		check_equal "$(last_commit_message)" "Merged changes from subproject using squash"
+		test "$(last_commit_message)" = "Merged changes from subproject using squash"
 	)
 '
 
@@ -239,7 +227,7 @@  test_expect_success 'merge the added subproj again, should do nothing' '
 		# this shouldn not actually do anything, since FETCH_HEAD
 		# is already a parent
 		result=$(git merge -s ours -m "merge -s -ours" FETCH_HEAD) &&
-		check_equal "${result}" "Already up to date."
+		test "${result}" = "Already up to date."
 	)
 '
 
@@ -258,7 +246,7 @@  test_expect_success 'merge new subproj history into subdir/ with a slash appende
 		cd "$test_count" &&
 		git fetch ./subproj HEAD &&
 		git subtree merge --prefix=subdir/ FETCH_HEAD &&
-		check_equal "$(last_commit_message)" "Merge commit '\''$(git rev-parse FETCH_HEAD)'\''"
+		test "$(last_commit_message)" = "Merge commit '\''$(git rev-parse FETCH_HEAD)'\''"
 	)
 '
 
@@ -324,7 +312,7 @@  test_expect_success 'split sub dir/ with --rejoin' '
 		git subtree merge --prefix="sub dir" FETCH_HEAD &&
 		split_hash=$(git subtree split --prefix="sub dir" --annotate="*") &&
 		git subtree split --prefix="sub dir" --annotate="*" --rejoin &&
-		check_equal "$(last_commit_message)" "Split '\''sub dir/'\'' into commit '\''$split_hash'\''"
+		test "$(last_commit_message)" = "Split '\''sub dir/'\'' into commit '\''$split_hash'\''"
 	)
 '
 
@@ -339,7 +327,7 @@  test_expect_success 'split sub dir/ with --rejoin from scratch' '
 		git commit -m"sub dir file" &&
 		split_hash=$(git subtree split --prefix="sub dir" --rejoin) &&
 		git subtree split --prefix="sub dir" --rejoin &&
-		check_equal "$(last_commit_message)" "Split '\''sub dir/'\'' into commit '\''$split_hash'\''"
+		test "$(last_commit_message)" = "Split '\''sub dir/'\'' into commit '\''$split_hash'\''"
 	)
 '
 
@@ -362,7 +350,7 @@  test_expect_success 'split sub dir/ with --rejoin and --message' '
 		git fetch ./"sub proj" HEAD &&
 		git subtree merge --prefix="sub dir" FETCH_HEAD &&
 		git subtree split --prefix="sub dir" --message="Split & rejoin" --annotate="*" --rejoin &&
-		check_equal "$(last_commit_message)" "Split & rejoin"
+		test "$(last_commit_message)" = "Split & rejoin"
 	)
 '
 
@@ -386,7 +374,7 @@  test_expect_success 'split "sub dir"/ with --branch' '
 		git subtree merge --prefix="sub dir" FETCH_HEAD &&
 		split_hash=$(git subtree split --prefix="sub dir" --annotate="*") &&
 		git subtree split --prefix="sub dir" --annotate="*" --branch subproj-br &&
-		check_equal "$(git rev-parse subproj-br)" "$split_hash"
+		test "$(git rev-parse subproj-br)" = "$split_hash"
 	)
 '
 
@@ -410,13 +398,13 @@  test_expect_success 'check hash of split' '
 		git subtree merge --prefix="sub dir" FETCH_HEAD &&
 		split_hash=$(git subtree split --prefix="sub dir" --annotate="*") &&
 		git subtree split --prefix="sub dir" --annotate="*" --branch subproj-br &&
-		check_equal "$(git rev-parse subproj-br)" "$split_hash" &&
+		test "$(git rev-parse subproj-br)" = "$split_hash" &&
 		# Check hash of split
 		new_hash=$(git rev-parse subproj-br^2) &&
 		(
 			cd ./"sub proj" &&
 			subdir_hash=$(git rev-parse HEAD) &&
-			check_equal ''"$new_hash"'' "$subdir_hash"
+			test ''"$new_hash"'' = "$subdir_hash"
 		)
 	)
 '
@@ -442,7 +430,7 @@  test_expect_success 'split "sub dir"/ with --branch for an existing branch' '
 		git subtree merge --prefix="sub dir" FETCH_HEAD &&
 		split_hash=$(git subtree split --prefix="sub dir" --annotate="*") &&
 		git subtree split --prefix="sub dir" --annotate="*" --branch subproj-br &&
-		check_equal "$(git rev-parse subproj-br)" "$split_hash"
+		test "$(git rev-parse subproj-br)" = "$split_hash"
 	)
 '
 
@@ -740,7 +728,7 @@  test_expect_success 'make sure the --rejoin commits never make it into subproj'
 	(
 		cd "$test_count" &&
 		git subtree pull --prefix="sub dir" ./"sub proj" HEAD &&
-		check_equal "$(git log --pretty=format:"%s" HEAD^2 | grep -i split)" ""
+		test "$(git log --pretty=format:"%s" HEAD^2 | grep -i split)" = ""
 	)
 '
 
@@ -791,7 +779,7 @@  test_expect_success 'make sure no "git subtree" tagged commits make it into subp
 		git subtree pull --prefix="sub dir" ./"sub proj" HEAD &&
 
 		# They are meaningless to subproj since one side of the merge refers to the mainline
-		check_equal "$(git log --pretty=format:"%s%n%b" HEAD^2 | grep "git-subtree.*:")" ""
+		test "$(git log --pretty=format:"%s%n%b" HEAD^2 | grep "git-subtree.*:")" = ""
 	)
 '
 
@@ -825,7 +813,7 @@  test_expect_success 'make sure "git subtree split" find the correct parent' '
 		# not, something went wrong (the "newparent" of "HEAD~" commit should
 		# have been sub2, but it was not, because its cache was not set to
 		# itself)
-		check_equal "$(git log --pretty=format:%P -1 subproj-br)" "$(git rev-parse subproj-ref)"
+		test "$(git log --pretty=format:%P -1 subproj-br)" = "$(git rev-parse subproj-ref)"
 	)
 '
 
@@ -859,7 +847,7 @@  test_expect_success 'split a new subtree without --onto option' '
 		# if the parent of the first commit in the tree is not empty,
 		# then the new subtree has accidentally been attached to something
 		git subtree split --prefix="sub dir2" --branch subproj2-br &&
-		check_equal "$(git log --pretty=format:%P -1 subproj2-br)" ""
+		test "$(git log --pretty=format:%P -1 subproj2-br)" = ""
 	)
 '
 
@@ -899,10 +887,10 @@  test_expect_success 'verify one file change per commit' '
 				test_debug "echo Verifying commit $commit"
 				test_debug "echo a: $a"
 				test_debug "echo b: $b"
-				check_equal "$b" ""
+				test "$b" = ""
 				x=1
 			done
-			check_equal "$x" 1
+			test "$x" = 1
 		)
 	)
 '
@@ -934,7 +922,7 @@  test_expect_success 'push split to subproj' '
 		git subtree push ./"sub proj" --prefix "sub dir" sub-branch-1 &&
 		cd ./"sub proj" &&
 		git checkout sub-branch-1 &&
-		check_equal "$(last_commit_message)" "sub dir/main-sub3"
+		test "$(last_commit_message)" = "sub dir/main-sub3"
 	)
 '
 
@@ -994,7 +982,7 @@  test_expect_success 'subtree descendant check' '
 
 		git subtree split --prefix folder_subtree/ --branch subtree_tip $defaultBranch &&
 		git subtree split --prefix folder_subtree/ --branch subtree_branch branch &&
-		check_equal $(git rev-list --count subtree_tip..subtree_branch) 0
+		test $(git rev-list --count subtree_tip..subtree_branch) = 0
 	)
 '