From b10df5334438d37a4470ec0a798e9ebdc13fd574 Mon Sep 17 00:00:00 2001 From: Jiang Xin Date: Thu, 14 Nov 2024 19:25:26 +0800 Subject: [PATCH] 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 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 Signed-off-by: Jiang Xin --- 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/t5516-fetch-push.sh | 3 +-- t/t5534-push-signed.sh | 1 - t/t5541-http-push-smart.sh | 1 - transport.c | 15 +++++++++++++-- 13 files changed, 14 insertions(+), 25 deletions(-) diff --git a/t/t5411/test-0001-standard-git-push--porcelain.sh b/t/t5411/test-0001-standard-git-push--porcelain.sh index 373ec3d865dba3..5ff901454acb59 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 > :refs/heads/next .. > ! 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 67ca6dc4f8f232..f4cdf9db42da01 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 > ! :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 04468b501887b6..563de678590829 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 > * 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 > ! :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 298a3d1feca1ff..096f13a8322ffb 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 ! 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 ! 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 ! 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 ! 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 ! 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 ! 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 > * 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 > ! 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 > ! 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 502b34fe3dd723..c4b1d25562257c 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 > ! 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 > ! 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 6d116ef692c6c2..d224e2400e56b3 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 > :refs/heads/main .. > ! 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 8b3f5d05a3fab6..278fc597ebab13 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 > ! 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 2e1831b104e8a9..2c1117457f37f1 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 > ! 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 40f4c5b1afba88..e1b64edea91bff 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 .. > ! 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 index 9d693eb57f7790..191e65a45beaa2 100755 --- 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 index c91a62b77afcfb..a17a2f22dd961e 100755 --- 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 index 71428f3d5c760a..813f21e31c2139 100755 --- 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 index e53f09d7ceb07d..6040593832a832 100644 --- 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 */