[08/12] t5520: use test_cmp_rev where possible
diff mbox series

Message ID ff05a0a8f7dc91d988e290b7d606e8a64f5daf54.1571354136.git.liu.denton@gmail.com
State New
Headers show
Series
  • t5520: various test cleanup
Related show

Commit Message

Denton Liu Oct. 17, 2019, 11:17 p.m. UTC
In case an invocation of `git rev-list` fails within the subshell, the
failure will be masked. Remove the subshell and use test_cmp_rev() so
that failures can be discovered.

This change was done with the following sed expressions:

	s/test "$(git rev-parse.* \([^)]*\))" = "$(git rev-parse \([^)]*\))"/test_cmp_rev \1 \2/
	s/test \([^ ]*\) = "$(git rev-parse.* \([^)]*\))"/test_cmp_rev \1 \2/
	s/test "$(git rev-parse.* \([^)]*\))" != "$(git rev-parse.* \([^)]*\))"/test_must_fail test_cmp_rev \1 \2/
	s/test \([^ ]*\) != "$(git rev-parse.* \([^)]*\))"/test_must_fail test_cmp_rev \1 \2/

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5520-pull.sh | 50 ++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

Comments

Eric Sunshine Oct. 17, 2019, 11:41 p.m. UTC | #1
On Thu, Oct 17, 2019 at 7:17 PM Denton Liu <liu.denton@gmail.com> wrote:
> In case an invocation of `git rev-list` fails within the subshell, the
> failure will be masked. Remove the subshell and use test_cmp_rev() so
> that failures can be discovered.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -597,10 +597,10 @@ test_expect_success 'pull --rebase dies early with dirty working directory' '
>         test_must_fail git pull &&
> -       test "$COPY" = "$(git rev-parse --verify me/copy)" &&
> +       test_cmp_rev "$COPY" me/copy &&

This transformation doesn't look correct. COPY already holds the
result of a git-rev-parse invocation:

    COPY="$(git rev-parse --verify me/copy)" &&

so passing it to test_cmp_rev() -- which applies its own git-rev-parse
invocation -- doesn't make sense.

>         git checkout HEAD -- file &&
>         git pull &&
> -       test "$COPY" != "$(git rev-parse --verify me/copy)"
> +       test_must_fail test_cmp_rev "$COPY" me/copy

As mentioned in my review of the other patch, this is not a valid use
of test_must_fail().
Denton Liu Oct. 18, 2019, 6:52 p.m. UTC | #2
Hi Eric,

Thanks for the reviews. I have no idea how you always get to my patches
so quickly but I appreciate the prompt reviews whenever I send a
test-related patchset in.

I've fixed up the other concerns you had and I'll send a v2 later but I
wanted to address this one.

On Thu, Oct 17, 2019 at 07:41:44PM -0400, Eric Sunshine wrote:
> On Thu, Oct 17, 2019 at 7:17 PM Denton Liu <liu.denton@gmail.com> wrote:
> > In case an invocation of `git rev-list` fails within the subshell, the
> > failure will be masked. Remove the subshell and use test_cmp_rev() so
> > that failures can be discovered.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> > @@ -597,10 +597,10 @@ test_expect_success 'pull --rebase dies early with dirty working directory' '
> >         test_must_fail git pull &&
> > -       test "$COPY" = "$(git rev-parse --verify me/copy)" &&
> > +       test_cmp_rev "$COPY" me/copy &&
> 
> This transformation doesn't look correct. COPY already holds the
> result of a git-rev-parse invocation:
> 
>     COPY="$(git rev-parse --verify me/copy)" &&
> 
> so passing it to test_cmp_rev() -- which applies its own git-rev-parse
> invocation -- doesn't make sense.

I'll annotate the entire test case with my comments:

	# so we'll be testing that pull --rebase dies early
	test_expect_success 'pull --rebase dies early with dirty working directory' '
		git checkout to-rebase &&
		git update-ref refs/remotes/me/copy copy^ &&
		COPY="$(git rev-parse --verify me/copy)" &&
		git rebase --onto $COPY copy &&

		# according to git log --graph, we have this currently:
		#
		# $ git log --pretty=oneline --graph to-rebase copy me/copy
		# * 9366795adb50ef6eb482b610b37cb1fb6edbd3d0 (HEAD -> to-rebase) to-rebase
		# * b3dcf50eea47c4ad85faabb0fb74eded71cc829f file3
		# * faf459539411b4557cf735232a3746be073177a9 new file
		# | * f340b1b8932f7b1e016c06867cbdc3f637eeea2d (copy) conflict
		# |/  
		# * 5e86d50a28977ebee5ea378f81591ea558149272 (me/second, me/copy, second) modified
		# * d25cf184567afad1dc1e018b2b7d793bc1bd2dc1 original

		test_config branch.to-rebase.remote me &&
		test_config branch.to-rebase.merge refs/heads/copy &&
		test_config branch.to-rebase.rebase true &&

		# we make the tree dirty here
		echo dirty >>file &&
		git add file &&

		# and over here, the pull should fail
		test_must_fail git pull &&

		# but since we're testing that it dies early, we want to
		# make sure that the remote ref doesn't change, which is
		# why it should still be equal
		test_cmp_rev "$COPY" me/copy &&

		git checkout HEAD -- file &&

		# but over here, the pull succeeds...
		git pull &&

		# so as a result, the remote ref should now be updated
		test_cmp_rev ! "$COPY" me/copy
	'

So after grokking the test case, it seems like the the transformation is
indeed correct. Maybe we can replace the last line with

	test_cmp_rev copy me/copy

but I think I'll leave it unless you have any strong opinions.

Thanks,

Denton
Eric Sunshine Oct. 23, 2019, 1:53 p.m. UTC | #3
On Fri, Oct 18, 2019 at 2:52 PM Denton Liu <liu.denton@gmail.com> wrote:
> On Thu, Oct 17, 2019 at 07:41:44PM -0400, Eric Sunshine wrote:
> > On Thu, Oct 17, 2019 at 7:17 PM Denton Liu <liu.denton@gmail.com> wrote:
> > > -       test "$COPY" = "$(git rev-parse --verify me/copy)" &&
> > > +       test_cmp_rev "$COPY" me/copy &&
> >
> > This transformation doesn't look correct. COPY already holds the
> > result of a git-rev-parse invocation:
> >
> >     COPY="$(git rev-parse --verify me/copy)" &&
> >
> > so passing it to test_cmp_rev() -- which applies its own git-rev-parse
> > invocation -- doesn't make sense.
>
> So after grokking the test case, it seems like the the transformation is
> indeed correct. Maybe we can replace the last line with
>
>         test_cmp_rev copy me/copy
>
> but I think I'll leave it unless you have any strong opinions.

For some reason, I had it in my mind that test_cmp_rev() was primarily
meant for comparing _named_ revisions, but of course there is nothing
about the function which even suggests that that is its intended
use-case. In retrospect, using it to compare an OID against a named
revision is a sensible use-case too, so I withdraw the objection.

Patch
diff mbox series

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c7797b13e6..f11fadc075 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -230,7 +230,7 @@  test_expect_success 'fast-forwards working tree if branch head is updated' '
 	git pull . second:third 2>err &&
 	test_i18ngrep "fetch updated the current branch head" err &&
 	test "$(cat file)" = modified &&
-	test "$(git rev-parse third)" = "$(git rev-parse second)"
+	test_cmp_rev third second
 '
 
 test_expect_success 'fast-forward fails with conflicting work tree' '
@@ -241,7 +241,7 @@  test_expect_success 'fast-forward fails with conflicting work tree' '
 	test_must_fail git pull . second:third 2>err &&
 	test_i18ngrep "Cannot fast-forward your working tree" err &&
 	test "$(cat file)" = conflict &&
-	test "$(git rev-parse third)" = "$(git rev-parse second)"
+	test_cmp_rev third second
 '
 
 test_expect_success '--rebase' '
@@ -254,7 +254,7 @@  test_expect_success '--rebase' '
 	git commit -m "new file" &&
 	git tag before-rebase &&
 	git pull --rebase . copy &&
-	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^ copy &&
 	test new = "$(git show HEAD:file2)"
 '
 
@@ -266,7 +266,7 @@  test_expect_success '--rebase fast forward' '
 
 	git checkout to-rebase &&
 	git pull --rebase . ff &&
-	test "$(git rev-parse HEAD)" = "$(git rev-parse ff)" &&
+	test_cmp_rev HEAD ff &&
 
 	# The above only validates the result.  Did we actually bypass rebase?
 	git reflog -1 >reflog.actual &&
@@ -290,7 +290,7 @@  test_expect_success '--rebase --autostash fast forward' '
 	git checkout behind &&
 	echo dirty >file &&
 	git pull --rebase --autostash . to-rebase-ff &&
-	test "$(git rev-parse HEAD)" = "$(git rev-parse to-rebase-ff)"
+	test_cmp_rev HEAD to-rebase-ff
 '
 
 test_expect_success '--rebase with conflicts shows advice' '
@@ -328,7 +328,7 @@  test_expect_success 'failed --rebase shows advice' '
 test_expect_success '--rebase fails with multiple branches' '
 	git reset --hard before-rebase &&
 	test_must_fail git pull --rebase . copy master 2>err &&
-	test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
+	test_cmp_rev HEAD before-rebase &&
 	test_i18ngrep "Cannot rebase onto multiple branches" err &&
 	test modified = "$(git show HEAD:file)"
 '
@@ -380,7 +380,7 @@  test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&
 	git pull . copy &&
-	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^ copy &&
 	test new = "$(git show HEAD:file2)"
 '
 
@@ -398,7 +398,7 @@  test_expect_success 'branch.to-rebase.rebase' '
 	git reset --hard before-rebase &&
 	test_config branch.to-rebase.rebase true &&
 	git pull . copy &&
-	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^ copy &&
 	test new = "$(git show HEAD:file2)"
 '
 
@@ -407,14 +407,14 @@  test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test_config pull.rebase true &&
 	test_config branch.to-rebase.rebase false &&
 	git pull . copy &&
-	test "$(git rev-parse HEAD^)" != "$(git rev-parse copy)" &&
+	test_must_fail test_cmp_rev HEAD^ copy &&
 	test new = "$(git show HEAD:file2)"
 '
 
 test_expect_success 'pull --rebase warns on --verify-signatures' '
 	git reset --hard before-rebase &&
 	git pull --rebase --verify-signatures . copy 2>err &&
-	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^ copy &&
 	test new = "$(git show HEAD:file2)" &&
 	test_i18ngrep "ignoring --verify-signatures for rebase" err
 '
@@ -422,7 +422,7 @@  test_expect_success 'pull --rebase warns on --verify-signatures' '
 test_expect_success 'pull --rebase does not warn on --no-verify-signatures' '
 	git reset --hard before-rebase &&
 	git pull --rebase --no-verify-signatures . copy 2>err &&
-	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^ copy &&
 	test new = "$(git show HEAD:file2)" &&
 	test_i18ngrep ! "verify-signatures" err
 '
@@ -443,8 +443,8 @@  test_expect_success 'pull.rebase=false create a new merge commit' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase false &&
 	git pull . copy &&
-	test "$(git rev-parse HEAD^1)" = "$(git rev-parse before-preserve-rebase)" &&
-	test "$(git rev-parse HEAD^2)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^1 before-preserve-rebase &&
+	test_cmp_rev HEAD^2 copy &&
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
@@ -452,7 +452,7 @@  test_expect_success 'pull.rebase=true flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull . copy &&
-	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^^ copy &&
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
@@ -460,7 +460,7 @@  test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase 1 &&
 	git pull . copy &&
-	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^^ copy &&
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
@@ -469,8 +469,8 @@  test_expect_success REBASE_P \
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull . copy &&
-	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
-	test "$(git rev-parse HEAD^2)" = "$(git rev-parse keep-merge)"
+	test_cmp_rev HEAD^^ copy &&
+	test_cmp_rev HEAD^2 keep-merge
 '
 
 test_expect_success 'pull.rebase=interactive' '
@@ -505,8 +505,8 @@  test_expect_success '--rebase=false create a new merge commit' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull --rebase=false . copy &&
-	test "$(git rev-parse HEAD^1)" = "$(git rev-parse before-preserve-rebase)" &&
-	test "$(git rev-parse HEAD^2)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^1 before-preserve-rebase &&
+	test_cmp_rev HEAD^2 copy &&
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
@@ -514,7 +514,7 @@  test_expect_success '--rebase=true rebases and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull --rebase=true . copy &&
-	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^^ copy &&
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
@@ -523,8 +523,8 @@  test_expect_success REBASE_P \
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull --rebase=preserve . copy &&
-	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
-	test "$(git rev-parse HEAD^2)" = "$(git rev-parse keep-merge)"
+	test_cmp_rev HEAD^^ copy &&
+	test_cmp_rev HEAD^2 keep-merge
 '
 
 test_expect_success '--rebase=invalid fails' '
@@ -536,7 +536,7 @@  test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-m
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull --rebase . copy &&
-	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	test_cmp_rev HEAD^^ copy &&
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
@@ -597,10 +597,10 @@  test_expect_success 'pull --rebase dies early with dirty working directory' '
 	echo dirty >>file &&
 	git add file &&
 	test_must_fail git pull &&
-	test "$COPY" = "$(git rev-parse --verify me/copy)" &&
+	test_cmp_rev "$COPY" me/copy &&
 	git checkout HEAD -- file &&
 	git pull &&
-	test "$COPY" != "$(git rev-parse --verify me/copy)"
+	test_must_fail test_cmp_rev "$COPY" me/copy
 '
 
 test_expect_success 'pull --rebase works on branch yet to be born' '