[PATCH 0/2] transport: don't ignore git-receive-pack(1) exit code on atomic push

[PATCH 0/2] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Patrick Steinhardt]

Hi,

we've hit an edge case at GitLab where an atomic push will not notice an
error when git-receive-pack(1) updates the refs, but otherwise fails
with a non-zero exit code. The push would be successful and no error
would be printed even though some things have gone wrong on the remote
side.

This patch serise fixes the issue. I'm not a 100% sure whether this fix
is correct or not because it has interactions with the "--porcelain"
mode, which is quite underspecified overall. So I'd appreciate some
extra scrutiny on this series.

Thanks!

Patrick

To: git@vger.kernel.org
Cc: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Patrick Steinhardt (2):
      t5504: modernize test by moving heredocs into test bodies
      transport: don't ignore git-receive-pack(1) exit code on atomic push

 send-pack.c                     |  2 +-
 t/t5504-fetch-receive-strict.sh | 36 +++++++++++++++++-------------------
 t/t5543-atomic-push.sh          | 30 ++++++++++++++++++++++++++++++
 transport.c                     |  9 ++-------
 4 files changed, 50 insertions(+), 27 deletions(-)

---
base-commit: 25b0f41288718625b18495de23cc066394c09a92
change-id: 20241113-pks-push-atomic-respect-exit-code-436c443a657d

[PATCH 1/2] t5504: modernize test by moving heredocs into test bodies

[Patrick Steinhardt]

We have several heredocs in t5504 located outside of any particular test
bodies. Move these into the test bodies to match our modern coding
style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5504-fetch-receive-strict.sh | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 138e6778a477650ecbe2dc3e480c5fe83d4bb485..0a3883043baf5c4c0fc43b52e8c5fc375f10a56a 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -65,12 +65,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 	)
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
-Done
-EOF
-
 test_expect_success 'push without strict' '
 	rm -rf dst &&
 	git init dst &&
@@ -79,6 +73,11 @@ test_expect_success 'push without strict' '
 		git config fetch.fsckobjects false &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
+	Done
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -95,11 +94,6 @@ test_expect_success 'push with !receive.fsckobjects' '
 	test_cmp exp act
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
-EOF
-
 test_expect_success 'push with receive.fsckobjects' '
 	rm -rf dst &&
 	git init dst &&
@@ -108,6 +102,10 @@ test_expect_success 'push with receive.fsckobjects' '
 		git config receive.fsckobjects true &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -130,15 +128,14 @@ test_expect_success 'repair the "corrupt or missing" object' '
 	git fsck
 '
 
-cat >bogus-commit <<EOF
-tree $EMPTY_TREE
-author Bugs Bunny 1234567890 +0000
-committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
-
-This commit object intentionally broken
-EOF
-
 test_expect_success 'setup bogus commit' '
+	cat >bogus-commit <<-EOF &&
+	tree $EMPTY_TREE
+	author Bugs Bunny 1234567890 +0000
+	committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
+
+	This commit object intentionally broken
+	EOF
 	commit="$(git hash-object --literally -t commit -w --stdin <bogus-commit)"
 '
 

-- 
2.47.0.251.gb31fb630c0.dirty

[PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Patrick Steinhardt]

When executing git-push(1) with the "--porcelain" flag, then we will
print updated references in a machine-readable format that looks like
this:

    To destination
    =	refs/heads/noop:refs/heads/noop	[up to date]
    !	refs/heads/rejected:refs/heads/rejected	[rejected] (atomic push failed)
    !	refs/heads/noff:refs/heads/(off (non-fast-forward)
    Done

The final "Done" stanza was introduced via 77555854be (git-push: make
git push --porcelain print "Done", 2010-02-26), where it was printed
"unless any errors have occurred". This behaviour was later changed via
7dcbeaa0df (send-pack: fix inconsistent porcelain output, 2020-04-17)
such that the stanza will also be printed when there was an error with
atomic pushes, which was previously inconsistent with non-atomic pushes.

The fixup commit has introduced a new issue though. During atomic pushes
it is expected that git-receive-pack(1) may exit early, and that causes
us to receive an error on the client-side. We (seemingly) still want to
print the "Done" stanza, but given that we only do so when the push has
been successful we started to ignore the error code by the child process
completely when doing an atomic push.

We'd typically notice this case because the refs would have their error
message set. But there is an edge case when pushing refs succeeds, but
git-receive-pack(1) exits with a non-zero exit code at a later point in
time due to another error. An atomic git-push(1) would ignore that error
code, and consequently it would return successfully and not print any
error message at all.

Now it is somewhat unclear what the correct fix is in this case, mostly
because the exact format of the porcelain output of git-push(1) is not
specified to its full extent. What is clear though is that ignoring the
error code is definitely not the correct thing to do.

Adapt the code such that we honor the error code and unconditionally
print the "Done" stanza even when pushing refs has failed. This ensures
that git-push(1) notices the error condition and exits with an error,
but slightly changes the output format.

This requires a change to t5504, where we previously didn't print "Done"
at the end of the push. As said, it is hard to say what the correct
behaviour is in this case. But two test cases further up we have another
test that fails in a similar way, and that test expects a final "Done"
stanza. So if nothing else this at least seems to make the behaviour
more consistent with other error cases.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c                     |  2 +-
 t/t5504-fetch-receive-strict.sh |  1 +
 t/t5543-atomic-push.sh          | 30 ++++++++++++++++++++++++++++++
 transport.c                     |  9 ++-------
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 6677c44e8acd19f16706ad2d78f72fee889daa55..815c1f206c09da8fb1ffe9be11beb9c5fa29d0c5 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,
 				reject_atomic_push(remote_refs, args->send_mirror);
 				error("atomic push failed for ref %s. status: %d",
 				      ref->name, ref->status);
-				ret = args->porcelain ? 0 : -1;
+				ret = -1;
 				goto out;
 			}
 			/* else fallthrough */
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 0a3883043baf5c4c0fc43b52e8c5fc375f10a56a..b3774e56e0f45a91d574a15f0d6b5c50c4da70d4 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -105,6 +105,7 @@ test_expect_success 'push with receive.fsckobjects' '
 	cat >exp <<-EOF &&
 	To dst
 	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
+	Done
 	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 479d103469527e6b923877cd480825b59e7094d4..1d8f088a004cf6743ade8e55536f04ac04000cf7 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -281,4 +281,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'atomic push reports exit code failure' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict 2>err &&
+	cat >expect <<-EOF &&
+	To ../upstream
+	 * [new branch]      HEAD -> no-conflict
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
+test_expect_success 'atomic push reports exit code failure with porcelain' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic --porcelain \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict-porcelain 2>err &&
+	cat >expect <<-EOF &&
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
 test_done
diff --git a/transport.c b/transport.c
index 47fda6a7732f4b8cdcb6e750f36b896a988ffd0b..76b0645a25fe936190ca52d595b5f02eedc27d23 100644
--- a/transport.c
+++ b/transport.c
@@ -921,12 +921,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 
 	close(data->fd[1]);
 	close(data->fd[0]);
-	/*
-	 * Atomic push may abort the connection early and close the pipe,
-	 * which may cause an error for `finish_connect()`. Ignore this error
-	 * for atomic git-push.
-	 */
-	if (ret || args.atomic)
+	if (ret)
 		finish_connect(data->conn);
 	else
 		ret = finish_connect(data->conn);
@@ -1503,7 +1498,7 @@ int transport_push(struct repository *r,
 			transport_update_tracking_ref(transport->remote, ref, verbose);
 	}
 
-	if (porcelain && !push_ret)
+	if (porcelain)
 		puts("Done");
 	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
 		/* stable plumbing output; do not modify or localize */

-- 
2.47.0.251.gb31fb630c0.dirty

Re: [PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Jiang Xin]

On Wed, Nov 13, 2024 at 7:25 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> When executing git-push(1) with the "--porcelain" flag, then we will
> print updated references in a machine-readable format that looks like
> this:
>
>     To destination
>     =   refs/heads/noop:refs/heads/noop [up to date]
>     !   refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)
>     !   refs/heads/noff:refs/heads/(off (non-fast-forward)
>     Done
>
> The final "Done" stanza was introduced via 77555854be (git-push: make
> git push --porcelain print "Done", 2010-02-26), where it was printed
> "unless any errors have occurred". This behaviour was later changed via
> 7dcbeaa0df (send-pack: fix inconsistent porcelain output, 2020-04-17)
> such that the stanza will also be printed when there was an error with
> atomic pushes, which was previously inconsistent with non-atomic pushes.
> The fixup commit has introduced a new issue though. During atomic pushes
> it is expected that git-receive-pack(1) may exit early, and that causes
> us to receive an error on the client-side. We (seemingly) still want to
> print the "Done" stanza, but given that we only do so when the push has
> been successful we started to ignore the error code by the child process
> completely when doing an atomic push.

I introduced the commit 7dcbeaa0df (send-pack: fix inconsistent porcelain
output, 2020-04-17), because the porcelain output for git push over local
file protocol and HTTP protocol are different and was hard to write the
some test cases to work for both protocols. I acknowledge the patch was
not perfect.

I read the commit 77555854be (git-push: make git push --porcelain
print "Done", 2010-02-26) and the code path of git push over two
protocols a second time, and find something:

The code snippet from 77555854be:

> -    ret = transport->push_refs(transport, remote_refs, flags);
> +    push_ret = transport->push_refs(transport, remote_refs, flags);
>      err = push_had_errors(remote_refs);
> -
> -    ret |= err;
> +    ret = push_ret | err;
>

The return code "push_ret" of push_refs() from different transport
vtable is not consist. For HTTP protocol, "push_ret" is zero if no
connection error or no protocol errors. So we should consider
“push_ret" as a protocol error rather than a reference update error.

If we want to print "Done" in porcelain mode when there are no
errors. (In dry run mode, ref-update-errors should not be
considered as errors, but the opposite should be regarded as errors).

Instead of using the following code,

> +    if (porcelain && !push_ret)
> +         puts("Done");

We can use like this ("pretend" is the flag for dry-run mode):

+    ret = pretend ? push_ret : (push_ret | err);
+    if (porcelain && !ret)
+         puts("Done");

I will send patches follow this patch series.

--
Jiang Xin

[PATCH v2 0/6] fix behaviors of git-push --porcelain

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When executing git-push(1) with the "--porcelain" flag, then we will
print updated references in a machine-readable format that looks
like this:

    To destination
    =   refs/heads/noop:refs/heads/noop [up to date]
    !   refs/heads/rejected:refs/heads/rejected [rejected] (atomic push
    failed)
    !   refs/heads/noff:refs/heads/(off (non-fast-forward)
    Done

The final "Done" stanza was introduced via 77555854be (git-push:
make git push --porcelain print "Done", 2010-02-26), with the
following behaviors:

 - Show a "Done" porcelain message if there are no errors.
 - Fail to update a ref in a --dry-run does not count as an error.
 - Actual rejections in non --dry-run pushes do count as errors.
 - Return a non-zero exit code if there are errors.

This patch series try to fix the broken behaviors introduced in
commit 7dcbeaa0df (send-pack: fix inconsistent porcelain output,
2020-04-17).

--

Jiang Xin (4):
  t5548: new test cases for push --porcelain and --dry-run
  push: fix the behavior of the Done message for porcelain
  push: only ignore finish_connect() for dry-run mode
  push: not send push-options to server with --dry-run

Patrick Steinhardt (2):
  t5504: modernize test by moving heredocs into test bodies
  t5543: atomic push reports exit code failure

 send-pack.c                                   |  13 +-
 .../test-0001-standard-git-push--porcelain.sh |   1 -
 ...st-0003-pre-receive-declined--porcelain.sh |   1 -
 t/t5411/test-0012-no-hook-error--porcelain.sh |   2 -
 t/t5411/test-0014-bad-protocol--porcelain.sh  |   9 -
 t/t5411/test-0021-report-ng--porcelain.sh     |   2 -
 ...est-0023-report-unexpect-ref--porcelain.sh |   1 -
 ...test-0025-report-unknown-ref--porcelain.sh |   1 -
 ...est-0033-report-with-options--porcelain.sh |   1 -
 .../test-0039-report-mixed-refs--porcelain.sh |   1 -
 t/t5504-fetch-receive-strict.sh               |  34 +-
 t/t5516-fetch-push.sh                         |   3 +-
 t/t5534-push-signed.sh                        |   1 -
 t/t5541-http-push-smart.sh                    |   1 -
 t/t5543-atomic-push.sh                        |  30 ++
 t/t5548-push-porcelain.sh                     | 335 +++++++++++++-----
 transport.c                                   |  21 +-
 17 files changed, 316 insertions(+), 141 deletions(-)

-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v2 1/6] t5548: new test cases for push --porcelain and --dry-run

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor t5548 and add new test cases for git-push with both --porcelain
and --dry-run flags in order to cover the issues reported by Patrick.

When executing git-push(1) with the "--porcelain" flag, then we will
print updated references in a machine-readable format that looks like
this:

To destination
=   refs/heads/noop:refs/heads/noop [up to date]
!   refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)
!   refs/heads/noff:refs/heads/(off (non-fast-forward)
Done

The final "Done" stanza was introduced via 77555854be (git-push: make
git push --porcelain print "Done", 2010-02-26), where it was printed
"unless any errors have occurred". For the purpose of the "Done" line,
knowing a ref will be rejected in a --dry-run does not count as an
error. Actual rejections in non --dry-run pushes do count as errors.

The new test cases will be used in the next commit to protect the above
behaviors of porcelain output of git push.

Reported-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5548-push-porcelain.sh | 338 ++++++++++++++++++++++++++++----------
 1 file changed, 250 insertions(+), 88 deletions(-)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index ecb3877aa4..ca5cf684bc 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -4,6 +4,9 @@
 #
 test_description='Test git push porcelain output'
 
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
@@ -56,10 +59,10 @@ format_and_save_expect () {
 }
 
 setup_upstream_and_workbench () {
-	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
-	# Workbench after setup : main(A)
+	# Upstream  after setup: main(B) foo(A) bar(A) baz(A)
+	# Workbench after setup: main(A)               baz(A) next(A)
 	test_expect_success "setup upstream repository and workbench" '
-		rm -rf upstream.git workbench &&
+		rm -rf upstream.git upstream-backup.git workbench &&
 		git init --bare upstream.git &&
 		git init workbench &&
 		create_commits_in workbench A B &&
@@ -68,19 +71,29 @@ setup_upstream_and_workbench () {
 			# Try to make a stable fixed width for abbreviated commit ID,
 			# this fixed-width oid will be replaced with "<OID>".
 			git config core.abbrev 7 &&
-			git remote add origin ../upstream.git &&
 			git update-ref refs/heads/main $A &&
+			git update-ref refs/heads/baz $A &&
+			git update-ref refs/heads/next $A &&
+			git remote add origin ../upstream.git &&
 			git push origin \
 				$B:refs/heads/main \
 				$A:refs/heads/foo \
 				$A:refs/heads/bar \
 				$A:refs/heads/baz
 		) &&
+		git clone --mirror upstream.git upstream-backup.git &&
 		git -C "workbench" config advice.pushUpdateRejected false &&
 		upstream=upstream.git
 	'
 }
 
+restore_upstream () {
+	test -n "$upstream" &&
+	rm -rf "$upstream" &&
+	git clone --mirror upstream-backup.git "$upstream" &&
+	git -C "$upstream" config http.receivepack true
+}
+
 run_git_push_porcelain_output_test() {
 	case $1 in
 	http)
@@ -96,19 +109,46 @@ run_git_push_porcelain_output_test() {
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
-	test_expect_success "porcelain output of successful git-push ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $A &&
-			git update-ref refs/heads/baz $A &&
-			git update-ref refs/heads/next $A &&
-			git push --porcelain --force origin \
-				main \
-				:refs/heads/foo \
-				$B:bar \
-				baz \
-				next
-		) >out &&
+	test_expect_success "git-push --porcelain --dry-run ($PROTOCOL)" '
+		test_must_fail git -C workbench push --porcelain --dry-run origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# push             : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success "git-push --porcelain --dry-run --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --dry-run --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
 		> To <URL/of/upstream.git>
@@ -121,39 +161,137 @@ run_git_push_porcelain_output_test() {
 		EOF
 		test_cmp expect actual &&
 
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success "git-push --porcelain --dry-run --atomic ($PROTOCOL)" '
+		test_must_fail git -C workbench push --porcelain --dry-run --atomic origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[rejected] (atomic push failed)
+		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# push             : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success "git-push --porcelain --dry-run --atomic --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --dry-run --atomic --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success "git-push --porcelain ($PROTOCOL)" '
+		test_when_finished restore_upstream &&
+		test_must_fail git -C workbench push --porcelain origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
 		<COMMIT-B> refs/heads/bar
 		<COMMIT-A> refs/heads/baz
-		<COMMIT-A> refs/heads/main
+		<COMMIT-B> refs/heads/main
 		<COMMIT-A> refs/heads/next
 		EOF
 		test_cmp expect actual
 	'
 
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "atomic push failed ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $B &&
-			git update-ref refs/heads/bar $A &&
-			test_must_fail git push --atomic --porcelain origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success "git-push --porcelain --force ($PROTOCOL)" '
+		test_when_finished restore_upstream &&
+		git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> !	refs/heads/bar:refs/heads/bar	[rejected] (non-fast-forward)
-		> !	(delete):refs/heads/baz	[rejected] (atomic push failed)
-		> !	refs/heads/main:refs/heads/main	[rejected] (atomic push failed)
-		Done
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
 		EOF
 		test_cmp expect actual &&
 
@@ -168,34 +306,60 @@ run_git_push_porcelain_output_test() {
 		test_cmp expect actual
 	'
 
-	test_expect_success "prepare pre-receive hook ($PROTOCOL)" '
-		test_hook --setup -C "$upstream" pre-receive <<-EOF
-		exit 1
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success "git push --porcelain --atomic ($PROTOCOL)" '
+		test_when_finished restore_upstream &&
+		test_must_fail git -C workbench push --porcelain --atomic origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[rejected] (atomic push failed)
+		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
+		> Done
 		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
 	'
 
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "pre-receive hook declined ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $B &&
-			git update-ref refs/heads/bar $A &&
-			test_must_fail git push --porcelain --force origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success "git push --porcelain --atomic --force ($PROTOCOL)" '
+		test_when_finished restore_upstream &&
+		git -C workbench push --porcelain --atomic --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> !	refs/heads/bar:refs/heads/bar	[remote rejected] (pre-receive hook declined)
-		> !	:refs/heads/baz	[remote rejected] (pre-receive hook declined)
-		> !	refs/heads/main:refs/heads/main	[remote rejected] (pre-receive hook declined)
-		Done
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
 		EOF
 		test_cmp expect actual &&
 
@@ -210,39 +374,39 @@ run_git_push_porcelain_output_test() {
 		test_cmp expect actual
 	'
 
-	test_expect_success "remove pre-receive hook ($PROTOCOL)" '
-		rm "$upstream/hooks/pre-receive"
-	'
-
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "non-fastforward push ($PROTOCOL)" '
-		(
-			cd workbench &&
-			test_must_fail git push --porcelain origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success "pre-receive hook declined ($PROTOCOL)" '
+		test_when_finished restore_upstream &&
+		test_hook --setup -C "$upstream" pre-receive <<-EOF &&
+			exit 1
+		EOF
+		test_must_fail git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> -	:refs/heads/baz	[deleted]
-		>  	refs/heads/main:refs/heads/main	<COMMIT-A>..<COMMIT-B>
-		> !	refs/heads/bar:refs/heads/bar	[rejected] (non-fast-forward)
-		Done
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[remote rejected] (pre-receive hook declined)
+		> !	:refs/heads/foo	[remote rejected] (pre-receive hook declined)
+		> !	refs/heads/main:refs/heads/main	[remote rejected] (pre-receive hook declined)
+		> !	refs/heads/next:refs/heads/next	[remote rejected] (pre-receive hook declined)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
 		<COMMIT-B> refs/heads/main
-		<COMMIT-A> refs/heads/next
 		EOF
 		test_cmp expect actual
 	'
@@ -264,10 +428,8 @@ start_httpd
 setup_upstream_and_workbench
 
 test_expect_success "setup for http" '
-	git -C upstream.git config http.receivepack true &&
 	upstream="$HTTPD_DOCUMENT_ROOT_PATH/upstream.git" &&
-	mv upstream.git "$upstream" &&
-
+	restore_upstream &&
 	git -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git
 '
 
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v2 2/6] push: fix the behavior of the Done message for porcelain

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When executing git-push(1) with the "--porcelain" flag, then we will
print updated references in a machine-readable format that looks like
this:

To destination
=   refs/heads/noop:refs/heads/noop [up to date]
!   refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)
!   refs/heads/noff:refs/heads/(off (non-fast-forward)
Done

The final "Done" stanza was introduced via 77555854be (git-push: make
git push --porcelain print "Done", 2010-02-26), with the following
behaviors:

 - Show a "Done" porcelain message if there are no errors.
 - Fail to update a ref in a --dry-run does not count as an error.
 - Actual rejections in non --dry-run pushes do count as errors.
 - Return a non-zero exit code if there are errors.

However, the behavior of the "Done" message is not consistent when
pushing with different protocols. This is because the return values of
transport->vtable->hush_refs() across different protocols are
inconsistent. For the HTTP protocol, the return value is zero when
there are no connection errors or protocol errors. We should reference
the return code of push_had_errors() to check for failures in updating
references. Since failing to update a reference in a --dry-run does not
count as an error, we should ignore the result of push_had_errors()
when both --porcelain and --dry-run options are set.

Reported-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 send-pack.c                                       | 11 ++++++-----
 t/t5411/test-0001-standard-git-push--porcelain.sh |  1 -
 .../test-0003-pre-receive-declined--porcelain.sh  |  1 -
 t/t5411/test-0012-no-hook-error--porcelain.sh     |  2 --
 t/t5411/test-0014-bad-protocol--porcelain.sh      |  9 ---------
 t/t5411/test-0021-report-ng--porcelain.sh         |  2 --
 .../test-0023-report-unexpect-ref--porcelain.sh   |  1 -
 .../test-0025-report-unknown-ref--porcelain.sh    |  1 -
 .../test-0033-report-with-options--porcelain.sh   |  1 -
 t/t5411/test-0039-report-mixed-refs--porcelain.sh |  1 -
 t/t5504-fetch-receive-strict.sh                   |  1 -
 t/t5516-fetch-push.sh                             |  3 +--
 t/t5534-push-signed.sh                            |  1 -
 t/t5541-http-push-smart.sh                        |  1 -
 t/t5548-push-porcelain.sh                         |  7 ++-----
 transport.c                                       | 15 +++++++++++++--
 16 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 6677c44e8a..4845f63737 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,
 				reject_atomic_push(remote_refs, args->send_mirror);
 				error("atomic push failed for ref %s. status: %d",
 				      ref->name, ref->status);
-				ret = args->porcelain ? 0 : -1;
+				ret = (args->porcelain && args->dry_run) ? 0 : -1;
 				goto out;
 			}
 			/* else fallthrough */
@@ -760,11 +760,12 @@ int send_pack(struct send_pack_args *args,
 
 	if (ret < 0)
 		goto out;
-
-	if (args->porcelain) {
-		ret = 0;
+	else if (args->porcelain && args->dry_run)
+		/*
+		 * Knowing a ref will be rejected in a --dry-run does not
+		 * count as an error.
+		 */
 		goto out;
-	}
 
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
diff --git a/t/t5411/test-0001-standard-git-push--porcelain.sh b/t/t5411/test-0001-standard-git-push--porcelain.sh
index 373ec3d865..5ff901454a 100644
--- a/t/t5411/test-0001-standard-git-push--porcelain.sh
+++ b/t/t5411/test-0001-standard-git-push--porcelain.sh
@@ -73,7 +73,6 @@ test_expect_success "non-fast-forward git-push ($PROTOCOL/porcelain)" '
 	> To <URL/of/upstream.git>
 	>  	<COMMIT-B>:refs/heads/next	<COMMIT-A>..<COMMIT-B>
 	> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
diff --git a/t/t5411/test-0003-pre-receive-declined--porcelain.sh b/t/t5411/test-0003-pre-receive-declined--porcelain.sh
index 67ca6dc4f8..f4cdf9db42 100644
--- a/t/t5411/test-0003-pre-receive-declined--porcelain.sh
+++ b/t/t5411/test-0003-pre-receive-declined--porcelain.sh
@@ -18,7 +18,6 @@ test_expect_success "git-push is declined ($PROTOCOL/porcelain)" '
 	> To <URL/of/upstream.git>
 	> !	<COMMIT-B>:refs/heads/main	[remote rejected] (pre-receive hook declined)
 	> !	HEAD:refs/heads/next	[remote rejected] (pre-receive hook declined)
-	Done
 	EOF
 	test_cmp expect actual &&
 
diff --git a/t/t5411/test-0012-no-hook-error--porcelain.sh b/t/t5411/test-0012-no-hook-error--porcelain.sh
index 04468b5018..563de67859 100644
--- a/t/t5411/test-0012-no-hook-error--porcelain.sh
+++ b/t/t5411/test-0012-no-hook-error--porcelain.sh
@@ -17,7 +17,6 @@ test_expect_success "proc-receive: no hook, fail to push special ref ($PROTOCOL/
 	> To <URL/of/upstream.git>
 	> *	HEAD:refs/heads/next	[new branch]
 	> !	HEAD:refs/for/main/topic	[remote rejected] (fail to run proc-receive hook)
-	Done
 	EOF
 	test_cmp expect actual &&
 
@@ -52,7 +51,6 @@ test_expect_success "proc-receive: no hook, all failed for atomic push ($PROTOCO
 	> !	<COMMIT-B>:refs/heads/main	[remote rejected] (fail to run proc-receive hook)
 	> !	HEAD:refs/heads/next	[remote rejected] (fail to run proc-receive hook)
 	> !	HEAD:refs/for/main/topic	[remote rejected] (fail to run proc-receive hook)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
diff --git a/t/t5411/test-0014-bad-protocol--porcelain.sh b/t/t5411/test-0014-bad-protocol--porcelain.sh
index 298a3d1fec..096f13a832 100644
--- a/t/t5411/test-0014-bad-protocol--porcelain.sh
+++ b/t/t5411/test-0014-bad-protocol--porcelain.sh
@@ -21,7 +21,6 @@ test_expect_success "proc-receive: bad protocol (unknown version, $PROTOCOL/porc
 	cat >expect <<-EOF &&
 	To <URL/of/upstream.git>
 	!	HEAD:refs/for/main/topic	[remote rejected] (fail to run proc-receive hook)
-	Done
 	EOF
 	test_cmp expect actual-report &&
 
@@ -59,7 +58,6 @@ test_expect_success "proc-receive: bad protocol (hook --die-read-version, $PROTO
 	cat >expect <<-EOF &&
 	To <URL/of/upstream.git>
 	!	HEAD:refs/for/main/topic	[remote rejected] (fail to run proc-receive hook)
-	Done
 	EOF
 	test_cmp expect actual &&
 	grep "remote: fatal: die with the --die-read-version option" out-$test_count &&
@@ -90,7 +88,6 @@ test_expect_success "proc-receive: bad protocol (hook --die-write-version, $PROT
 	cat >expect <<-EOF &&
 	To <URL/of/upstream.git>
 	!	HEAD:refs/for/main/topic	[remote rejected] (fail to run proc-receive hook)
-	Done
 	EOF
 	test_cmp expect actual &&
 	grep "remote: fatal: die with the --die-write-version option" out-$test_count &&
@@ -121,7 +118,6 @@ test_expect_success "proc-receive: bad protocol (hook --die-read-commands, $PROT
 	cat >expect <<-EOF &&
 	To <URL/of/upstream.git>
 	!	HEAD:refs/for/main/topic	[remote rejected] (fail to run proc-receive hook)
-	Done
 	EOF
 	test_cmp expect actual &&
 	grep "remote: fatal: die with the --die-read-commands option" out-$test_count &&
@@ -153,7 +149,6 @@ test_expect_success "proc-receive: bad protocol (hook --die-read-push-options, $
 	cat >expect <<-EOF &&
 	To <URL/of/upstream.git>
 	!	HEAD:refs/for/main/topic	[remote rejected] (fail to run proc-receive hook)
-	Done
 	EOF
 	test_cmp expect actual &&
 	grep "remote: fatal: die with the --die-read-push-options option" out-$test_count &&
@@ -183,7 +178,6 @@ test_expect_success "proc-receive: bad protocol (hook --die-write-report, $PROTO
 	cat >expect <<-EOF &&
 	To <URL/of/upstream.git>
 	!	HEAD:refs/for/main/topic	[remote rejected] (fail to run proc-receive hook)
-	Done
 	EOF
 	test_cmp expect actual &&
 	grep "remote: fatal: die with the --die-write-report option" out-$test_count &&
@@ -219,7 +213,6 @@ test_expect_success "proc-receive: bad protocol (no report, $PROTOCOL/porcelain)
 	> To <URL/of/upstream.git>
 	> *	HEAD:refs/heads/next	[new branch]
 	> !	HEAD:refs/for/main/topic	[remote rejected] (proc-receive failed to report status)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
@@ -260,7 +253,6 @@ test_expect_success "proc-receive: bad protocol (no ref, $PROTOCOL/porcelain)" '
 	> remote: error: proc-receive reported incomplete status line: "ok"        Z
 	> To <URL/of/upstream.git>
 	> !	HEAD:refs/for/main/topic	[remote rejected] (proc-receive failed to report status)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
@@ -294,7 +286,6 @@ test_expect_success "proc-receive: bad protocol (unknown status, $PROTOCOL/porce
 	> remote: error: proc-receive reported bad status "xx" on ref "refs/for/main/topic"        Z
 	> To <URL/of/upstream.git>
 	> !	HEAD:refs/for/main/topic	[remote rejected] (proc-receive failed to report status)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
diff --git a/t/t5411/test-0021-report-ng--porcelain.sh b/t/t5411/test-0021-report-ng--porcelain.sh
index 502b34fe3d..c4b1d25562 100644
--- a/t/t5411/test-0021-report-ng--porcelain.sh
+++ b/t/t5411/test-0021-report-ng--porcelain.sh
@@ -22,7 +22,6 @@ test_expect_success "proc-receive: fail to update (ng, no message, $PROTOCOL/por
 	> remote: proc-receive> ng refs/for/main/topic        Z
 	> To <URL/of/upstream.git>
 	> !	HEAD:refs/for/main/topic	[remote rejected] (failed)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
@@ -55,7 +54,6 @@ test_expect_success "proc-receive: fail to update (ng, with message, $PROTOCOL/p
 	> remote: proc-receive> ng refs/for/main/topic error msg        Z
 	> To <URL/of/upstream.git>
 	> !	HEAD:refs/for/main/topic	[remote rejected] (error msg)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
diff --git a/t/t5411/test-0023-report-unexpect-ref--porcelain.sh b/t/t5411/test-0023-report-unexpect-ref--porcelain.sh
index 6d116ef692..d224e2400e 100644
--- a/t/t5411/test-0023-report-unexpect-ref--porcelain.sh
+++ b/t/t5411/test-0023-report-unexpect-ref--porcelain.sh
@@ -28,7 +28,6 @@ test_expect_success "proc-receive: report unexpected ref ($PROTOCOL/porcelain)"
 	> To <URL/of/upstream.git>
 	>  	<COMMIT-B>:refs/heads/main	<COMMIT-A>..<COMMIT-B>
 	> !	HEAD:refs/for/main/topic	[remote rejected] (proc-receive failed to report status)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
diff --git a/t/t5411/test-0025-report-unknown-ref--porcelain.sh b/t/t5411/test-0025-report-unknown-ref--porcelain.sh
index 8b3f5d05a3..278fc597eb 100644
--- a/t/t5411/test-0025-report-unknown-ref--porcelain.sh
+++ b/t/t5411/test-0025-report-unknown-ref--porcelain.sh
@@ -23,7 +23,6 @@ test_expect_success "proc-receive: report unknown reference ($PROTOCOL/porcelain
 	> remote: error: proc-receive reported status on unknown ref: refs/for/main/topic        Z
 	> To <URL/of/upstream.git>
 	> !	HEAD:refs/for/a/b/c/my/topic	[remote rejected] (proc-receive failed to report status)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
diff --git a/t/t5411/test-0033-report-with-options--porcelain.sh b/t/t5411/test-0033-report-with-options--porcelain.sh
index 2e1831b104..2c1117457f 100644
--- a/t/t5411/test-0033-report-with-options--porcelain.sh
+++ b/t/t5411/test-0033-report-with-options--porcelain.sh
@@ -25,7 +25,6 @@ test_expect_success "proc-receive: report option without matching ok ($PROTOCOL/
 	> remote: error: proc-receive reported "option" without a matching "ok/ng" directive        Z
 	> To <URL/of/upstream.git>
 	> !	HEAD:refs/for/main/topic	[remote rejected] (proc-receive failed to report status)
-	> Done
 	EOF
 	test_cmp expect actual
 '
diff --git a/t/t5411/test-0039-report-mixed-refs--porcelain.sh b/t/t5411/test-0039-report-mixed-refs--porcelain.sh
index 40f4c5b1af..e1b64edea9 100644
--- a/t/t5411/test-0039-report-mixed-refs--porcelain.sh
+++ b/t/t5411/test-0039-report-mixed-refs--porcelain.sh
@@ -63,7 +63,6 @@ test_expect_success "proc-receive: report update of mixed refs ($PROTOCOL/porcel
 	>  	HEAD:refs/for/main/topic	<COMMIT-A>..<COMMIT-B>
 	> !	HEAD:refs/for/next/topic1	[remote rejected] (fail to call Web API)
 	> !	HEAD:refs/for/next/topic3	[remote rejected] (proc-receive failed to report status)
-	> Done
 	EOF
 	test_cmp expect actual &&
 
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 138e6778a4..bf33cc69d0 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -68,7 +68,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 cat >exp <<EOF
 To dst
 !	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
-Done
 EOF
 
 test_expect_success 'push without strict' '
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 331778bd42..b133ab6ffc 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1190,7 +1190,6 @@ test_expect_success 'push --porcelain rejected' '
 
 	echo >.git/foo  "To testrepo"  &&
 	echo >>.git/foo "!	refs/heads/main:refs/heads/main	[remote rejected] (branch is currently checked out)" &&
-	echo >>.git/foo "Done" &&
 
 	test_must_fail git push >.git/bar --porcelain  testrepo refs/heads/main:refs/heads/main &&
 	test_cmp .git/foo .git/bar
@@ -1207,7 +1206,7 @@ test_expect_success 'push --porcelain --dry-run rejected' '
 	echo >>.git/foo "!	refs/heads/main^:refs/heads/main	[rejected] (non-fast-forward)" &&
 	echo >>.git/foo "Done" &&
 
-	test_must_fail git push >.git/bar --porcelain  --dry-run testrepo refs/heads/main^:refs/heads/main &&
+	git push >.git/bar --porcelain  --dry-run testrepo refs/heads/main^:refs/heads/main &&
 	test_cmp .git/foo .git/bar
 '
 
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index d43aee0c32..2e40bad53d 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -385,7 +385,6 @@ test_expect_success GPG 'failed atomic push does not execute GPG' '
 	=	refs/heads/noop:refs/heads/noop	[up to date]
 	!	refs/heads/ff:refs/heads/ff	[rejected] (atomic push failed)
 	!	refs/heads/noff:refs/heads/noff	[rejected] (non-fast-forward)
-	Done
 	EOF
 	test_cmp expect out
 '
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 3ad514bbd4..b543c10314 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -499,7 +499,6 @@ test_expect_success 'report error server does not provide ref status' '
 	cat >expect <<-EOF &&
 	To $HTTPD_URL/smart/no_report
 	!	HEAD:refs/tags/will-fail	[remote failure] (remote failed to report status)
-	Done
 	EOF
 	test_cmp expect actual
 '
diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index ca5cf684bc..424391eadd 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -110,7 +110,7 @@ run_git_push_porcelain_output_test() {
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
 	test_expect_success "git-push --porcelain --dry-run ($PROTOCOL)" '
-		test_must_fail git -C workbench push --porcelain --dry-run origin \
+		git -C workbench push --porcelain --dry-run origin \
 			main \
 			:refs/heads/foo \
 			$B:bar \
@@ -176,7 +176,7 @@ run_git_push_porcelain_output_test() {
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
 	test_expect_success "git-push --porcelain --dry-run --atomic ($PROTOCOL)" '
-		test_must_fail git -C workbench push --porcelain --dry-run --atomic origin \
+		git -C workbench push --porcelain --dry-run --atomic origin \
 			main \
 			:refs/heads/foo \
 			$B:bar \
@@ -257,7 +257,6 @@ run_git_push_porcelain_output_test() {
 		> -	:refs/heads/foo	[deleted]
 		> *	refs/heads/next:refs/heads/next	[new branch]
 		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
-		> Done
 		EOF
 		test_cmp expect actual &&
 
@@ -325,7 +324,6 @@ run_git_push_porcelain_output_test() {
 		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
 		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
 		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
-		> Done
 		EOF
 		test_cmp expect actual &&
 
@@ -396,7 +394,6 @@ run_git_push_porcelain_output_test() {
 		> !	:refs/heads/foo	[remote rejected] (pre-receive hook declined)
 		> !	refs/heads/main:refs/heads/main	[remote rejected] (pre-receive hook declined)
 		> !	refs/heads/next:refs/heads/next	[remote rejected] (pre-receive hook declined)
-		> Done
 		EOF
 		test_cmp expect actual &&
 
diff --git a/transport.c b/transport.c
index 47fda6a773..9e03a7148c 100644
--- a/transport.c
+++ b/transport.c
@@ -1486,7 +1486,18 @@ int transport_push(struct repository *r,
 	} else
 		push_ret = 0;
 	err = push_had_errors(remote_refs);
-	ret = push_ret | err;
+	/*
+	 * The return values of transport->vtable->hush_refs() across
+	 * different protocols are inconsistent. For the HTTP protocol,
+	 * the return value is zero when there are no connection errors
+	 * or protocol errors. We should reference the return code of
+	 * push_had_errors() to check for failures in updating references.
+	 * Since failing to update a reference in a --dry-run does not
+	 * count as an error, we could ignore the result of
+	 * push_had_errors() when both --porcelain and --dry-run options
+	 * are set.
+	 */
+	ret = (porcelain && pretend) ? push_ret : (push_ret | err);
 
 	if (!quiet || err)
 		transport_print_push_status(transport->url, remote_refs,
@@ -1503,7 +1514,7 @@ int transport_push(struct repository *r,
 			transport_update_tracking_ref(transport->remote, ref, verbose);
 	}
 
-	if (porcelain && !push_ret)
+	if (porcelain && !ret)
 		puts("Done");
 	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
 		/* stable plumbing output; do not modify or localize */
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v2 3/6] t5504: modernize test by moving heredocs into test bodies

[Jiang Xin]

From: Patrick Steinhardt <ps@pks.im>

We have several heredocs in t5504 located outside of any particular test
bodies. Move these into the test bodies to match our modern coding
style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5504-fetch-receive-strict.sh | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index bf33cc69d0..56b31cf557 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -65,11 +65,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 	)
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
-EOF
-
 test_expect_success 'push without strict' '
 	rm -rf dst &&
 	git init dst &&
@@ -78,6 +73,10 @@ test_expect_success 'push without strict' '
 		git config fetch.fsckobjects false &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '
 	test_cmp exp act
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
-EOF
-
 test_expect_success 'push with receive.fsckobjects' '
 	rm -rf dst &&
 	git init dst &&
@@ -107,6 +101,10 @@ test_expect_success 'push with receive.fsckobjects' '
 		git config receive.fsckobjects true &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -129,15 +127,14 @@ test_expect_success 'repair the "corrupt or missing" object' '
 	git fsck
 '
 
-cat >bogus-commit <<EOF
-tree $EMPTY_TREE
-author Bugs Bunny 1234567890 +0000
-committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
-
-This commit object intentionally broken
-EOF
-
 test_expect_success 'setup bogus commit' '
+	cat >bogus-commit <<-EOF &&
+	tree $EMPTY_TREE
+	author Bugs Bunny 1234567890 +0000
+	committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
+
+	This commit object intentionally broken
+	EOF
 	commit="$(git hash-object --literally -t commit -w --stdin <bogus-commit)"
 '
 
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v2 4/6] t5543: atomic push reports exit code failure

[Jiang Xin]

From: Patrick Steinhardt <ps@pks.im>

Add new test cases in t5543 to avoid ignoring the exit code of
git-receive-pack(1) during atomic push with "--porcelain" flag.

We'd typically notice this case because the refs would have their error
message set. But there is an edge case when pushing refs succeeds, but
git-receive-pack(1) exits with a non-zero exit code at a later point in
time due to another error. An atomic git-push(1) would ignore that error
code, and consequently it would return successfully and not print any
error message at all.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 479d103469..a40afb949b 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -281,4 +281,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'atomic push reports exit code failure' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict 2>err &&
+	cat >expect <<-EOF &&
+	To ../upstream
+	 * [new branch]      HEAD -> no-conflict
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
+test_expect_failure 'atomic push reports exit code failure with porcelain' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic --porcelain \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict-porcelain 2>err &&
+	cat >expect <<-EOF &&
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
 test_done
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v2 5/6] push: only ignore finish_connect() for dry-run mode

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Patrick reported an issue that the exit code of git-receive-pack(1) is
ignored during atomic push with "--porcelain" flag, and added new test
cases in t5543.

Atomic push may abort the connection early and close the pipe, which
may cause an error for `finish_connect()`. Arbitrarily ignoring this
error will lead to issues. Modify it to only ignore the finish_connect()
error when both the --atomic and --dry-run flags are set.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5543-atomic-push.sh | 4 ++--
 transport.c            | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index a40afb949b..1d8f088a00 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -281,7 +281,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'atomic push reports exit code failure' '
+test_expect_success 'atomic push reports exit code failure' '
 	write_script receive-pack-wrapper <<-\EOF &&
 	git-receive-pack "$@"
 	exit 1
@@ -297,7 +297,7 @@ test_expect_failure 'atomic push reports exit code failure' '
 	test_cmp expect err
 '
 
-test_expect_failure 'atomic push reports exit code failure with porcelain' '
+test_expect_success 'atomic push reports exit code failure with porcelain' '
 	write_script receive-pack-wrapper <<-\EOF &&
 	git-receive-pack "$@"
 	exit 1
diff --git a/transport.c b/transport.c
index 9e03a7148c..186d58e907 100644
--- a/transport.c
+++ b/transport.c
@@ -923,10 +923,10 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	close(data->fd[0]);
 	/*
 	 * Atomic push may abort the connection early and close the pipe,
-	 * which may cause an error for `finish_connect()`. Ignore this error
-	 * for atomic git-push.
+	 * which may cause an error for `finish_connect()`. We can ignore
+	 * this error when both `--atomic` and `--dry-run` flags provided.
 	 */
-	if (ret || args.atomic)
+	if (ret || (args.atomic && args.dry_run))
 		finish_connect(data->conn);
 	else
 		ret = finish_connect(data->conn);
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v2 6/6] push: not send push-options to server with --dry-run

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 send-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/send-pack.c b/send-pack.c
index 4845f63737..89a2d47928 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -678,7 +678,7 @@ int send_pack(struct send_pack_args *args,
 		}
 	}
 
-	if (use_push_options) {
+	if (use_push_options && !args->dry_run) {
 		struct string_list_item *item;
 
 		packet_buf_flush(&req_buf);
-- 
2.47.0.rc1.21.g81e7bd6151

Re: [PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> If we want to print "Done" in porcelain mode when there are no
> errors. (In dry run mode, ref-update-errors should not be
> considered as errors, but the opposite should be regarded as errors).

Hmph, should we allow dry-run to deliberately behave differently
from the for-real execution?  Wouldn't it discard the primary value
of dry-run, to see if it is likely that an operation will succeed
(or fail)?

Re: [PATCH v2 1/6] t5548: new test cases for push --porcelain and --dry-run

[Patrick Steinhardt]

On Fri, Nov 15, 2024 at 01:15:32AM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> 
> Refactor t5548 and add new test cases for git-push with both --porcelain
> and --dry-run flags in order to cover the issues reported by Patrick.
> 
> When executing git-push(1) with the "--porcelain" flag, then we will
> print updated references in a machine-readable format that looks like
> this:
> 
> To destination
> =   refs/heads/noop:refs/heads/noop [up to date]
> !   refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)
> !   refs/heads/noff:refs/heads/(off (non-fast-forward)
> Done
> 
> The final "Done" stanza was introduced via 77555854be (git-push: make
> git push --porcelain print "Done", 2010-02-26), where it was printed
> "unless any errors have occurred". For the purpose of the "Done" line,
> knowing a ref will be rejected in a --dry-run does not count as an
> error. Actual rejections in non --dry-run pushes do count as errors.
> 
> The new test cases will be used in the next commit to protect the above
> behaviors of porcelain output of git push.

It's a bit hard to make sense of this commit: does it introduce new
tests? Does it refactor existing tests? Does it change the test setup?
It seems to be a mixture of all of these, which makes it hard to see
what the actual change is.

I'd propose to split up this commit into multiple ones: one that
introduces `restore_upstream()`, one that adapts the test data, and one
that introduces new tests.

Patrick

Re: [PATCH v2 2/6] push: fix the behavior of the Done message for porcelain

[Patrick Steinhardt]

On Fri, Nov 15, 2024 at 01:15:33AM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> 
> When executing git-push(1) with the "--porcelain" flag, then we will
> print updated references in a machine-readable format that looks like
> this:
> 
> To destination
> =   refs/heads/noop:refs/heads/noop [up to date]
> !   refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)
> !   refs/heads/noff:refs/heads/(off (non-fast-forward)
> Done
> 
> The final "Done" stanza was introduced via 77555854be (git-push: make
> git push --porcelain print "Done", 2010-02-26), with the following
> behaviors:
> 
>  - Show a "Done" porcelain message if there are no errors.

It would be great to update our documentation so that we spell out when
exactly the user can expect such a "Done" message.

>  - Fail to update a ref in a --dry-run does not count as an error.
>  - Actual rejections in non --dry-run pushes do count as errors.
>  - Return a non-zero exit code if there are errors.

> However, the behavior of the "Done" message is not consistent when
> pushing with different protocols. This is because the return values of
> transport->vtable->hush_refs() across different protocols are

s/hush_refs/push_refs/

> diff --git a/send-pack.c b/send-pack.c
> index 6677c44e8a..4845f63737 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,
>  				reject_atomic_push(remote_refs, args->send_mirror);
>  				error("atomic push failed for ref %s. status: %d",
>  				      ref->name, ref->status);
> -				ret = args->porcelain ? 0 : -1;
> +				ret = (args->porcelain && args->dry_run) ? 0 : -1;
>  				goto out;
>  			}
>  			/* else fallthrough */

This feels weird to me. Why would we ignore an error with `--dry-run`?
Doesn't that mean that the resulting behaviour is now different
depending on whether or not you pass `--dry-run`?

> @@ -760,11 +760,12 @@ int send_pack(struct send_pack_args *args,
>  
>  	if (ret < 0)
>  		goto out;
> -
> -	if (args->porcelain) {
> -		ret = 0;
> +	else if (args->porcelain && args->dry_run)
> +		/*
> +		 * Knowing a ref will be rejected in a --dry-run does not
> +		 * count as an error.
> +		 */
>  		goto out;
> -	}
>  
>  	for (ref = remote_refs; ref; ref = ref->next) {
>  		switch (ref->status) {

Same question here.

> diff --git a/t/t5411/test-0001-standard-git-push--porcelain.sh b/t/t5411/test-0001-standard-git-push--porcelain.sh
> index 373ec3d865..5ff901454a 100644
> --- a/t/t5411/test-0001-standard-git-push--porcelain.sh
> +++ b/t/t5411/test-0001-standard-git-push--porcelain.sh
> @@ -73,7 +73,6 @@ test_expect_success "non-fast-forward git-push ($PROTOCOL/porcelain)" '
>  	> To <URL/of/upstream.git>
>  	>  	<COMMIT-B>:refs/heads/next	<COMMIT-A>..<COMMIT-B>
>  	> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
> -	> Done
>  	EOF
>  	test_cmp expect actual &&

77555854be (git-push: make git push --porcelain print "Done",
2010-02-26) seems to cover this case, where it mentions that a
conflicting ref update does not count as error with "--dry-run", but
does count as error without that option. And this change here seems to
be in line with that.

Whether the documented behaviour of that commit is reasonable is a
different question though. I find it to be of dubious nature to make any
runtime behaviour (other than committing the actual change) depend on
"--dry-run". The behaviour should be the exact same, as otherwise the
dry run becomes useless if the behaviour differs.

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 331778bd42..b133ab6ffc 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1207,7 +1206,7 @@ test_expect_success 'push --porcelain --dry-run rejected' '
>  	echo >>.git/foo "!	refs/heads/main^:refs/heads/main	[rejected] (non-fast-forward)" &&
>  	echo >>.git/foo "Done" &&
>  
> -	test_must_fail git push >.git/bar --porcelain  --dry-run testrepo refs/heads/main^:refs/heads/main &&
> +	git push >.git/bar --porcelain  --dry-run testrepo refs/heads/main^:refs/heads/main &&
>  	test_cmp .git/foo .git/bar
>  '

Huh. So this is the dubious part I mean. It seems like the push would
fail without "--dry-run", but because we pass it it does not indicate
that failure via the exit code. This behaviour does not make any sense
to me. There are more cases in your change where we stop failing now.

> diff --git a/transport.c b/transport.c
> index 47fda6a773..9e03a7148c 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1486,7 +1486,18 @@ int transport_push(struct repository *r,
>  	} else
>  		push_ret = 0;
>  	err = push_had_errors(remote_refs);
> -	ret = push_ret | err;
> +	/*
> +	 * The return values of transport->vtable->hush_refs() across

s/hush_refs/push_refs/

> +	 * different protocols are inconsistent. For the HTTP protocol,
> +	 * the return value is zero when there are no connection errors
> +	 * or protocol errors. We should reference the return code of
> +	 * push_had_errors() to check for failures in updating references.
> +	 * Since failing to update a reference in a --dry-run does not
> +	 * count as an error, we could ignore the result of
> +	 * push_had_errors() when both --porcelain and --dry-run options
> +	 * are set.
> +	 */
> +	ret = (porcelain && pretend) ? push_ret : (push_ret | err);
>  
>  	if (!quiet || err)
>  		transport_print_push_status(transport->url, remote_refs,
> @@ -1503,7 +1514,7 @@ int transport_push(struct repository *r,
>  			transport_update_tracking_ref(transport->remote, ref, verbose);
>  	}
>  
> -	if (porcelain && !push_ret)
> +	if (porcelain && !ret)
>  		puts("Done");
>  	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
>  		/* stable plumbing output; do not modify or localize */

I'm not yet convinced that it makes sense to ignore either `ret` or
`push_ret` in the above two hunks. Making this dependent on `porcelain`
may be okay if we want to report errors via the output, but making the
behaviour dependent on whether or not we perform a dry run continues to
feel wrong to me.

Patrick

Re: [PATCH v2 5/6] push: only ignore finish_connect() for dry-run mode

[Patrick Steinhardt]

On Fri, Nov 15, 2024 at 01:15:36AM +0800, Jiang Xin wrote:
> diff --git a/transport.c b/transport.c
> index 9e03a7148c..186d58e907 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -923,10 +923,10 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
>  	close(data->fd[0]);
>  	/*
>  	 * Atomic push may abort the connection early and close the pipe,
> -	 * which may cause an error for `finish_connect()`. Ignore this error
> -	 * for atomic git-push.
> +	 * which may cause an error for `finish_connect()`. We can ignore
> +	 * this error when both `--atomic` and `--dry-run` flags provided.
>  	 */
> -	if (ret || args.atomic)
> +	if (ret || (args.atomic && args.dry_run))
>  		finish_connect(data->conn);
>  	else
>  		ret = finish_connect(data->conn);

In the same spirit as mentioned in other commits I don't think it is a
good idea to condition this behaviour on "--dry-run".

Patrick

Re: [PATCH v2 6/6] push: not send push-options to server with --dry-run

[Patrick Steinhardt]

On Fri, Nov 15, 2024 at 01:15:37AM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> 
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>

It would be nice to have a summary of what this change intends to do and
why that is a good idea.

Patrick

Re: [PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Patrick Steinhardt]

On Fri, Nov 15, 2024 at 08:36:20AM +0900, Junio C Hamano wrote:
> Jiang Xin <worldhello.net@gmail.com> writes:
> 
> > If we want to print "Done" in porcelain mode when there are no
> > errors. (In dry run mode, ref-update-errors should not be
> > considered as errors, but the opposite should be regarded as errors).
> 
> Hmph, should we allow dry-run to deliberately behave differently
> from the for-real execution?  Wouldn't it discard the primary value
> of dry-run, to see if it is likely that an operation will succeed
> (or fail)?

I agree, I don't think that this is a good idea. "--dry-run" should
behave as close as possible to the same command executed without that
flag, the only change should be that the underlying operation is not
actually applied in the end. But other than that both the output and the
return values should ideally match exactly so that the user can actually
tell what would happen.

Patrick

Re: [PATCH v2 1/6] t5548: new test cases for push --porcelain and --dry-run

[Jiang Xin]

On Mon, Nov 25, 2024 at 4:26 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Nov 15, 2024 at 01:15:32AM +0800, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > Refactor t5548 and add new test cases for git-push with both --porcelain
> > and --dry-run flags in order to cover the issues reported by Patrick.
> >
> > When executing git-push(1) with the "--porcelain" flag, then we will
> > print updated references in a machine-readable format that looks like
> > this:
> >
> > To destination
> > =   refs/heads/noop:refs/heads/noop [up to date]
> > !   refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)
> > !   refs/heads/noff:refs/heads/(off (non-fast-forward)
> > Done
> >
> > The final "Done" stanza was introduced via 77555854be (git-push: make
> > git push --porcelain print "Done", 2010-02-26), where it was printed
> > "unless any errors have occurred". For the purpose of the "Done" line,
> > knowing a ref will be rejected in a --dry-run does not count as an
> > error. Actual rejections in non --dry-run pushes do count as errors.
> >
> > The new test cases will be used in the next commit to protect the above
> > behaviors of porcelain output of git push.
>
> It's a bit hard to make sense of this commit: does it introduce new
> tests? Does it refactor existing tests? Does it change the test setup?
> It seems to be a mixture of all of these, which makes it hard to see
> what the actual change is.
>
> I'd propose to split up this commit into multiple ones: one that
> introduces `restore_upstream()`, one that adapts the test data, and one
> that introduces new tests.

I've been quite busy lately, but I plan to revisit this next week,
incorporating both your and Junio's suggestions. Thank you for your
input!

>
> Patrick

[PATCH v3 0/8] fix behaviors of git-push --porcelain

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Patrick reported an issue that the exit code of git-receive-pack(1)
is ignored during atomic push with "--porcelain" flag, and added new
test cases in t5543.

This issue originated from commit 7dcbeaa0df (send-pack: fix
inconsistent porcelain output, 2020-04-17). At that time, I chose to
ignore the exit code of "finish_connect()" without investigating the
root cause of the abnormal termination of git-receive-pack. That was an
incorrect solution.

The root cause is that an atomic push operation terminates early without
sending a flush packet to git-receive-pack. As a result,
git-receive-pack continues waiting for commands without exiting. By
sending a flush packet at the appropriate location in "send_pack()", we
ensure that the git-receive-pack process closes properly, avoiding an
erroneous exit code for git-push.


# Changes since v2

* Split the changes made to t5548 into several commits for clarity.

* Correct the inconsistent return code of "push_refs()" across different
  transports to ensure that git push --porcelain behaves consistently.

* Gracefully close the connection for early exist of atomic push to
  resolve to root cause of wrong exit code of atomic git-push.

--

Jiang Xin (6):
  t5548: refactor to reuse setup_upstream() function
  t5548: refactor test cases by resetting upstream
  t5548: add new porcelain test cases
  t5548: add porcelain push test cases for dry-run mode
  send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"
  send-pack: gracefully close the connection for atomic push

Patrick Steinhardt (2):
  t5504: modernize test by moving heredocs into test bodies
  t5543: atomic push reports exit code failure

 send-pack.c                     |  10 +-
 send-pack.h                     |   3 +
 t/t5504-fetch-receive-strict.sh |  35 ++-
 t/t5543-atomic-push.sh          |  30 +++
 t/t5548-push-porcelain.sh       | 425 +++++++++++++++++++++++---------
 transport.c                     |  17 +-
 6 files changed, 374 insertions(+), 146 deletions(-)

-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v3 1/8] t5504: modernize test by moving heredocs into test bodies

[Jiang Xin]

From: Patrick Steinhardt <ps@pks.im>

We have several heredocs in t5504 located outside of any particular test
bodies. Move these into the test bodies to match our modern coding
style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5504-fetch-receive-strict.sh | 35 +++++++++++++++------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 53dbc8ce3a..488310c340 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 	)
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
-Done
-EOF
-
 test_expect_success 'push without strict' '
 	rm -rf dst &&
 	git init dst &&
@@ -78,6 +72,11 @@ test_expect_success 'push without strict' '
 		git config fetch.fsckobjects false &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
+	Done
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '
 	test_cmp exp act
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
-EOF
-
 test_expect_success 'push with receive.fsckobjects' '
 	rm -rf dst &&
 	git init dst &&
@@ -107,6 +101,10 @@ test_expect_success 'push with receive.fsckobjects' '
 		git config receive.fsckobjects true &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -129,15 +127,14 @@ test_expect_success 'repair the "corrupt or missing" object' '
 	git fsck
 '
 
-cat >bogus-commit <<EOF
-tree $EMPTY_TREE
-author Bugs Bunny 1234567890 +0000
-committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
-
-This commit object intentionally broken
-EOF
-
 test_expect_success 'setup bogus commit' '
+	cat >bogus-commit <<-EOF &&
+	tree $EMPTY_TREE
+	author Bugs Bunny 1234567890 +0000
+	committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
+
+	This commit object intentionally broken
+	EOF
 	commit="$(git hash-object --literally -t commit -w --stdin <bogus-commit)"
 '
 
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v3 2/8] t5548: refactor to reuse setup_upstream() function

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor the function setup_upstream_and_workbench(), extracting
create_upstream_template() and setup_upstream() from it. The former is
used to create the upstream repository template, while the latter is
used to rebuild the upstream repository and will be reused in subsequent
commits.

To ensure that setup_upstream() works properly in both local and HTTP
protocols, the HTTP settings have been moved to the setup_upstream() and
setup_upstream_and_workbench() functions.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5548-push-porcelain.sh | 85 +++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 30 deletions(-)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index 6282728eaf..a3defd5b75 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -54,29 +54,67 @@ format_and_save_expect () {
 	sed -e 's/^> //' -e 's/Z$//' >expect
 }
 
+create_upstream_template () {
+	git init --bare upstream-template.git &&
+	git clone upstream-template.git tmp_work_dir &&
+	create_commits_in tmp_work_dir A B &&
+	(
+		cd tmp_work_dir &&
+		git push origin \
+			$B:refs/heads/main \
+			$A:refs/heads/foo \
+			$A:refs/heads/bar \
+			$A:refs/heads/baz
+	) &&
+	rm -rf tmp_work_dir
+}
+
+setup_upstream () {
+	if test $# -ne 1
+	then
+		BUG "location of upstream repository is not provided"
+	fi &&
+	rm -rf "$1" &&
+	if ! test -d upstream-template.git
+	then
+		create_upstream_template
+	fi &&
+	git clone --mirror upstream-template.git "$1" &&
+	# The upstream repository provides services using the HTTP protocol.
+	if ! test "$1" = "upstream.git"
+	then
+		git -C "$1" config http.receivepack true
+	fi
+}
+
 setup_upstream_and_workbench () {
+	if test $# -ne 1
+	then
+		BUG "location of upstream repository is not provided"
+	fi &&
+	# Assign the first argument to the variable upstream;
+	# we will use it in the subsequent test cases.
+	upstream="$1"
+
 	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
 	# Workbench after setup : main(A)
 	test_expect_success "setup upstream repository and workbench" '
-		rm -rf upstream.git workbench &&
-		git init --bare upstream.git &&
-		git init workbench &&
-		create_commits_in workbench A B &&
+		setup_upstream "$upstream" &&
+		rm -rf workbench &&
+		git clone "$upstream" workbench &&
 		(
 			cd workbench &&
+			git update-ref refs/heads/main $A &&
 			# Try to make a stable fixed width for abbreviated commit ID,
 			# this fixed-width oid will be replaced with "<OID>".
 			git config core.abbrev 7 &&
-			git remote add origin ../upstream.git &&
-			git update-ref refs/heads/main $A &&
-			git push origin \
-				$B:refs/heads/main \
-				$A:refs/heads/foo \
-				$A:refs/heads/bar \
-				$A:refs/heads/baz
+			git config advice.pushUpdateRejected false
 		) &&
-		git -C "workbench" config advice.pushUpdateRejected false &&
-		upstream=upstream.git
+		# The upstream repository provides services using the HTTP protocol.
+		if ! test "$upstream" = "upstream.git"
+		then
+			git -C workbench remote set-url origin "$HTTPD_URL/smart/upstream.git"
+		fi
 	'
 }
 
@@ -88,7 +126,7 @@ run_git_push_porcelain_output_test() {
 		;;
 	file)
 		PROTOCOL="builtin protocol"
-		URL_PREFIX="\.\."
+		URL_PREFIX=".*"
 		;;
 	esac
 
@@ -247,10 +285,8 @@ run_git_push_porcelain_output_test() {
 	'
 }
 
-# Initialize the upstream repository and local workbench.
-setup_upstream_and_workbench
+setup_upstream_and_workbench upstream.git
 
-# Run git-push porcelain test on builtin protocol
 run_git_push_porcelain_output_test file
 
 ROOT_PATH="$PWD"
@@ -258,21 +294,10 @@ ROOT_PATH="$PWD"
 . "$TEST_DIRECTORY"/lib-httpd.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 start_httpd
-
-# Re-initialize the upstream repository and local workbench.
-setup_upstream_and_workbench
-
-test_expect_success "setup for http" '
-	git -C upstream.git config http.receivepack true &&
-	upstream="$HTTPD_DOCUMENT_ROOT_PATH/upstream.git" &&
-	mv upstream.git "$upstream" &&
-
-	git -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git
-'
-
 setup_askpass_helper
 
-# Run git-push porcelain test on HTTP protocol
+setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
+
 run_git_push_porcelain_output_test http
 
 test_done
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v3 3/8] t5548: refactor test cases by resetting upstream

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor the test cases with the following changes:

 - Calling setup_upstream() to reset upstream after running each test
   case.

 - Change the initial branch tips of the workspace to reduce the branch
   setup operations in the workspace.

 - Reduced the two steps of setting up and cleaning up the pre-receive
   hook by moving the operations into the corresponding test case,

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5548-push-porcelain.sh | 149 +++++++++++++++++---------------------
 1 file changed, 67 insertions(+), 82 deletions(-)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index a3defd5b75..ededd8edb9 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -96,8 +96,8 @@ setup_upstream_and_workbench () {
 	# we will use it in the subsequent test cases.
 	upstream="$1"
 
-	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
-	# Workbench after setup : main(A)
+	# Upstream  after setup: main(B)  foo(A)  bar(A)  baz(A)
+	# Workbench after setup: main(A)                  baz(A)  next(A)
 	test_expect_success "setup upstream repository and workbench" '
 		setup_upstream "$upstream" &&
 		rm -rf workbench &&
@@ -105,6 +105,8 @@ setup_upstream_and_workbench () {
 		(
 			cd workbench &&
 			git update-ref refs/heads/main $A &&
+			git update-ref refs/heads/baz $A &&
+			git update-ref refs/heads/next $A &&
 			# Try to make a stable fixed width for abbreviated commit ID,
 			# this fixed-width oid will be replaced with "<OID>".
 			git config core.abbrev 7 &&
@@ -133,19 +135,14 @@ run_git_push_porcelain_output_test() {
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
-	test_expect_success "porcelain output of successful git-push ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $A &&
-			git update-ref refs/heads/baz $A &&
-			git update-ref refs/heads/next $A &&
-			git push --porcelain --force origin \
-				main \
-				:refs/heads/foo \
-				$B:bar \
-				baz \
-				next
-		) >out &&
+	test_expect_success ".. git-push --porcelain --force ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
 		> To <URL/of/upstream.git>
@@ -169,115 +166,103 @@ run_git_push_porcelain_output_test() {
 		test_cmp expect actual
 	'
 
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "atomic push failed ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $B &&
-			git update-ref refs/heads/bar $A &&
-			test_must_fail git push --atomic --porcelain origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git push --porcelain --atomic ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		test_must_fail git -C workbench push --porcelain --atomic origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> !	refs/heads/bar:refs/heads/bar	[rejected] (non-fast-forward)
-		> !	(delete):refs/heads/baz	[rejected] (atomic push failed)
-		> !	refs/heads/main:refs/heads/main	[rejected] (atomic push failed)
-		Done
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[rejected] (atomic push failed)
+		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
 		<COMMIT-A> refs/heads/baz
-		<COMMIT-A> refs/heads/main
-		<COMMIT-A> refs/heads/next
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
 		EOF
 		test_cmp expect actual
 	'
 
-	test_expect_success "prepare pre-receive hook ($PROTOCOL)" '
-		test_hook --setup -C "$upstream" pre-receive <<-EOF
-		exit 1
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. pre-receive hook declined ($PROTOCOL)" '
+		test_when_finished "rm -f \"$upstream/hooks/pre-receive\" &&
+			setup_upstream \"$upstream\"" &&
+		test_hook --setup -C "$upstream" pre-receive <<-EOF &&
+			exit 1
 		EOF
-	'
-
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "pre-receive hook declined ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $B &&
-			git update-ref refs/heads/bar $A &&
-			test_must_fail git push --porcelain --force origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+		test_must_fail git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> !	refs/heads/bar:refs/heads/bar	[remote rejected] (pre-receive hook declined)
-		> !	:refs/heads/baz	[remote rejected] (pre-receive hook declined)
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[remote rejected] (pre-receive hook declined)
+		> !	:refs/heads/foo	[remote rejected] (pre-receive hook declined)
 		> !	refs/heads/main:refs/heads/main	[remote rejected] (pre-receive hook declined)
-		Done
+		> !	refs/heads/next:refs/heads/next	[remote rejected] (pre-receive hook declined)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
 		<COMMIT-A> refs/heads/baz
-		<COMMIT-A> refs/heads/main
-		<COMMIT-A> refs/heads/next
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
 		EOF
 		test_cmp expect actual
 	'
 
-	test_expect_success "remove pre-receive hook ($PROTOCOL)" '
-		rm "$upstream/hooks/pre-receive"
-	'
-
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "non-fastforward push ($PROTOCOL)" '
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)                          next(A)
+	test_expect_success ".. non-fastforward push ($PROTOCOL)" '
 		(
 			cd workbench &&
 			test_must_fail git push --porcelain origin \
 				main \
-				bar \
-				:baz \
 				next
 		) >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> -	:refs/heads/baz	[deleted]
-		>  	refs/heads/main:refs/heads/main	<COMMIT-A>..<COMMIT-B>
-		> !	refs/heads/bar:refs/heads/bar	[rejected] (non-fast-forward)
-		Done
+		> To <URL/of/upstream.git>
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
 		<COMMIT-B> refs/heads/main
 		<COMMIT-A> refs/heads/next
 		EOF
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v3 4/8] t5548: add new porcelain test cases

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Change order of test cases and add new test cases:

 - git push --porcelain                  # failed: non-fast-forward
 - git push --porcelain --force
 - git push --porcelain --atomic         # failed: non-fast-forward
 - git push --porcelain --atomic --force
 - git push --porcelain --force          # failed: pre-receive declined

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5548-push-porcelain.sh | 90 ++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 25 deletions(-)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index ededd8edb9..799f6066a3 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -4,6 +4,9 @@
 #
 test_description='Test git push porcelain output'
 
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
 . ./test-lib.sh
 
 # Create commits in <repo> and assign each commit's oid to shell variables
@@ -132,6 +135,40 @@ run_git_push_porcelain_output_test() {
 		;;
 	esac
 
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		test_must_fail git -C workbench push --porcelain origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-B> refs/heads/main
+		<COMMIT-A> refs/heads/next
+		EOF
+		test_cmp expect actual
+	'
+
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
@@ -203,13 +240,9 @@ run_git_push_porcelain_output_test() {
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
-	test_expect_success ".. pre-receive hook declined ($PROTOCOL)" '
-		test_when_finished "rm -f \"$upstream/hooks/pre-receive\" &&
-			setup_upstream \"$upstream\"" &&
-		test_hook --setup -C "$upstream" pre-receive <<-EOF &&
-			exit 1
-		EOF
-		test_must_fail git -C workbench push --porcelain --force origin \
+	test_expect_success ".. git push --porcelain --atomic --force ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		git -C workbench push --porcelain --atomic --force origin \
 			main \
 			:refs/heads/foo \
 			$B:bar \
@@ -219,10 +252,10 @@ run_git_push_porcelain_output_test() {
 		format_and_save_expect <<-EOF &&
 		> To <URL/of/upstream.git>
 		> =	refs/heads/baz:refs/heads/baz	[up to date]
-		> !	<COMMIT-B>:refs/heads/bar	[remote rejected] (pre-receive hook declined)
-		> !	:refs/heads/foo	[remote rejected] (pre-receive hook declined)
-		> !	refs/heads/main:refs/heads/main	[remote rejected] (pre-receive hook declined)
-		> !	refs/heads/next:refs/heads/next	[remote rejected] (pre-receive hook declined)
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
 		> Done
 		EOF
 		test_cmp expect actual &&
@@ -230,29 +263,37 @@ run_git_push_porcelain_output_test() {
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-A> refs/heads/bar
+		<COMMIT-B> refs/heads/bar
 		<COMMIT-A> refs/heads/baz
-		<COMMIT-A> refs/heads/foo
-		<COMMIT-B> refs/heads/main
+		<COMMIT-A> refs/heads/main
+		<COMMIT-A> refs/heads/next
 		EOF
 		test_cmp expect actual
 	'
 
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
-	# git-push         : main(A)                          next(A)
-	test_expect_success ".. non-fastforward push ($PROTOCOL)" '
-		(
-			cd workbench &&
-			test_must_fail git push --porcelain origin \
-				main \
-				next
-		) >out &&
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. pre-receive hook declined ($PROTOCOL)" '
+		test_when_finished "rm -f \"$upstream/hooks/pre-receive\" &&
+			setup_upstream \"$upstream\"" &&
+		test_hook --setup -C "$upstream" pre-receive <<-EOF &&
+			exit 1
+		EOF
+		test_must_fail git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
 		> To <URL/of/upstream.git>
-		> *	refs/heads/next:refs/heads/next	[new branch]
-		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[remote rejected] (pre-receive hook declined)
+		> !	:refs/heads/foo	[remote rejected] (pre-receive hook declined)
+		> !	refs/heads/main:refs/heads/main	[remote rejected] (pre-receive hook declined)
+		> !	refs/heads/next:refs/heads/next	[remote rejected] (pre-receive hook declined)
 		> Done
 		EOF
 		test_cmp expect actual &&
@@ -264,7 +305,6 @@ run_git_push_porcelain_output_test() {
 		<COMMIT-A> refs/heads/baz
 		<COMMIT-A> refs/heads/foo
 		<COMMIT-B> refs/heads/main
-		<COMMIT-A> refs/heads/next
 		EOF
 		test_cmp expect actual
 	'
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v3 5/8] t5548: add porcelain push test cases for dry-run mode

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

New dry-run test cases:

 - git push --porcelain --dry-run
 - git push --porcelain --dry-run --force
 - git push --porcelain --dry-run --atomic
 - git push --porcelain --dry-run --atomic --force

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5548-push-porcelain.sh | 153 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index 799f6066a3..ba68808459 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -310,10 +310,159 @@ run_git_push_porcelain_output_test() {
 	'
 }
 
+run_git_push_dry_run_porcelain_output_test() {
+	case $1 in
+	http)
+		PROTOCOL="HTTP protocol"
+		URL_PREFIX="http://.*"
+		;;
+	file)
+		PROTOCOL="builtin protocol"
+		URL_PREFIX="/.*"
+		;;
+	esac
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run ($PROTOCOL)" '
+		test_must_fail git -C workbench push --porcelain --dry-run origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# push             : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --dry-run --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --atomic ($PROTOCOL)" '
+		test_must_fail git -C workbench push --porcelain --dry-run --atomic origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[rejected] (atomic push failed)
+		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# push             : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --atomic --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --dry-run --atomic --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+}
+
 setup_upstream_and_workbench upstream.git
 
 run_git_push_porcelain_output_test file
 
+setup_upstream_and_workbench upstream.git
+
+run_git_push_dry_run_porcelain_output_test file
+
 ROOT_PATH="$PWD"
 . "$TEST_DIRECTORY"/lib-gpg.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
@@ -325,4 +474,8 @@ setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
 
 run_git_push_porcelain_output_test http
 
+setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
+
+run_git_push_dry_run_porcelain_output_test http
+
 test_done
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v3 6/8] send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The "push_refs" function in the transport_vtable is the handler for
git-push operation. All the "push_refs" functions for different
transports (protocols) should have the same behavior, but the behavior
of "git_transport_push()" function for builtin_smart_vtable in
"transport.c" (which calls "send_pack()" in "send-pack.c") differs from
the handler of the HTTP protocol.

The "push_refs()" function for the HTTP protocol which calls the
"push_refs_with_push()" function in "transport-helper.c" will return 0
even when a bad REF_STATUS (such as REF_STATUS_REJECT_NONFASTFORWARD)
was found. But "send_pack()" for Git smart protocol will return -1 for
a bad REF_STATUS.

We cannot ignore bad REF_STATUS directly in the "send_pack()" function,
because the function is also used in "builtin/send-pack.c". So we add a
new non-zero error code "SEND_PACK_ERROR_REF_STATUS" for "send_pack()".
We can choose to ignore the specific error code in the
"git_transport_push()" function to have the same behavior as
"push_refs()" for HTTP protocol.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 send-pack.c | 9 ++-------
 send-pack.h | 3 +++
 transport.c | 7 +++++++
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 6677c44e8a..f1556dd53c 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,
 				reject_atomic_push(remote_refs, args->send_mirror);
 				error("atomic push failed for ref %s. status: %d",
 				      ref->name, ref->status);
-				ret = args->porcelain ? 0 : -1;
+				ret = ERROR_SEND_PACK_BAD_REF_STATUS;
 				goto out;
 			}
 			/* else fallthrough */
@@ -761,11 +761,6 @@ int send_pack(struct send_pack_args *args,
 	if (ret < 0)
 		goto out;
 
-	if (args->porcelain) {
-		ret = 0;
-		goto out;
-	}
-
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:
@@ -773,7 +768,7 @@ int send_pack(struct send_pack_args *args,
 		case REF_STATUS_OK:
 			break;
 		default:
-			ret = -1;
+			ret = ERROR_SEND_PACK_BAD_REF_STATUS;
 			goto out;
 		}
 	}
diff --git a/send-pack.h b/send-pack.h
index 7edb80596c..ee88f9fe9f 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -12,6 +12,9 @@ struct ref;
 #define SEND_PACK_PUSH_CERT_IF_ASKED 1
 #define SEND_PACK_PUSH_CERT_ALWAYS 2
 
+/* Custom exit code from send_pack. */
+#define ERROR_SEND_PACK_BAD_REF_STATUS 1
+
 struct send_pack_args {
 	const char *url;
 	unsigned verbose:1,
diff --git a/transport.c b/transport.c
index 47fda6a773..454d7f21a9 100644
--- a/transport.c
+++ b/transport.c
@@ -914,6 +914,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	case protocol_v0:
 		ret = send_pack(&args, data->fd, data->conn, remote_refs,
 				&data->extra_have);
+		/*
+		 * Ignore the specific error code to maintain consistent behavior
+		 * with the "push_refs()" function across different transports,
+		 * such as "push_refs_with_push()" for HTTP protocol.
+		 */
+		if (ret == ERROR_SEND_PACK_BAD_REF_STATUS)
+			ret = 0;
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v3 7/8] t5543: atomic push reports exit code failure

[Jiang Xin]

From: Patrick Steinhardt <ps@pks.im>

Add new test cases in t5543 to avoid ignoring the exit code of
git-receive-pack(1) during atomic push with "--porcelain" flag.

We'd typically notice this case because the refs would have their error
message set. But there is an edge case when pushing refs succeeds, but
git-receive-pack(1) exits with a non-zero exit code at a later point in
time due to another error. An atomic git-push(1) would ignore that error
code, and consequently it would return successfully and not print any
error message at all.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 04b47ad84a..32181b9afb 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -280,4 +280,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'atomic push reports exit code failure' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict 2>err &&
+	cat >expect <<-EOF &&
+	To ../upstream
+	 * [new branch]      HEAD -> no-conflict
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
+test_expect_failure 'atomic push reports exit code failure with porcelain' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic --porcelain \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict-porcelain 2>err &&
+	cat >expect <<-EOF &&
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
 test_done
-- 
2.47.0.rc1.21.g81e7bd6151

[PATCH v3 8/8] send-pack: gracefully close the connection for atomic push

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Patrick reported an issue that the exit code of git-receive-pack(1) is
ignored during atomic push with "--porcelain" flag, and added new test
cases in t5543.

This issue originated from commit 7dcbeaa0df (send-pack: fix
inconsistent porcelain output, 2020-04-17). At that time, I chose to
ignore the exit code of "finish_connect()" without investigating the
root cause of the abnormal termination of git-receive-pack. That was an
incorrect solution.

The root cause is that an atomic push operation terminates early without
sending a flush packet to git-receive-pack. As a result,
git-receive-pack continues waiting for commands without exiting. By
sending a flush packet at the appropriate location in "send_pack()", we
ensure that the git-receive-pack process closes properly, avoiding an
erroneous exit code for git-push. At the same time, revert the changes
to the "transport.c" file made in commit 7dcbeaa0df.

Reported-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 send-pack.c            |  1 +
 t/t5543-atomic-push.sh |  4 ++--
 transport.c            | 10 +---------
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index f1556dd53c..439b249c79 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -631,6 +631,7 @@ int send_pack(struct send_pack_args *args,
 				error("atomic push failed for ref %s. status: %d",
 				      ref->name, ref->status);
 				ret = ERROR_SEND_PACK_BAD_REF_STATUS;
+				packet_flush(out);
 				goto out;
 			}
 			/* else fallthrough */
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 32181b9afb..3a700b0676 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -280,7 +280,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'atomic push reports exit code failure' '
+test_expect_success 'atomic push reports exit code failure' '
 	write_script receive-pack-wrapper <<-\EOF &&
 	git-receive-pack "$@"
 	exit 1
@@ -296,7 +296,7 @@ test_expect_failure 'atomic push reports exit code failure' '
 	test_cmp expect err
 '
 
-test_expect_failure 'atomic push reports exit code failure with porcelain' '
+test_expect_success 'atomic push reports exit code failure with porcelain' '
 	write_script receive-pack-wrapper <<-\EOF &&
 	git-receive-pack "$@"
 	exit 1
diff --git a/transport.c b/transport.c
index 454d7f21a9..afcbca293b 100644
--- a/transport.c
+++ b/transport.c
@@ -928,15 +928,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 
 	close(data->fd[1]);
 	close(data->fd[0]);
-	/*
-	 * Atomic push may abort the connection early and close the pipe,
-	 * which may cause an error for `finish_connect()`. Ignore this error
-	 * for atomic git-push.
-	 */
-	if (ret || args.atomic)
-		finish_connect(data->conn);
-	else
-		ret = finish_connect(data->conn);
+	ret |= finish_connect(data->conn);
 	data->conn = NULL;
 	data->finished_handshake = 0;
 
-- 
2.47.0.rc1.21.g81e7bd6151

Re: [PATCH v3 5/8] t5548: add porcelain push test cases for dry-run mode

[Jiang Xin]

On Tue, Dec 10, 2024 at 7:36 PM Jiang Xin <worldhello.net@gmail.com> wrote:
>
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> New dry-run test cases:
>
>  - git push --porcelain --dry-run
>  - git push --porcelain --dry-run --force
>  - git push --porcelain --dry-run --atomic
>  - git push --porcelain --dry-run --atomic --force
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  t/t5548-push-porcelain.sh | 153 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
>
> diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
> index 799f6066a3..ba68808459 100755
> --- a/t/t5548-push-porcelain.sh
> +++ b/t/t5548-push-porcelain.sh
> @@ -310,10 +310,159 @@ run_git_push_porcelain_output_test() {
>         '
>  }
>
> +run_git_push_dry_run_porcelain_output_test() {
> +       case $1 in
> +       http)
> +               PROTOCOL="HTTP protocol"
> +               URL_PREFIX="http://.*"
> +               ;;
> +       file)
> +               PROTOCOL="builtin protocol"
> +               URL_PREFIX="/.*"

This line breaks CI on Windows, see:
https://github.com/jiangxin/git/actions/runs/12255233680/job/34188220514
 Should remove the leading "/". as follows:

1:  93123988ae ! 1:  9e764b6faf t5548: add porcelain push test cases
for dry-run mode
    @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
     +          ;;
     +  file)
     +          PROTOCOL="builtin protocol"
    -+          URL_PREFIX="/.*"
    ++          URL_PREFIX=".*"
     +          ;;
     +  esac
     +

Re: [PATCH v3 6/8] send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"

[Patrick Steinhardt]

On Tue, Dec 10, 2024 at 07:36:26PM +0800, Jiang Xin wrote:
> diff --git a/send-pack.c b/send-pack.c
> index 6677c44e8a..f1556dd53c 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,
>  				reject_atomic_push(remote_refs, args->send_mirror);
>  				error("atomic push failed for ref %s. status: %d",
>  				      ref->name, ref->status);
> -				ret = args->porcelain ? 0 : -1;
> +				ret = ERROR_SEND_PACK_BAD_REF_STATUS;
>  				goto out;
>  			}
>  			/* else fallthrough */

Okay, this change looks sensible -- instead of having different
behaviour with `--porcelain` we now have the exact same behaviour with
and without that flag set, which is good.

> @@ -761,11 +761,6 @@ int send_pack(struct send_pack_args *args,
>  	if (ret < 0)
>  		goto out;
>  
> -	if (args->porcelain) {
> -		ret = 0;
> -		goto out;
> -	}

We don't reset the error code here anymore.

>  	for (ref = remote_refs; ref; ref = ref->next) {
>  		switch (ref->status) {
>  		case REF_STATUS_NONE:
> @@ -773,7 +768,7 @@ int send_pack(struct send_pack_args *args,
>  		case REF_STATUS_OK:
>  			break;
>  		default:
> -			ret = -1;
> +			ret = ERROR_SEND_PACK_BAD_REF_STATUS;
>  			goto out;
>  		}
>  	}

And this is the equivalent change without `--atomic`.

Also, none of the existing code paths used to set `ret = 1`. We do call
`ret = receive_status()` and bubble that up, but neither that nor any of
its transitive calls set `ret = 1`, either. So this should be fine and
we don't seem to have conflicting error codes.

> diff --git a/send-pack.h b/send-pack.h
> index 7edb80596c..ee88f9fe9f 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -12,6 +12,9 @@ struct ref;
>  #define SEND_PACK_PUSH_CERT_IF_ASKED 1
>  #define SEND_PACK_PUSH_CERT_ALWAYS 2
>  
> +/* Custom exit code from send_pack. */
> +#define ERROR_SEND_PACK_BAD_REF_STATUS 1
> +
>  struct send_pack_args {
>  	const char *url;
>  	unsigned verbose:1,

It might help to document `send_pack()` accordingly and point out the
special return code.

> diff --git a/transport.c b/transport.c
> index 47fda6a773..454d7f21a9 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -914,6 +914,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
>  	case protocol_v0:
>  		ret = send_pack(&args, data->fd, data->conn, remote_refs,
>  				&data->extra_have);
> +		/*
> +		 * Ignore the specific error code to maintain consistent behavior
> +		 * with the "push_refs()" function across different transports,
> +		 * such as "push_refs_with_push()" for HTTP protocol.
> +		 */
> +		if (ret == ERROR_SEND_PACK_BAD_REF_STATUS)
> +			ret = 0;
>  		break;
>  	case protocol_unknown_version:
>  		BUG("unknown protocol version");

And here we ignore the error code to retain compatibility.

Is there any case where this causes a user-visible change in behaviour
already? If so, we can add a test for this?

Patrick

Re: [PATCH v3 8/8] send-pack: gracefully close the connection for atomic push

[Patrick Steinhardt]

On Tue, Dec 10, 2024 at 07:36:28PM +0800, Jiang Xin wrote:
> diff --git a/send-pack.c b/send-pack.c
> index f1556dd53c..439b249c79 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -631,6 +631,7 @@ int send_pack(struct send_pack_args *args,
>  				error("atomic push failed for ref %s. status: %d",
>  				      ref->name, ref->status);
>  				ret = ERROR_SEND_PACK_BAD_REF_STATUS;
> +				packet_flush(out);
>  				goto out;
>  			}
>  			/* else fallthrough */

Nice and easy fix.

Patrick

[PATCH v4 0/8] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Patrick Steinhardt]

Hi,

we've hit an edge case at GitLab where an atomic push will not notice an
error when git-receive-pack(1) updates the refs, but otherwise fails
with a non-zero exit code. The push would be successful and no error
would be printed even though some things have gone wrong on the remote
side.

As promised last week, I've now adopted the patch series from Jiang Xin
to make some progress on the issue. The series is now based on the
latest master branch at 3b0d05c4a7 (The fifth batch, 2025-01-29).

Changes in v4:
  - Rewrite the commit that adds two new tests for `--porcelain`.
    Previously, the commit both reordered existing tests and added new
    ones, which made it hard to see what actually changed. The reorder
    wasn't necessary though, so I've adapted it to only add new tests
    now.
  - Fix the URL prefix in t5548 to also work on Windows.
  - Document why it's fine to start ignoring the return value of
    `git_transport_push()`.
  - Improve comments to document behaviour better.
  - Link to v3: https://lore.kernel.org/r/cover.1733830410.git.zhiyou.jx@alibaba-inc.com

Thanks!

Patrick

---
Jiang Xin (5):
      t5548: refactor to reuse setup_upstream() function
      t5548: refactor test cases by resetting upstream
      t5548: add porcelain push test cases for dry-run mode
      send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"
      send-pack: gracefully close the connection for atomic push

Patrick Steinhardt (3):
      t5504: modernize test by moving heredocs into test bodies
      t5548: add new porcelain test cases
      t5543: atomic push reports exit code failure

 send-pack.c                     |  10 +-
 send-pack.h                     |  13 ++
 t/t5504-fetch-receive-strict.sh |  35 ++--
 t/t5543-atomic-push.sh          |  30 +++
 t/t5548-push-porcelain.sh       | 443 ++++++++++++++++++++++++++++++----------
 transport.c                     |  17 +-
 6 files changed, 407 insertions(+), 141 deletions(-)

Range-diff versus v3:

1:  5001da5515 = 1:  5a68112d63 t5504: modernize test by moving heredocs into test bodies
2:  de1b7eab6a ! 2:  bb3b9cd8b7 t5548: refactor to reuse setup_upstream() function
    @@ Commit message
         setup_upstream_and_workbench() functions.
     
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
    +    Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t5548-push-porcelain.sh ##
     @@ t/t5548-push-porcelain.sh: format_and_save_expect () {
3:  be1daa20cd ! 3:  7268ab4fdf t5548: refactor test cases by resetting upstream
    @@ Commit message
            hook by moving the operations into the corresponding test case,
     
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
    +    Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t5548-push-porcelain.sh ##
     @@ t/t5548-push-porcelain.sh: setup_upstream_and_workbench () {
4:  d353cc13d0 < -:  ---------- t5548: add new porcelain test cases
-:  ---------- > 4:  1b721338eb t5548: add new porcelain test cases
5:  2f4723d34f ! 5:  7be03b6fdd t5548: add porcelain push test cases for dry-run mode
    @@ Commit message
          - git push --porcelain --dry-run --atomic --force
     
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
    +    Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t5548-push-porcelain.sh ##
     @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
    @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
     +		;;
     +	file)
     +		PROTOCOL="builtin protocol"
    -+		URL_PREFIX="/.*"
    ++		URL_PREFIX=".*"
     +		;;
     +	esac
     +
6:  73728ec873 ! 6:  ea0e885613 send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"
    @@ Commit message
         We cannot ignore bad REF_STATUS directly in the "send_pack()" function,
         because the function is also used in "builtin/send-pack.c". So we add a
         new non-zero error code "SEND_PACK_ERROR_REF_STATUS" for "send_pack()".
    -    We can choose to ignore the specific error code in the
    -    "git_transport_push()" function to have the same behavior as
    -    "push_refs()" for HTTP protocol.
    +
    +    Ignore the specific error code in the "git_transport_push()" function to
    +    have the same behavior as "push_refs()" for HTTP protocol. Note that
    +    even though we ignore the error here, we'll ultimately still end up
    +    detecting that a subset of refs was not pushed in `transport_push()`
    +    because we eventually call `push_had_errors()` on the remote refs.
     
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
    +    Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## send-pack.c ##
    -@@ send-pack.c: int send_pack(struct send_pack_args *args,
    +@@ send-pack.c: int send_pack(struct repository *r,
      				reject_atomic_push(remote_refs, args->send_mirror);
      				error("atomic push failed for ref %s. status: %d",
      				      ref->name, ref->status);
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      				goto out;
      			}
      			/* else fallthrough */
    -@@ send-pack.c: int send_pack(struct send_pack_args *args,
    +@@ send-pack.c: int send_pack(struct repository *r,
      	if (ret < 0)
      		goto out;
      
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      	for (ref = remote_refs; ref; ref = ref->next) {
      		switch (ref->status) {
      		case REF_STATUS_NONE:
    -@@ send-pack.c: int send_pack(struct send_pack_args *args,
    +@@ send-pack.c: int send_pack(struct repository *r,
      		case REF_STATUS_OK:
      			break;
      		default:
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      	}
     
      ## send-pack.h ##
    -@@ send-pack.h: struct ref;
    +@@ send-pack.h: struct repository;
      #define SEND_PACK_PUSH_CERT_IF_ASKED 1
      #define SEND_PACK_PUSH_CERT_ALWAYS 2
      
    -+/* Custom exit code from send_pack. */
    ++/* At least one reference has been rejected by the remote side. */
     +#define ERROR_SEND_PACK_BAD_REF_STATUS 1
     +
      struct send_pack_args {
      	const char *url;
      	unsigned verbose:1,
    +@@ send-pack.h: struct option;
    + int option_parse_push_signed(const struct option *opt,
    + 			     const char *arg, int unset);
    + 
    ++/*
    ++ * Compute a packfile and write it to a file descriptor. The `fd` array needs
    ++ * to contain two file descriptors: `fd[0]` is the file descriptor used as
    ++ * input for the packet reader, whereas `fd[1]` is the file descriptor the
    ++ * packfile will be written to.
    ++ *
    ++ * Returns 0 on success, non-zero otherwise. Negative return values indicate a
    ++ * generic error, whereas positive return values indicate specific error
    ++ * conditions as documented with the `ERROR_SEND_PACK_*` constants.
    ++ */
    + int send_pack(struct repository *r, struct send_pack_args *args,
    + 	      int fd[], struct child_process *conn,
    + 	      struct ref *remote_refs, struct oid_array *extra_have);
     
      ## transport.c ##
     @@ transport.c: static int git_transport_push(struct transport *transport, struct ref *remote_re
      	case protocol_v0:
    - 		ret = send_pack(&args, data->fd, data->conn, remote_refs,
    + 		ret = send_pack(the_repository, &args, data->fd, data->conn, remote_refs,
      				&data->extra_have);
     +		/*
     +		 * Ignore the specific error code to maintain consistent behavior
7:  b26774fee4 = 7:  6930d7ec2b t5543: atomic push reports exit code failure
8:  ab4d0906f6 ! 8:  c2c82673fd send-pack: gracefully close the connection for atomic push
    @@ Commit message
     
         Reported-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
    +    Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## send-pack.c ##
    -@@ send-pack.c: int send_pack(struct send_pack_args *args,
    +@@ send-pack.c: int send_pack(struct repository *r,
      				error("atomic push failed for ref %s. status: %d",
      				      ref->name, ref->status);
      				ret = ERROR_SEND_PACK_BAD_REF_STATUS;

---
base-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08
change-id: 20241113-pks-push-atomic-respect-exit-code-436c443a657d

[PATCH v4 1/8] t5504: modernize test by moving heredocs into test bodies

[Patrick Steinhardt]

We have several heredocs in t5504 located outside of any particular test
bodies. Move these into the test bodies to match our modern coding
style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5504-fetch-receive-strict.sh | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index e273ab29c7..29f18841c3 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 	)
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
-Done
-EOF
-
 test_expect_success 'push without strict' '
 	rm -rf dst &&
 	git init dst &&
@@ -78,6 +72,11 @@ test_expect_success 'push without strict' '
 		git config fetch.fsckobjects false &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
+	Done
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '
 	test_cmp exp act
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
-EOF
-
 test_expect_success 'push with receive.fsckobjects' '
 	rm -rf dst &&
 	git init dst &&
@@ -107,6 +101,10 @@ test_expect_success 'push with receive.fsckobjects' '
 		git config receive.fsckobjects true &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -129,15 +127,14 @@ test_expect_success 'repair the "corrupt or missing" object' '
 	git fsck
 '
 
-cat >bogus-commit <<EOF
-tree $EMPTY_TREE
-author Bugs Bunny 1234567890 +0000
-committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
-
-This commit object intentionally broken
-EOF
-
 test_expect_success 'setup bogus commit' '
+	cat >bogus-commit <<-EOF &&
+	tree $EMPTY_TREE
+	author Bugs Bunny 1234567890 +0000
+	committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
+
+	This commit object intentionally broken
+	EOF
 	commit="$(git hash-object --literally -t commit -w --stdin <bogus-commit)"
 '
 

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v4 2/8] t5548: refactor to reuse setup_upstream() function

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor the function setup_upstream_and_workbench(), extracting
create_upstream_template() and setup_upstream() from it. The former is
used to create the upstream repository template, while the latter is
used to rebuild the upstream repository and will be reused in subsequent
commits.

To ensure that setup_upstream() works properly in both local and HTTP
protocols, the HTTP settings have been moved to the setup_upstream() and
setup_upstream_and_workbench() functions.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5548-push-porcelain.sh | 85 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 30 deletions(-)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index 6282728eaf..a3defd5b75 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -54,29 +54,67 @@ format_and_save_expect () {
 	sed -e 's/^> //' -e 's/Z$//' >expect
 }
 
+create_upstream_template () {
+	git init --bare upstream-template.git &&
+	git clone upstream-template.git tmp_work_dir &&
+	create_commits_in tmp_work_dir A B &&
+	(
+		cd tmp_work_dir &&
+		git push origin \
+			$B:refs/heads/main \
+			$A:refs/heads/foo \
+			$A:refs/heads/bar \
+			$A:refs/heads/baz
+	) &&
+	rm -rf tmp_work_dir
+}
+
+setup_upstream () {
+	if test $# -ne 1
+	then
+		BUG "location of upstream repository is not provided"
+	fi &&
+	rm -rf "$1" &&
+	if ! test -d upstream-template.git
+	then
+		create_upstream_template
+	fi &&
+	git clone --mirror upstream-template.git "$1" &&
+	# The upstream repository provides services using the HTTP protocol.
+	if ! test "$1" = "upstream.git"
+	then
+		git -C "$1" config http.receivepack true
+	fi
+}
+
 setup_upstream_and_workbench () {
+	if test $# -ne 1
+	then
+		BUG "location of upstream repository is not provided"
+	fi &&
+	# Assign the first argument to the variable upstream;
+	# we will use it in the subsequent test cases.
+	upstream="$1"
+
 	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
 	# Workbench after setup : main(A)
 	test_expect_success "setup upstream repository and workbench" '
-		rm -rf upstream.git workbench &&
-		git init --bare upstream.git &&
-		git init workbench &&
-		create_commits_in workbench A B &&
+		setup_upstream "$upstream" &&
+		rm -rf workbench &&
+		git clone "$upstream" workbench &&
 		(
 			cd workbench &&
+			git update-ref refs/heads/main $A &&
 			# Try to make a stable fixed width for abbreviated commit ID,
 			# this fixed-width oid will be replaced with "<OID>".
 			git config core.abbrev 7 &&
-			git remote add origin ../upstream.git &&
-			git update-ref refs/heads/main $A &&
-			git push origin \
-				$B:refs/heads/main \
-				$A:refs/heads/foo \
-				$A:refs/heads/bar \
-				$A:refs/heads/baz
+			git config advice.pushUpdateRejected false
 		) &&
-		git -C "workbench" config advice.pushUpdateRejected false &&
-		upstream=upstream.git
+		# The upstream repository provides services using the HTTP protocol.
+		if ! test "$upstream" = "upstream.git"
+		then
+			git -C workbench remote set-url origin "$HTTPD_URL/smart/upstream.git"
+		fi
 	'
 }
 
@@ -88,7 +126,7 @@ run_git_push_porcelain_output_test() {
 		;;
 	file)
 		PROTOCOL="builtin protocol"
-		URL_PREFIX="\.\."
+		URL_PREFIX=".*"
 		;;
 	esac
 
@@ -247,10 +285,8 @@ run_git_push_porcelain_output_test() {
 	'
 }
 
-# Initialize the upstream repository and local workbench.
-setup_upstream_and_workbench
+setup_upstream_and_workbench upstream.git
 
-# Run git-push porcelain test on builtin protocol
 run_git_push_porcelain_output_test file
 
 ROOT_PATH="$PWD"
@@ -258,21 +294,10 @@ ROOT_PATH="$PWD"
 . "$TEST_DIRECTORY"/lib-httpd.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 start_httpd
-
-# Re-initialize the upstream repository and local workbench.
-setup_upstream_and_workbench
-
-test_expect_success "setup for http" '
-	git -C upstream.git config http.receivepack true &&
-	upstream="$HTTPD_DOCUMENT_ROOT_PATH/upstream.git" &&
-	mv upstream.git "$upstream" &&
-
-	git -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git
-'
-
 setup_askpass_helper
 
-# Run git-push porcelain test on HTTP protocol
+setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
+
 run_git_push_porcelain_output_test http
 
 test_done

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v4 3/8] t5548: refactor test cases by resetting upstream

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor the test cases with the following changes:

 - Calling setup_upstream() to reset upstream after running each test
   case.

 - Change the initial branch tips of the workspace to reduce the branch
   setup operations in the workspace.

 - Reduced the two steps of setting up and cleaning up the pre-receive
   hook by moving the operations into the corresponding test case,

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5548-push-porcelain.sh | 149 +++++++++++++++++++++-------------------------
 1 file changed, 67 insertions(+), 82 deletions(-)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index a3defd5b75..ededd8edb9 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -96,8 +96,8 @@ setup_upstream_and_workbench () {
 	# we will use it in the subsequent test cases.
 	upstream="$1"
 
-	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
-	# Workbench after setup : main(A)
+	# Upstream  after setup: main(B)  foo(A)  bar(A)  baz(A)
+	# Workbench after setup: main(A)                  baz(A)  next(A)
 	test_expect_success "setup upstream repository and workbench" '
 		setup_upstream "$upstream" &&
 		rm -rf workbench &&
@@ -105,6 +105,8 @@ setup_upstream_and_workbench () {
 		(
 			cd workbench &&
 			git update-ref refs/heads/main $A &&
+			git update-ref refs/heads/baz $A &&
+			git update-ref refs/heads/next $A &&
 			# Try to make a stable fixed width for abbreviated commit ID,
 			# this fixed-width oid will be replaced with "<OID>".
 			git config core.abbrev 7 &&
@@ -133,19 +135,14 @@ run_git_push_porcelain_output_test() {
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
-	test_expect_success "porcelain output of successful git-push ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $A &&
-			git update-ref refs/heads/baz $A &&
-			git update-ref refs/heads/next $A &&
-			git push --porcelain --force origin \
-				main \
-				:refs/heads/foo \
-				$B:bar \
-				baz \
-				next
-		) >out &&
+	test_expect_success ".. git-push --porcelain --force ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
 		> To <URL/of/upstream.git>
@@ -169,115 +166,103 @@ run_git_push_porcelain_output_test() {
 		test_cmp expect actual
 	'
 
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "atomic push failed ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $B &&
-			git update-ref refs/heads/bar $A &&
-			test_must_fail git push --atomic --porcelain origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git push --porcelain --atomic ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		test_must_fail git -C workbench push --porcelain --atomic origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> !	refs/heads/bar:refs/heads/bar	[rejected] (non-fast-forward)
-		> !	(delete):refs/heads/baz	[rejected] (atomic push failed)
-		> !	refs/heads/main:refs/heads/main	[rejected] (atomic push failed)
-		Done
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[rejected] (atomic push failed)
+		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
 		<COMMIT-A> refs/heads/baz
-		<COMMIT-A> refs/heads/main
-		<COMMIT-A> refs/heads/next
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
 		EOF
 		test_cmp expect actual
 	'
 
-	test_expect_success "prepare pre-receive hook ($PROTOCOL)" '
-		test_hook --setup -C "$upstream" pre-receive <<-EOF
-		exit 1
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. pre-receive hook declined ($PROTOCOL)" '
+		test_when_finished "rm -f \"$upstream/hooks/pre-receive\" &&
+			setup_upstream \"$upstream\"" &&
+		test_hook --setup -C "$upstream" pre-receive <<-EOF &&
+			exit 1
 		EOF
-	'
-
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "pre-receive hook declined ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $B &&
-			git update-ref refs/heads/bar $A &&
-			test_must_fail git push --porcelain --force origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+		test_must_fail git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> !	refs/heads/bar:refs/heads/bar	[remote rejected] (pre-receive hook declined)
-		> !	:refs/heads/baz	[remote rejected] (pre-receive hook declined)
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[remote rejected] (pre-receive hook declined)
+		> !	:refs/heads/foo	[remote rejected] (pre-receive hook declined)
 		> !	refs/heads/main:refs/heads/main	[remote rejected] (pre-receive hook declined)
-		Done
+		> !	refs/heads/next:refs/heads/next	[remote rejected] (pre-receive hook declined)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
 		<COMMIT-A> refs/heads/baz
-		<COMMIT-A> refs/heads/main
-		<COMMIT-A> refs/heads/next
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
 		EOF
 		test_cmp expect actual
 	'
 
-	test_expect_success "remove pre-receive hook ($PROTOCOL)" '
-		rm "$upstream/hooks/pre-receive"
-	'
-
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "non-fastforward push ($PROTOCOL)" '
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)                          next(A)
+	test_expect_success ".. non-fastforward push ($PROTOCOL)" '
 		(
 			cd workbench &&
 			test_must_fail git push --porcelain origin \
 				main \
-				bar \
-				:baz \
 				next
 		) >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> -	:refs/heads/baz	[deleted]
-		>  	refs/heads/main:refs/heads/main	<COMMIT-A>..<COMMIT-B>
-		> !	refs/heads/bar:refs/heads/bar	[rejected] (non-fast-forward)
-		Done
+		> To <URL/of/upstream.git>
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
 		<COMMIT-B> refs/heads/main
 		<COMMIT-A> refs/heads/next
 		EOF

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v4 4/8] t5548: add new porcelain test cases

[Patrick Steinhardt]

Add two more test cases exercising git-push(1) with `--procelain`, one
exercising a non-atomic and one exercising an atomic push.

Based-on-patch-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5548-push-porcelain.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index ededd8edb9..b510a2e6bb 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -132,6 +132,40 @@ run_git_push_porcelain_output_test() {
 		;;
 	esac
 
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		test_must_fail git -C workbench push --porcelain origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-B> refs/heads/main
+		<COMMIT-A> refs/heads/next
+		EOF
+		test_cmp expect actual
+	'
+
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
@@ -242,6 +276,7 @@ run_git_push_porcelain_output_test() {
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)                          next(A)
 	test_expect_success ".. non-fastforward push ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
 		(
 			cd workbench &&
 			test_must_fail git push --porcelain origin \
@@ -268,6 +303,39 @@ run_git_push_porcelain_output_test() {
 		EOF
 		test_cmp expect actual
 	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git push --porcelain --atomic --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --atomic --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/main
+		<COMMIT-A> refs/heads/next
+		EOF
+		test_cmp expect actual
+	'
 }
 
 setup_upstream_and_workbench upstream.git

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v4 5/8] t5548: add porcelain push test cases for dry-run mode

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

New dry-run test cases:

 - git push --porcelain --dry-run
 - git push --porcelain --dry-run --force
 - git push --porcelain --dry-run --atomic
 - git push --porcelain --dry-run --atomic --force

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5548-push-porcelain.sh | 153 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index b510a2e6bb..c20e4ddc01 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -338,10 +338,159 @@ run_git_push_porcelain_output_test() {
 	'
 }
 
+run_git_push_dry_run_porcelain_output_test() {
+	case $1 in
+	http)
+		PROTOCOL="HTTP protocol"
+		URL_PREFIX="http://.*"
+		;;
+	file)
+		PROTOCOL="builtin protocol"
+		URL_PREFIX=".*"
+		;;
+	esac
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run ($PROTOCOL)" '
+		test_must_fail git -C workbench push --porcelain --dry-run origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# push             : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --dry-run --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --atomic ($PROTOCOL)" '
+		test_must_fail git -C workbench push --porcelain --dry-run --atomic origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[rejected] (atomic push failed)
+		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# push             : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --atomic --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --dry-run --atomic --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+}
+
 setup_upstream_and_workbench upstream.git
 
 run_git_push_porcelain_output_test file
 
+setup_upstream_and_workbench upstream.git
+
+run_git_push_dry_run_porcelain_output_test file
+
 ROOT_PATH="$PWD"
 . "$TEST_DIRECTORY"/lib-gpg.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
@@ -353,4 +502,8 @@ setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
 
 run_git_push_porcelain_output_test http
 
+setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
+
+run_git_push_dry_run_porcelain_output_test http
+
 test_done

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v4 6/8] send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The "push_refs" function in the transport_vtable is the handler for
git-push operation. All the "push_refs" functions for different
transports (protocols) should have the same behavior, but the behavior
of "git_transport_push()" function for builtin_smart_vtable in
"transport.c" (which calls "send_pack()" in "send-pack.c") differs from
the handler of the HTTP protocol.

The "push_refs()" function for the HTTP protocol which calls the
"push_refs_with_push()" function in "transport-helper.c" will return 0
even when a bad REF_STATUS (such as REF_STATUS_REJECT_NONFASTFORWARD)
was found. But "send_pack()" for Git smart protocol will return -1 for
a bad REF_STATUS.

We cannot ignore bad REF_STATUS directly in the "send_pack()" function,
because the function is also used in "builtin/send-pack.c". So we add a
new non-zero error code "SEND_PACK_ERROR_REF_STATUS" for "send_pack()".

Ignore the specific error code in the "git_transport_push()" function to
have the same behavior as "push_refs()" for HTTP protocol. Note that
even though we ignore the error here, we'll ultimately still end up
detecting that a subset of refs was not pushed in `transport_push()`
because we eventually call `push_had_errors()` on the remote refs.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c |  9 ++-------
 send-pack.h | 13 +++++++++++++
 transport.c |  7 +++++++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 772c7683a0..4448c081cc 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -632,7 +632,7 @@ int send_pack(struct repository *r,
 				reject_atomic_push(remote_refs, args->send_mirror);
 				error("atomic push failed for ref %s. status: %d",
 				      ref->name, ref->status);
-				ret = args->porcelain ? 0 : -1;
+				ret = ERROR_SEND_PACK_BAD_REF_STATUS;
 				goto out;
 			}
 			/* else fallthrough */
@@ -763,11 +763,6 @@ int send_pack(struct repository *r,
 	if (ret < 0)
 		goto out;
 
-	if (args->porcelain) {
-		ret = 0;
-		goto out;
-	}
-
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:
@@ -775,7 +770,7 @@ int send_pack(struct repository *r,
 		case REF_STATUS_OK:
 			break;
 		default:
-			ret = -1;
+			ret = ERROR_SEND_PACK_BAD_REF_STATUS;
 			goto out;
 		}
 	}
diff --git a/send-pack.h b/send-pack.h
index d256715681..c5ded2d200 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -13,6 +13,9 @@ struct repository;
 #define SEND_PACK_PUSH_CERT_IF_ASKED 1
 #define SEND_PACK_PUSH_CERT_ALWAYS 2
 
+/* At least one reference has been rejected by the remote side. */
+#define ERROR_SEND_PACK_BAD_REF_STATUS 1
+
 struct send_pack_args {
 	const char *url;
 	unsigned verbose:1,
@@ -36,6 +39,16 @@ struct option;
 int option_parse_push_signed(const struct option *opt,
 			     const char *arg, int unset);
 
+/*
+ * Compute a packfile and write it to a file descriptor. The `fd` array needs
+ * to contain two file descriptors: `fd[0]` is the file descriptor used as
+ * input for the packet reader, whereas `fd[1]` is the file descriptor the
+ * packfile will be written to.
+ *
+ * Returns 0 on success, non-zero otherwise. Negative return values indicate a
+ * generic error, whereas positive return values indicate specific error
+ * conditions as documented with the `ERROR_SEND_PACK_*` constants.
+ */
 int send_pack(struct repository *r, struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs, struct oid_array *extra_have);
diff --git a/transport.c b/transport.c
index 81ae8243b9..d064aff33e 100644
--- a/transport.c
+++ b/transport.c
@@ -934,6 +934,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	case protocol_v0:
 		ret = send_pack(the_repository, &args, data->fd, data->conn, remote_refs,
 				&data->extra_have);
+		/*
+		 * Ignore the specific error code to maintain consistent behavior
+		 * with the "push_refs()" function across different transports,
+		 * such as "push_refs_with_push()" for HTTP protocol.
+		 */
+		if (ret == ERROR_SEND_PACK_BAD_REF_STATUS)
+			ret = 0;
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v4 7/8] t5543: atomic push reports exit code failure

[Patrick Steinhardt]

Add new test cases in t5543 to avoid ignoring the exit code of
git-receive-pack(1) during atomic push with "--porcelain" flag.

We'd typically notice this case because the refs would have their error
message set. But there is an edge case when pushing refs succeeds, but
git-receive-pack(1) exits with a non-zero exit code at a later point in
time due to another error. An atomic git-push(1) would ignore that error
code, and consequently it would return successfully and not print any
error message at all.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 04b47ad84a..32181b9afb 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -280,4 +280,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'atomic push reports exit code failure' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict 2>err &&
+	cat >expect <<-EOF &&
+	To ../upstream
+	 * [new branch]      HEAD -> no-conflict
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
+test_expect_failure 'atomic push reports exit code failure with porcelain' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic --porcelain \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict-porcelain 2>err &&
+	cat >expect <<-EOF &&
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
 test_done

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v4 8/8] send-pack: gracefully close the connection for atomic push

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Patrick reported an issue that the exit code of git-receive-pack(1) is
ignored during atomic push with "--porcelain" flag, and added new test
cases in t5543.

This issue originated from commit 7dcbeaa0df (send-pack: fix
inconsistent porcelain output, 2020-04-17). At that time, I chose to
ignore the exit code of "finish_connect()" without investigating the
root cause of the abnormal termination of git-receive-pack. That was an
incorrect solution.

The root cause is that an atomic push operation terminates early without
sending a flush packet to git-receive-pack. As a result,
git-receive-pack continues waiting for commands without exiting. By
sending a flush packet at the appropriate location in "send_pack()", we
ensure that the git-receive-pack process closes properly, avoiding an
erroneous exit code for git-push. At the same time, revert the changes
to the "transport.c" file made in commit 7dcbeaa0df.

Reported-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c            |  1 +
 t/t5543-atomic-push.sh |  4 ++--
 transport.c            | 10 +---------
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 4448c081cc..856a65d5f5 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -633,6 +633,7 @@ int send_pack(struct repository *r,
 				error("atomic push failed for ref %s. status: %d",
 				      ref->name, ref->status);
 				ret = ERROR_SEND_PACK_BAD_REF_STATUS;
+				packet_flush(out);
 				goto out;
 			}
 			/* else fallthrough */
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 32181b9afb..3a700b0676 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -280,7 +280,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'atomic push reports exit code failure' '
+test_expect_success 'atomic push reports exit code failure' '
 	write_script receive-pack-wrapper <<-\EOF &&
 	git-receive-pack "$@"
 	exit 1
@@ -296,7 +296,7 @@ test_expect_failure 'atomic push reports exit code failure' '
 	test_cmp expect err
 '
 
-test_expect_failure 'atomic push reports exit code failure with porcelain' '
+test_expect_success 'atomic push reports exit code failure with porcelain' '
 	write_script receive-pack-wrapper <<-\EOF &&
 	git-receive-pack "$@"
 	exit 1
diff --git a/transport.c b/transport.c
index d064aff33e..b0c6c339f4 100644
--- a/transport.c
+++ b/transport.c
@@ -948,15 +948,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 
 	close(data->fd[1]);
 	close(data->fd[0]);
-	/*
-	 * Atomic push may abort the connection early and close the pipe,
-	 * which may cause an error for `finish_connect()`. Ignore this error
-	 * for atomic git-push.
-	 */
-	if (ret || args.atomic)
-		finish_connect(data->conn);
-	else
-		ret = finish_connect(data->conn);
+	ret |= finish_connect(data->conn);
 	data->conn = NULL;
 	data->finished_handshake = 0;
 

-- 
2.48.1.502.g6dc24dfdaf.dirty

Re: [PATCH v4 1/8] t5504: modernize test by moving heredocs into test bodies

[Eric Sunshine]

On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt <ps@pks.im> wrote:
> We have several heredocs in t5504 located outside of any particular test
> bodies. Move these into the test bodies to match our modern coding
> style.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
> @@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '
> -cat >exp <<EOF
> -To dst
> -!      refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)
> -Done
> -EOF
> -
>  test_expect_success 'push without strict' '
> @@ -78,6 +72,11 @@ test_expect_success 'push without strict' '
> +       cat >exp <<-EOF &&
> +       To dst
> +       !       refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)
> +       Done
> +       EOF

It's minor, but to make this conform to modern style even more, it
would use `\EOF` rather than `EOF`.

(Probably not worth a reroll on its own.)

> @@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '
> -cat >exp <<EOF
> -To dst
> -!      refs/heads/main:refs/heads/test [remote rejected] (unpacker error)
> -EOF
> -
>  test_expect_success 'push with receive.fsckobjects' '
> @@ -107,6 +101,10 @@ test_expect_success 'push with receive.fsckobjects' '
> +       cat >exp <<-EOF &&
> +       To dst
> +       !       refs/heads/main:refs/heads/test [remote rejected] (unpacker error)
> +       EOF

Ditto.

> @@ -129,15 +127,14 @@ test_expect_success 'repair the "corrupt or missing" object' '
> -cat >bogus-commit <<EOF
> -tree $EMPTY_TREE
> -author Bugs Bunny 1234567890 +0000
> -committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
> -
> -This commit object intentionally broken
> -EOF
> -
>  test_expect_success 'setup bogus commit' '
> +       cat >bogus-commit <<-EOF &&
> +       tree $EMPTY_TREE
> +       author Bugs Bunny 1234567890 +0000
> +       committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
> +
> +       This commit object intentionally broken
> +       EOF

This one is correctly using `EOF` since it's interpolating variables.

Re: [PATCH v4 2/8] t5548: refactor to reuse setup_upstream() function

[Eric Sunshine]

On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt <ps@pks.im> wrote:
> Refactor the function setup_upstream_and_workbench(), extracting
> create_upstream_template() and setup_upstream() from it. The former is
> used to create the upstream repository template, while the latter is
> used to rebuild the upstream repository and will be reused in subsequent
> commits.
>
> To ensure that setup_upstream() works properly in both local and HTTP
> protocols, the HTTP settings have been moved to the setup_upstream() and
> setup_upstream_and_workbench() functions.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
> @@ -54,29 +54,67 @@ format_and_save_expect () {
>  setup_upstream_and_workbench () {
> +       if test $# -ne 1
> +       then
> +               BUG "location of upstream repository is not provided"
> +       fi &&

It's not clear why &&-chaining is being used here considering that
this function is not called by any tests and is itself not linked into
any &&-chains, hence the use of && is superfluous and misleading.

> +       # Assign the first argument to the variable upstream;
> +       # we will use it in the subsequent test cases.
> +       upstream="$1"

Comment doesn't seem to add any value.

(Neither of these are worth a reroll on their own.)

Re: [PATCH v4 4/8] t5548: add new porcelain test cases

[Eric Sunshine]

On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt <ps@pks.im> wrote:
> Add two more test cases exercising git-push(1) with `--procelain`, one
> exercising a non-atomic and one exercising an atomic push.
>
> Based-on-patch-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
> @@ -132,6 +132,40 @@ run_git_push_porcelain_output_test() {
> +       # Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
> +       # Refs of workbench: main(A)                  baz(A)  next(A)
> +       # git-push         : main(A)  NULL    (B)     baz(A)  next(A)
> +       test_expect_success ".. git-push --porcelain ($PROTOCOL)" '
> +               test_when_finished "setup_upstream \"$upstream\"" &&
> +               test_must_fail git -C workbench push --porcelain origin \
> +                       main \
> +                       :refs/heads/foo \
> +                       $B:bar \
> +                       baz \
> +                       next >out &&
> +               make_user_friendly_and_stable_output <out >actual &&
> +               format_and_save_expect <<-EOF &&
> +               > To <URL/of/upstream.git>
> +               > =     refs/heads/baz:refs/heads/baz   [up to date]
> +               >       <COMMIT-B>:refs/heads/bar       <COMMIT-A>..<COMMIT-B>
> +               > -     :refs/heads/foo [deleted]
> +               > *     refs/heads/next:refs/heads/next [new branch]
> +               > !     refs/heads/main:refs/heads/main [rejected] (non-fast-forward)
> +               > Done
> +               EOF

Using '\EOF' rather than bare 'EOF' would be appropriate here and in
the other new heredocs added by this patch.

(Not worth a reroll on its own.)

Re: [PATCH v4 1/8] t5504: modernize test by moving heredocs into test bodies

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> @@ -78,6 +72,11 @@ test_expect_success 'push without strict' '
>> +       cat >exp <<-EOF &&
>> +       To dst
>> +       !       refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)
>> +       Done
>> +       EOF
>
> It's minor, but to make this conform to modern style even more, it
> would use `\EOF` rather than `EOF`.
>
> (Probably not worth a reroll on its own.)

Yup.  The rule of thumb is to quote the end-of-heredoc marker when
the here-doc does not need interpolation, to serve as a hint to tell
the readers that the here-doc is a literal text.

Re: [PATCH v4 1/8] t5504: modernize test by moving heredocs into test bodies

[Patrick Steinhardt]

On Fri, Jan 31, 2025 at 09:28:01AM -0500, Eric Sunshine wrote:
> On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt <ps@pks.im> wrote:
> > We have several heredocs in t5504 located outside of any particular test
> > bodies. Move these into the test bodies to match our modern coding
> > style.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
> > @@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '
> > -cat >exp <<EOF
> > -To dst
> > -!      refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)
> > -Done
> > -EOF
> > -
> >  test_expect_success 'push without strict' '
> > @@ -78,6 +72,11 @@ test_expect_success 'push without strict' '
> > +       cat >exp <<-EOF &&
> > +       To dst
> > +       !       refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)
> > +       Done
> > +       EOF
> 
> It's minor, but to make this conform to modern style even more, it
> would use `\EOF` rather than `EOF`.

Yup, indeed. Will fix, thanks for your review!

Patrick

[PATCH v5 0/8] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Patrick Steinhardt]

Hi,

we've hit an edge case at GitLab where an atomic push will not notice an
error when git-receive-pack(1) updates the refs, but otherwise fails
with a non-zero exit code. The push would be successful and no error
would be printed even though some things have gone wrong on the remote
side.

As promised last week, I've now adopted the patch series from Jiang Xin
to make some progress on the issue. The series is now based on the
latest master branch at 3b0d05c4a7 (The fifth batch, 2025-01-29).

Changes in v4:
  - Rewrite the commit that adds two new tests for `--porcelain`.
    Previously, the commit both reordered existing tests and added new
    ones, which made it hard to see what actually changed. The reorder
    wasn't necessary though, so I've adapted it to only add new tests
    now.
  - Fix the URL prefix in t5548 to also work on Windows.
  - Document why it's fine to start ignoring the return value of
    `git_transport_push()`.
  - Improve comments to document behaviour better.
  - Link to v3: https://lore.kernel.org/r/cover.1733830410.git.zhiyou.jx@alibaba-inc.com

Changes in v5:
  - Escape heredocs where possible.
  - Link to v4: https://lore.kernel.org/r/20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im

Thanks!

Patrick

---
Jiang Xin (5):
      t5548: refactor to reuse setup_upstream() function
      t5548: refactor test cases by resetting upstream
      t5548: add porcelain push test cases for dry-run mode
      send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"
      send-pack: gracefully close the connection for atomic push

Patrick Steinhardt (3):
      t5504: modernize test by moving heredocs into test bodies
      t5548: add new porcelain test cases
      t5543: atomic push reports exit code failure

 send-pack.c                     |  10 +-
 send-pack.h                     |  13 ++
 t/t5504-fetch-receive-strict.sh |  35 ++--
 t/t5543-atomic-push.sh          |  30 +++
 t/t5548-push-porcelain.sh       | 443 ++++++++++++++++++++++++++++++----------
 transport.c                     |  17 +-
 6 files changed, 406 insertions(+), 142 deletions(-)

Range-diff versus v4:

1:  ef3732a280 ! 1:  2d04ec6ca3 t5504: modernize test by moving heredocs into test bodies
    @@ t/t5504-fetch-receive-strict.sh: test_expect_success 'push without strict' '
      		git config fetch.fsckobjects false &&
      		git config transfer.fsckobjects false
      	) &&
    -+	cat >exp <<-EOF &&
    ++	cat >exp <<-\EOF &&
     +	To dst
     +	!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
     +	Done
    @@ t/t5504-fetch-receive-strict.sh: test_expect_success 'push with receive.fsckobje
      		git config receive.fsckobjects true &&
      		git config transfer.fsckobjects false
      	) &&
    -+	cat >exp <<-EOF &&
    ++	cat >exp <<-\EOF &&
     +	To dst
     +	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
     +	EOF
2:  c4a57bf457 ! 2:  31c8b10e7d t5548: refactor to reuse setup_upstream() function
    @@ t/t5548-push-porcelain.sh: format_and_save_expect () {
     +	if test $# -ne 1
     +	then
     +		BUG "location of upstream repository is not provided"
    -+	fi &&
    -+	# Assign the first argument to the variable upstream;
    -+	# we will use it in the subsequent test cases.
    ++	fi
     +	upstream="$1"
     +
      	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
3:  67928693cc ! 3:  a8de197677 t5548: refactor test cases by resetting upstream
    @@ Commit message
     
      ## t/t5548-push-porcelain.sh ##
     @@ t/t5548-push-porcelain.sh: setup_upstream_and_workbench () {
    - 	# we will use it in the subsequent test cases.
    + 	fi
      	upstream="$1"
      
     -	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
4:  6e0f0f791f ! 4:  9e4f3be9e0 t5548: add new porcelain test cases
    @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
     +			baz \
     +			next >out &&
     +		make_user_friendly_and_stable_output <out >actual &&
    -+		format_and_save_expect <<-EOF &&
    ++		format_and_save_expect <<-\EOF &&
     +		> To <URL/of/upstream.git>
     +		> =	refs/heads/baz:refs/heads/baz	[up to date]
     +		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
    @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
     +			baz \
     +			next >out &&
     +		make_user_friendly_and_stable_output <out >actual &&
    -+		format_and_save_expect <<-EOF &&
    ++		format_and_save_expect <<-\EOF &&
     +		> To <URL/of/upstream.git>
     +		> =	refs/heads/baz:refs/heads/baz	[up to date]
     +		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
5:  cb985baec9 = 5:  093b50d785 t5548: add porcelain push test cases for dry-run mode
6:  68ae698e4b = 6:  9a1851f06e send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"
7:  525cefd4f2 = 7:  e789913922 t5543: atomic push reports exit code failure
8:  19cdbf991f = 8:  52101a1f14 send-pack: gracefully close the connection for atomic push

---
base-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08
change-id: 20241113-pks-push-atomic-respect-exit-code-436c443a657d

[PATCH v5 1/8] t5504: modernize test by moving heredocs into test bodies

[Patrick Steinhardt]

We have several heredocs in t5504 located outside of any particular test
bodies. Move these into the test bodies to match our modern coding
style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5504-fetch-receive-strict.sh | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index e273ab29c7..58074506c5 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 	)
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
-Done
-EOF
-
 test_expect_success 'push without strict' '
 	rm -rf dst &&
 	git init dst &&
@@ -78,6 +72,11 @@ test_expect_success 'push without strict' '
 		git config fetch.fsckobjects false &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-\EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
+	Done
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '
 	test_cmp exp act
 '
 
-cat >exp <<EOF
-To dst
-!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
-EOF
-
 test_expect_success 'push with receive.fsckobjects' '
 	rm -rf dst &&
 	git init dst &&
@@ -107,6 +101,10 @@ test_expect_success 'push with receive.fsckobjects' '
 		git config receive.fsckobjects true &&
 		git config transfer.fsckobjects false
 	) &&
+	cat >exp <<-\EOF &&
+	To dst
+	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
+	EOF
 	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
 	test_cmp exp act
 '
@@ -129,15 +127,14 @@ test_expect_success 'repair the "corrupt or missing" object' '
 	git fsck
 '
 
-cat >bogus-commit <<EOF
-tree $EMPTY_TREE
-author Bugs Bunny 1234567890 +0000
-committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
-
-This commit object intentionally broken
-EOF
-
 test_expect_success 'setup bogus commit' '
+	cat >bogus-commit <<-EOF &&
+	tree $EMPTY_TREE
+	author Bugs Bunny 1234567890 +0000
+	committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
+
+	This commit object intentionally broken
+	EOF
 	commit="$(git hash-object --literally -t commit -w --stdin <bogus-commit)"
 '
 

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v5 2/8] t5548: refactor to reuse setup_upstream() function

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor the function setup_upstream_and_workbench(), extracting
create_upstream_template() and setup_upstream() from it. The former is
used to create the upstream repository template, while the latter is
used to rebuild the upstream repository and will be reused in subsequent
commits.

To ensure that setup_upstream() works properly in both local and HTTP
protocols, the HTTP settings have been moved to the setup_upstream() and
setup_upstream_and_workbench() functions.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5548-push-porcelain.sh | 83 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index 6282728eaf..1bf4d48cd9 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -54,29 +54,65 @@ format_and_save_expect () {
 	sed -e 's/^> //' -e 's/Z$//' >expect
 }
 
+create_upstream_template () {
+	git init --bare upstream-template.git &&
+	git clone upstream-template.git tmp_work_dir &&
+	create_commits_in tmp_work_dir A B &&
+	(
+		cd tmp_work_dir &&
+		git push origin \
+			$B:refs/heads/main \
+			$A:refs/heads/foo \
+			$A:refs/heads/bar \
+			$A:refs/heads/baz
+	) &&
+	rm -rf tmp_work_dir
+}
+
+setup_upstream () {
+	if test $# -ne 1
+	then
+		BUG "location of upstream repository is not provided"
+	fi &&
+	rm -rf "$1" &&
+	if ! test -d upstream-template.git
+	then
+		create_upstream_template
+	fi &&
+	git clone --mirror upstream-template.git "$1" &&
+	# The upstream repository provides services using the HTTP protocol.
+	if ! test "$1" = "upstream.git"
+	then
+		git -C "$1" config http.receivepack true
+	fi
+}
+
 setup_upstream_and_workbench () {
+	if test $# -ne 1
+	then
+		BUG "location of upstream repository is not provided"
+	fi
+	upstream="$1"
+
 	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
 	# Workbench after setup : main(A)
 	test_expect_success "setup upstream repository and workbench" '
-		rm -rf upstream.git workbench &&
-		git init --bare upstream.git &&
-		git init workbench &&
-		create_commits_in workbench A B &&
+		setup_upstream "$upstream" &&
+		rm -rf workbench &&
+		git clone "$upstream" workbench &&
 		(
 			cd workbench &&
+			git update-ref refs/heads/main $A &&
 			# Try to make a stable fixed width for abbreviated commit ID,
 			# this fixed-width oid will be replaced with "<OID>".
 			git config core.abbrev 7 &&
-			git remote add origin ../upstream.git &&
-			git update-ref refs/heads/main $A &&
-			git push origin \
-				$B:refs/heads/main \
-				$A:refs/heads/foo \
-				$A:refs/heads/bar \
-				$A:refs/heads/baz
+			git config advice.pushUpdateRejected false
 		) &&
-		git -C "workbench" config advice.pushUpdateRejected false &&
-		upstream=upstream.git
+		# The upstream repository provides services using the HTTP protocol.
+		if ! test "$upstream" = "upstream.git"
+		then
+			git -C workbench remote set-url origin "$HTTPD_URL/smart/upstream.git"
+		fi
 	'
 }
 
@@ -88,7 +124,7 @@ run_git_push_porcelain_output_test() {
 		;;
 	file)
 		PROTOCOL="builtin protocol"
-		URL_PREFIX="\.\."
+		URL_PREFIX=".*"
 		;;
 	esac
 
@@ -247,10 +283,8 @@ run_git_push_porcelain_output_test() {
 	'
 }
 
-# Initialize the upstream repository and local workbench.
-setup_upstream_and_workbench
+setup_upstream_and_workbench upstream.git
 
-# Run git-push porcelain test on builtin protocol
 run_git_push_porcelain_output_test file
 
 ROOT_PATH="$PWD"
@@ -258,21 +292,10 @@ ROOT_PATH="$PWD"
 . "$TEST_DIRECTORY"/lib-httpd.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 start_httpd
-
-# Re-initialize the upstream repository and local workbench.
-setup_upstream_and_workbench
-
-test_expect_success "setup for http" '
-	git -C upstream.git config http.receivepack true &&
-	upstream="$HTTPD_DOCUMENT_ROOT_PATH/upstream.git" &&
-	mv upstream.git "$upstream" &&
-
-	git -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git
-'
-
 setup_askpass_helper
 
-# Run git-push porcelain test on HTTP protocol
+setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
+
 run_git_push_porcelain_output_test http
 
 test_done

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v5 3/8] t5548: refactor test cases by resetting upstream

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor the test cases with the following changes:

 - Calling setup_upstream() to reset upstream after running each test
   case.

 - Change the initial branch tips of the workspace to reduce the branch
   setup operations in the workspace.

 - Reduced the two steps of setting up and cleaning up the pre-receive
   hook by moving the operations into the corresponding test case,

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5548-push-porcelain.sh | 149 +++++++++++++++++++++-------------------------
 1 file changed, 67 insertions(+), 82 deletions(-)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index 1bf4d48cd9..f9aeb5a9d9 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -94,8 +94,8 @@ setup_upstream_and_workbench () {
 	fi
 	upstream="$1"
 
-	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
-	# Workbench after setup : main(A)
+	# Upstream  after setup: main(B)  foo(A)  bar(A)  baz(A)
+	# Workbench after setup: main(A)                  baz(A)  next(A)
 	test_expect_success "setup upstream repository and workbench" '
 		setup_upstream "$upstream" &&
 		rm -rf workbench &&
@@ -103,6 +103,8 @@ setup_upstream_and_workbench () {
 		(
 			cd workbench &&
 			git update-ref refs/heads/main $A &&
+			git update-ref refs/heads/baz $A &&
+			git update-ref refs/heads/next $A &&
 			# Try to make a stable fixed width for abbreviated commit ID,
 			# this fixed-width oid will be replaced with "<OID>".
 			git config core.abbrev 7 &&
@@ -131,19 +133,14 @@ run_git_push_porcelain_output_test() {
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
-	test_expect_success "porcelain output of successful git-push ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $A &&
-			git update-ref refs/heads/baz $A &&
-			git update-ref refs/heads/next $A &&
-			git push --porcelain --force origin \
-				main \
-				:refs/heads/foo \
-				$B:bar \
-				baz \
-				next
-		) >out &&
+	test_expect_success ".. git-push --porcelain --force ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
 		> To <URL/of/upstream.git>
@@ -167,115 +164,103 @@ run_git_push_porcelain_output_test() {
 		test_cmp expect actual
 	'
 
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "atomic push failed ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $B &&
-			git update-ref refs/heads/bar $A &&
-			test_must_fail git push --atomic --porcelain origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git push --porcelain --atomic ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		test_must_fail git -C workbench push --porcelain --atomic origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> !	refs/heads/bar:refs/heads/bar	[rejected] (non-fast-forward)
-		> !	(delete):refs/heads/baz	[rejected] (atomic push failed)
-		> !	refs/heads/main:refs/heads/main	[rejected] (atomic push failed)
-		Done
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[rejected] (atomic push failed)
+		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
 		<COMMIT-A> refs/heads/baz
-		<COMMIT-A> refs/heads/main
-		<COMMIT-A> refs/heads/next
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
 		EOF
 		test_cmp expect actual
 	'
 
-	test_expect_success "prepare pre-receive hook ($PROTOCOL)" '
-		test_hook --setup -C "$upstream" pre-receive <<-EOF
-		exit 1
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. pre-receive hook declined ($PROTOCOL)" '
+		test_when_finished "rm -f \"$upstream/hooks/pre-receive\" &&
+			setup_upstream \"$upstream\"" &&
+		test_hook --setup -C "$upstream" pre-receive <<-EOF &&
+			exit 1
 		EOF
-	'
-
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "pre-receive hook declined ($PROTOCOL)" '
-		(
-			cd workbench &&
-			git update-ref refs/heads/main $B &&
-			git update-ref refs/heads/bar $A &&
-			test_must_fail git push --porcelain --force origin \
-				main \
-				bar \
-				:baz \
-				next
-		) >out &&
+		test_must_fail git -C workbench push --porcelain --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> !	refs/heads/bar:refs/heads/bar	[remote rejected] (pre-receive hook declined)
-		> !	:refs/heads/baz	[remote rejected] (pre-receive hook declined)
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[remote rejected] (pre-receive hook declined)
+		> !	:refs/heads/foo	[remote rejected] (pre-receive hook declined)
 		> !	refs/heads/main:refs/heads/main	[remote rejected] (pre-receive hook declined)
-		Done
+		> !	refs/heads/next:refs/heads/next	[remote rejected] (pre-receive hook declined)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
 		<COMMIT-A> refs/heads/baz
-		<COMMIT-A> refs/heads/main
-		<COMMIT-A> refs/heads/next
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
 		EOF
 		test_cmp expect actual
 	'
 
-	test_expect_success "remove pre-receive hook ($PROTOCOL)" '
-		rm "$upstream/hooks/pre-receive"
-	'
-
-	# Refs of upstream : main(A)  bar(B)  baz(A)  next(A)
-	# Refs of workbench: main(B)  bar(A)  baz(A)  next(A)
-	# git-push         : main(B)  bar(A)  NULL    next(A)
-	test_expect_success "non-fastforward push ($PROTOCOL)" '
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)                          next(A)
+	test_expect_success ".. non-fastforward push ($PROTOCOL)" '
 		(
 			cd workbench &&
 			test_must_fail git push --porcelain origin \
 				main \
-				bar \
-				:baz \
 				next
 		) >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		format_and_save_expect <<-EOF &&
-		To <URL/of/upstream.git>
-		> =	refs/heads/next:refs/heads/next	[up to date]
-		> -	:refs/heads/baz	[deleted]
-		>  	refs/heads/main:refs/heads/main	<COMMIT-A>..<COMMIT-B>
-		> !	refs/heads/bar:refs/heads/bar	[rejected] (non-fast-forward)
-		Done
+		> To <URL/of/upstream.git>
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
 		EOF
 		test_cmp expect actual &&
 
 		git -C "$upstream" show-ref >out &&
 		make_user_friendly_and_stable_output <out >actual &&
 		cat >expect <<-EOF &&
-		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
 		<COMMIT-B> refs/heads/main
 		<COMMIT-A> refs/heads/next
 		EOF

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v5 4/8] t5548: add new porcelain test cases

[Patrick Steinhardt]

Add two more test cases exercising git-push(1) with `--procelain`, one
exercising a non-atomic and one exercising an atomic push.

Based-on-patch-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5548-push-porcelain.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index f9aeb5a9d9..5d724145d2 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -130,6 +130,40 @@ run_git_push_porcelain_output_test() {
 		;;
 	esac
 
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
+		test_must_fail git -C workbench push --porcelain origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-\EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-B> refs/heads/main
+		<COMMIT-A> refs/heads/next
+		EOF
+		test_cmp expect actual
+	'
+
 	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
@@ -240,6 +274,7 @@ run_git_push_porcelain_output_test() {
 	# Refs of workbench: main(A)                  baz(A)  next(A)
 	# git-push         : main(A)                          next(A)
 	test_expect_success ".. non-fastforward push ($PROTOCOL)" '
+		test_when_finished "setup_upstream \"$upstream\"" &&
 		(
 			cd workbench &&
 			test_must_fail git push --porcelain origin \
@@ -266,6 +301,39 @@ run_git_push_porcelain_output_test() {
 		EOF
 		test_cmp expect actual
 	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git push --porcelain --atomic --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --atomic --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-\EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-B> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/main
+		<COMMIT-A> refs/heads/next
+		EOF
+		test_cmp expect actual
+	'
 }
 
 setup_upstream_and_workbench upstream.git

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v5 5/8] t5548: add porcelain push test cases for dry-run mode

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

New dry-run test cases:

 - git push --porcelain --dry-run
 - git push --porcelain --dry-run --force
 - git push --porcelain --dry-run --atomic
 - git push --porcelain --dry-run --atomic --force

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5548-push-porcelain.sh | 153 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index 5d724145d2..4c19404ebe 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -336,10 +336,159 @@ run_git_push_porcelain_output_test() {
 	'
 }
 
+run_git_push_dry_run_porcelain_output_test() {
+	case $1 in
+	http)
+		PROTOCOL="HTTP protocol"
+		URL_PREFIX="http://.*"
+		;;
+	file)
+		PROTOCOL="builtin protocol"
+		URL_PREFIX=".*"
+		;;
+	esac
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run ($PROTOCOL)" '
+		test_must_fail git -C workbench push --porcelain --dry-run origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# push             : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --dry-run --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# git-push         : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --atomic ($PROTOCOL)" '
+		test_must_fail git -C workbench push --porcelain --dry-run --atomic origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		> !	<COMMIT-B>:refs/heads/bar	[rejected] (atomic push failed)
+		> !	(delete):refs/heads/foo	[rejected] (atomic push failed)
+		> !	refs/heads/main:refs/heads/main	[rejected] (non-fast-forward)
+		> !	refs/heads/next:refs/heads/next	[rejected] (atomic push failed)
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+
+	# Refs of upstream : main(B)  foo(A)  bar(A)  baz(A)
+	# Refs of workbench: main(A)                  baz(A)  next(A)
+	# push             : main(A)  NULL    (B)     baz(A)  next(A)
+	test_expect_success ".. git-push --porcelain --dry-run --atomic --force ($PROTOCOL)" '
+		git -C workbench push --porcelain --dry-run --atomic --force origin \
+			main \
+			:refs/heads/foo \
+			$B:bar \
+			baz \
+			next >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		format_and_save_expect <<-EOF &&
+		> To <URL/of/upstream.git>
+		> =	refs/heads/baz:refs/heads/baz	[up to date]
+		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
+		> -	:refs/heads/foo	[deleted]
+		> +	refs/heads/main:refs/heads/main	<COMMIT-B>...<COMMIT-A> (forced update)
+		> *	refs/heads/next:refs/heads/next	[new branch]
+		> Done
+		EOF
+		test_cmp expect actual &&
+
+		git -C "$upstream" show-ref >out &&
+		make_user_friendly_and_stable_output <out >actual &&
+		cat >expect <<-EOF &&
+		<COMMIT-A> refs/heads/bar
+		<COMMIT-A> refs/heads/baz
+		<COMMIT-A> refs/heads/foo
+		<COMMIT-B> refs/heads/main
+		EOF
+		test_cmp expect actual
+	'
+}
+
 setup_upstream_and_workbench upstream.git
 
 run_git_push_porcelain_output_test file
 
+setup_upstream_and_workbench upstream.git
+
+run_git_push_dry_run_porcelain_output_test file
+
 ROOT_PATH="$PWD"
 . "$TEST_DIRECTORY"/lib-gpg.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
@@ -351,4 +500,8 @@ setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
 
 run_git_push_porcelain_output_test http
 
+setup_upstream_and_workbench "$HTTPD_DOCUMENT_ROOT_PATH/upstream.git"
+
+run_git_push_dry_run_porcelain_output_test http
+
 test_done

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v5 6/8] send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The "push_refs" function in the transport_vtable is the handler for
git-push operation. All the "push_refs" functions for different
transports (protocols) should have the same behavior, but the behavior
of "git_transport_push()" function for builtin_smart_vtable in
"transport.c" (which calls "send_pack()" in "send-pack.c") differs from
the handler of the HTTP protocol.

The "push_refs()" function for the HTTP protocol which calls the
"push_refs_with_push()" function in "transport-helper.c" will return 0
even when a bad REF_STATUS (such as REF_STATUS_REJECT_NONFASTFORWARD)
was found. But "send_pack()" for Git smart protocol will return -1 for
a bad REF_STATUS.

We cannot ignore bad REF_STATUS directly in the "send_pack()" function,
because the function is also used in "builtin/send-pack.c". So we add a
new non-zero error code "SEND_PACK_ERROR_REF_STATUS" for "send_pack()".

Ignore the specific error code in the "git_transport_push()" function to
have the same behavior as "push_refs()" for HTTP protocol. Note that
even though we ignore the error here, we'll ultimately still end up
detecting that a subset of refs was not pushed in `transport_push()`
because we eventually call `push_had_errors()` on the remote refs.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c |  9 ++-------
 send-pack.h | 13 +++++++++++++
 transport.c |  7 +++++++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 772c7683a0..4448c081cc 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -632,7 +632,7 @@ int send_pack(struct repository *r,
 				reject_atomic_push(remote_refs, args->send_mirror);
 				error("atomic push failed for ref %s. status: %d",
 				      ref->name, ref->status);
-				ret = args->porcelain ? 0 : -1;
+				ret = ERROR_SEND_PACK_BAD_REF_STATUS;
 				goto out;
 			}
 			/* else fallthrough */
@@ -763,11 +763,6 @@ int send_pack(struct repository *r,
 	if (ret < 0)
 		goto out;
 
-	if (args->porcelain) {
-		ret = 0;
-		goto out;
-	}
-
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:
@@ -775,7 +770,7 @@ int send_pack(struct repository *r,
 		case REF_STATUS_OK:
 			break;
 		default:
-			ret = -1;
+			ret = ERROR_SEND_PACK_BAD_REF_STATUS;
 			goto out;
 		}
 	}
diff --git a/send-pack.h b/send-pack.h
index d256715681..c5ded2d200 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -13,6 +13,9 @@ struct repository;
 #define SEND_PACK_PUSH_CERT_IF_ASKED 1
 #define SEND_PACK_PUSH_CERT_ALWAYS 2
 
+/* At least one reference has been rejected by the remote side. */
+#define ERROR_SEND_PACK_BAD_REF_STATUS 1
+
 struct send_pack_args {
 	const char *url;
 	unsigned verbose:1,
@@ -36,6 +39,16 @@ struct option;
 int option_parse_push_signed(const struct option *opt,
 			     const char *arg, int unset);
 
+/*
+ * Compute a packfile and write it to a file descriptor. The `fd` array needs
+ * to contain two file descriptors: `fd[0]` is the file descriptor used as
+ * input for the packet reader, whereas `fd[1]` is the file descriptor the
+ * packfile will be written to.
+ *
+ * Returns 0 on success, non-zero otherwise. Negative return values indicate a
+ * generic error, whereas positive return values indicate specific error
+ * conditions as documented with the `ERROR_SEND_PACK_*` constants.
+ */
 int send_pack(struct repository *r, struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs, struct oid_array *extra_have);
diff --git a/transport.c b/transport.c
index 81ae8243b9..d064aff33e 100644
--- a/transport.c
+++ b/transport.c
@@ -934,6 +934,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	case protocol_v0:
 		ret = send_pack(the_repository, &args, data->fd, data->conn, remote_refs,
 				&data->extra_have);
+		/*
+		 * Ignore the specific error code to maintain consistent behavior
+		 * with the "push_refs()" function across different transports,
+		 * such as "push_refs_with_push()" for HTTP protocol.
+		 */
+		if (ret == ERROR_SEND_PACK_BAD_REF_STATUS)
+			ret = 0;
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v5 7/8] t5543: atomic push reports exit code failure

[Patrick Steinhardt]

Add new test cases in t5543 to avoid ignoring the exit code of
git-receive-pack(1) during atomic push with "--porcelain" flag.

We'd typically notice this case because the refs would have their error
message set. But there is an edge case when pushing refs succeeds, but
git-receive-pack(1) exits with a non-zero exit code at a later point in
time due to another error. An atomic git-push(1) would ignore that error
code, and consequently it would return successfully and not print any
error message at all.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 04b47ad84a..32181b9afb 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -280,4 +280,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'atomic push reports exit code failure' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict 2>err &&
+	cat >expect <<-EOF &&
+	To ../upstream
+	 * [new branch]      HEAD -> no-conflict
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
+test_expect_failure 'atomic push reports exit code failure with porcelain' '
+	write_script receive-pack-wrapper <<-\EOF &&
+	git-receive-pack "$@"
+	exit 1
+	EOF
+	test_must_fail git -C workbench push --atomic --porcelain \
+		--receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \
+		up HEAD:refs/heads/no-conflict-porcelain 2>err &&
+	cat >expect <<-EOF &&
+	error: failed to push some refs to ${SQ}../upstream${SQ}
+	EOF
+	test_cmp expect err
+'
+
 test_done

-- 
2.48.1.502.g6dc24dfdaf.dirty

[PATCH v5 8/8] send-pack: gracefully close the connection for atomic push

[Patrick Steinhardt]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Patrick reported an issue that the exit code of git-receive-pack(1) is
ignored during atomic push with "--porcelain" flag, and added new test
cases in t5543.

This issue originated from commit 7dcbeaa0df (send-pack: fix
inconsistent porcelain output, 2020-04-17). At that time, I chose to
ignore the exit code of "finish_connect()" without investigating the
root cause of the abnormal termination of git-receive-pack. That was an
incorrect solution.

The root cause is that an atomic push operation terminates early without
sending a flush packet to git-receive-pack. As a result,
git-receive-pack continues waiting for commands without exiting. By
sending a flush packet at the appropriate location in "send_pack()", we
ensure that the git-receive-pack process closes properly, avoiding an
erroneous exit code for git-push. At the same time, revert the changes
to the "transport.c" file made in commit 7dcbeaa0df.

Reported-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c            |  1 +
 t/t5543-atomic-push.sh |  4 ++--
 transport.c            | 10 +---------
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 4448c081cc..856a65d5f5 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -633,6 +633,7 @@ int send_pack(struct repository *r,
 				error("atomic push failed for ref %s. status: %d",
 				      ref->name, ref->status);
 				ret = ERROR_SEND_PACK_BAD_REF_STATUS;
+				packet_flush(out);
 				goto out;
 			}
 			/* else fallthrough */
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 32181b9afb..3a700b0676 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -280,7 +280,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'atomic push reports exit code failure' '
+test_expect_success 'atomic push reports exit code failure' '
 	write_script receive-pack-wrapper <<-\EOF &&
 	git-receive-pack "$@"
 	exit 1
@@ -296,7 +296,7 @@ test_expect_failure 'atomic push reports exit code failure' '
 	test_cmp expect err
 '
 
-test_expect_failure 'atomic push reports exit code failure with porcelain' '
+test_expect_success 'atomic push reports exit code failure with porcelain' '
 	write_script receive-pack-wrapper <<-\EOF &&
 	git-receive-pack "$@"
 	exit 1
diff --git a/transport.c b/transport.c
index d064aff33e..b0c6c339f4 100644
--- a/transport.c
+++ b/transport.c
@@ -948,15 +948,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 
 	close(data->fd[1]);
 	close(data->fd[0]);
-	/*
-	 * Atomic push may abort the connection early and close the pipe,
-	 * which may cause an error for `finish_connect()`. Ignore this error
-	 * for atomic git-push.
-	 */
-	if (ret || args.atomic)
-		finish_connect(data->conn);
-	else
-		ret = finish_connect(data->conn);
+	ret |= finish_connect(data->conn);
 	data->conn = NULL;
 	data->finished_handshake = 0;
 

-- 
2.48.1.502.g6dc24dfdaf.dirty

Re: [PATCH v5 0/8] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Changes in v5:
>   - Escape heredocs where possible.
>   - Link to v4: https://lore.kernel.org/r/20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im
>
> Thanks!

Changes since the last round looked trivially correct ;-)

Will replace.