diff mbox series

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

Message ID 9dfea9953c2a671600beebbaa0692a1e68afea34.1731603991.git.zhiyou.jx@alibaba-inc.com (mailing list archive)
State New
Headers show
Series fix behaviors of git-push --porcelain | expand

Commit Message

Jiang Xin Nov. 14, 2024, 5:15 p.m. UTC 
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 mbox series

Patch

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 */