diff mbox series

[v2,1/2] t5574: test porcelain output of atomic fetch

Message ID 210191917bcfa9293622908c291652059576f3e5.1702556642.git.zhiyou.jx@alibaba-inc.com (mailing list archive)
State New, archived
Headers show
Series jx/fetch-atomic-error-message-fix | expand

Commit Message

Jiang Xin Dec. 14, 2023, 12:33 p.m. UTC 
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The test case "fetch porcelain output" checks output of the fetch
command. The error output must be empty with the follow assertion:

    test_must_be_empty stderr

But this assertion fails if using atomic fetch. Refactor this test case
to use different fetch options by splitting it into three test cases.

  1. "setup for fetch porcelain output".

  2. "fetch porcelain output": for non-atomic fetch.

  3. "fetch porcelain output (atomic)": for atomic fetch.

Add new command "test_commit ..." in the first test case, so that if we
run these test cases individually (--run=4-6), "git rev-parse HEAD~"
command will work properly. Run the above test cases, we can find that
one test case has a known breakage, as shown below:

    ok 4 - setup for fetch porcelain output
    ok 5 - fetch porcelain output  # TODO known breakage vanished
    not ok 6 - fetch porcelain output (atomic) # TODO known breakage

The failed test case has an error message with only the error prompt but
no message body, as follows:

    'stderr' is not empty, it contains:
    error:

In a later commit, we will fix this issue.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5574-fetch-output.sh | 97 ++++++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 39 deletions(-)

Comments

Patrick Steinhardt Dec. 15, 2023, 9:56 a.m. UTC | #1 
On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
[snip]
> @@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '
>  	git checkout force-updated &&
>  	git reset --hard HEAD~ &&
>  	test_commit --no-tag force-update-new &&
> -	FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
> -
> -	cat >expect <<-EOF &&
> -	- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
> -	- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
> -	  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
> -	! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
> -	* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
> -	  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
> -	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
> -	* $ZERO_OID $MAIN_OLD refs/forced/new-branch
> -	  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
> -	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
> -	* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
> -	EOF
> -
> -	# Execute a dry-run fetch first. We do this to assert that the dry-run
> -	# and non-dry-run fetches produces the same output. Execution of the
> -	# fetch is expected to fail as we have a rejected reference update.
> -	test_must_fail git -C porcelain fetch \
> -		--porcelain --dry-run --prune origin $refspecs >actual &&
> -	test_cmp expect actual &&
> -
> -	# And now we perform a non-dry-run fetch.
> -	test_must_fail git -C porcelain fetch \
> -		--porcelain --prune origin $refspecs >actual 2>stderr &&
> -	test_cmp expect actual &&
> -	test_must_be_empty stderr
> +	FORCE_UPDATED_NEW=$(git rev-parse HEAD)
>  '
>  
> +for opt in off on
> +do
> +	case $opt in
> +	on)
> +		opt=--atomic
> +		;;
> +	off)
> +		opt=
> +		;;
> +	esac

Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
this case statement. Not sure whether this is worth a reroll though,
probably not.

Patrick
Jiang Xin Dec. 15, 2023, 11:16 a.m. UTC | #2 
On Fri, Dec 15, 2023 at 5:56 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> [snip]
> > @@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '
> >       git checkout force-updated &&
> >       git reset --hard HEAD~ &&
> >       test_commit --no-tag force-update-new &&
> > -     FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
> > -
> > -     cat >expect <<-EOF &&
> > -     - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
> > -     - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
> > -     ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
> > -     * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
> > -     + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
> > -     * $ZERO_OID $MAIN_OLD refs/forced/new-branch
> > -       $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
> > -     + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
> > -     * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
> > -     EOF
> > -
> > -     # Execute a dry-run fetch first. We do this to assert that the dry-run
> > -     # and non-dry-run fetches produces the same output. Execution of the
> > -     # fetch is expected to fail as we have a rejected reference update.
> > -     test_must_fail git -C porcelain fetch \
> > -             --porcelain --dry-run --prune origin $refspecs >actual &&
> > -     test_cmp expect actual &&
> > -
> > -     # And now we perform a non-dry-run fetch.
> > -     test_must_fail git -C porcelain fetch \
> > -             --porcelain --prune origin $refspecs >actual 2>stderr &&
> > -     test_cmp expect actual &&
> > -     test_must_be_empty stderr
> > +     FORCE_UPDATED_NEW=$(git rev-parse HEAD)
> >  '
> >
> > +for opt in off on
> > +do
> > +     case $opt in
> > +     on)
> > +             opt=--atomic
> > +             ;;
> > +     off)
> > +             opt=
> > +             ;;
> > +     esac
>
> Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
> this case statement. Not sure whether this is worth a reroll though,
> probably not.

Yes, your code is much simpler.
Junio C Hamano Dec. 15, 2023, 4:47 p.m. UTC | #3 
Jiang Xin <worldhello.net@gmail.com> writes:

>> Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
>> this case statement. Not sure whether this is worth a reroll though,
>> probably not.
>
> Yes, your code is much simpler.

Yup, thanks for careful and helpful reviews, Patrick, on both
patches.  And of course, thanks, Jiang, for working on them.
diff mbox series

Patch

diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 90e6dcb9a7..bc747efefc 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -61,11 +61,10 @@  test_expect_success 'fetch compact output' '
 	test_cmp expect actual
 '
 
-test_expect_success 'fetch porcelain output' '
-	test_when_finished "rm -rf porcelain" &&
-
+test_expect_success 'setup for fetch porcelain output' '
 	# Set up a bunch of references that we can use to demonstrate different
 	# kinds of flag symbols in the output format.
+	test_commit commit-for-porcelain-output &&
 	MAIN_OLD=$(git rev-parse HEAD) &&
 	git branch "fast-forward" &&
 	git branch "deleted-branch" &&
@@ -74,15 +73,10 @@  test_expect_success 'fetch porcelain output' '
 	FORCE_UPDATED_OLD=$(git rev-parse HEAD) &&
 	git checkout main &&
 
-	# Clone and pre-seed the repositories. We fetch references into two
-	# namespaces so that we can test that rejected and force-updated
-	# references are reported properly.
-	refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
-	git clone . porcelain &&
-	git -C porcelain fetch origin $refspecs &&
+	# Backup to preseed.git
+	git clone --mirror . preseed.git &&
 
-	# Now that we have set up the client repositories we can change our
-	# local references.
+	# Continue changing our local references.
 	git branch new-branch &&
 	git branch -d deleted-branch &&
 	git checkout fast-forward &&
@@ -91,36 +85,61 @@  test_expect_success 'fetch porcelain output' '
 	git checkout force-updated &&
 	git reset --hard HEAD~ &&
 	test_commit --no-tag force-update-new &&
-	FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
-
-	cat >expect <<-EOF &&
-	- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
-	- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
-	! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
-	* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
-	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
-	* $ZERO_OID $MAIN_OLD refs/forced/new-branch
-	  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
-	+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
-	* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
-	EOF
-
-	# Execute a dry-run fetch first. We do this to assert that the dry-run
-	# and non-dry-run fetches produces the same output. Execution of the
-	# fetch is expected to fail as we have a rejected reference update.
-	test_must_fail git -C porcelain fetch \
-		--porcelain --dry-run --prune origin $refspecs >actual &&
-	test_cmp expect actual &&
-
-	# And now we perform a non-dry-run fetch.
-	test_must_fail git -C porcelain fetch \
-		--porcelain --prune origin $refspecs >actual 2>stderr &&
-	test_cmp expect actual &&
-	test_must_be_empty stderr
+	FORCE_UPDATED_NEW=$(git rev-parse HEAD)
 '
 
+for opt in off on
+do
+	case $opt in
+	on)
+		opt=--atomic
+		;;
+	off)
+		opt=
+		;;
+	esac
+	test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+		test_when_finished "rm -rf porcelain" &&
+
+		# Clone and pre-seed the repositories. We fetch references into two
+		# namespaces so that we can test that rejected and force-updated
+		# references are reported properly.
+		refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
+		git clone preseed.git porcelain &&
+		git -C porcelain fetch origin $opt $refspecs &&
+
+		cat >expect <<-EOF &&
+		- $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
+		- $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
+		! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
+		* $ZERO_OID $MAIN_OLD refs/unforced/new-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
+		+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
+		* $ZERO_OID $MAIN_OLD refs/forced/new-branch
+		  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
+		+ $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
+		* $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
+		EOF
+
+		# Change the URL of the repository to fetch different references.
+		git -C porcelain remote set-url origin .. &&
+
+		# Execute a dry-run fetch first. We do this to assert that the dry-run
+		# and non-dry-run fetches produces the same output. Execution of the
+		# fetch is expected to fail as we have a rejected reference update.
+		test_must_fail git -C porcelain fetch $opt \
+			--porcelain --dry-run --prune origin $refspecs >actual &&
+		test_cmp expect actual &&
+
+		# And now we perform a non-dry-run fetch.
+		test_must_fail git -C porcelain fetch $opt \
+			--porcelain --prune origin $refspecs >actual 2>stderr &&
+		test_cmp expect actual &&
+		test_must_be_empty stderr
+	'
+done
+
 test_expect_success 'fetch porcelain with multiple remotes' '
 	test_when_finished "rm -rf porcelain" &&