push: fix the behavior of the Done message for porcelain

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 7755585 (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 <[email protected]> Signed-off-by: Jiang Xin <[email protected]>
jiangxin · Dec 10, 2024 · b10df53 · b10df53
1 parent c741b2a
commit b10df53
Show file tree

Hide file tree

Showing 13 changed files with 14 additions and 25 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
@@ -1189,7 +1189,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
@@ -1206,7 +1205,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
@@ -384,7 +384,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
@@ -498,7 +498,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/transport.c b/transport.c
@@ -1489,7 +1489,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,
@@ -1506,7 +1517,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 */