{"thread":{"id":"62487","subject":"[PATCH 0/2] transport: don't ignore git-receive-pack(1) exit code on atomic push","startedAt":"2024-11-13T11:24:31Z","lastAt":"2025-02-03T23:26:04Z","messageCount":54,"participants":["Patrick Steinhardt","Jiang Xin","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"507207","messageId":"20241113-pks-push-atomic-respect-exit-code-v1-0-7965f01e7f4e@pks.im","threadId":"62487","inReplyTo":null,"subject":"[PATCH 0/2] transport: don't ignore git-receive-pack(1) exit code on atomic push","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-13T11:24:15Z","receivedAt":"2024-11-13T11:24:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwe've hit an edge case at GitLab where an atomic push will not notice an\nerror when git-receive-pack(1) updates the refs, but otherwise fails\nwith a non-zero exit code. The push would be successful and no error\nwould be printed even though some things have gone wrong on the remote\nside.\n\nThis patch serise fixes the issue. I'm not a 100% sure whether this fix\nis correct or not because it has interactions with the \"--porcelain\"\nmode, which is quite underspecified overall. So I'd appreciate some\nextra scrutiny on this series.\n\nThanks!\n\nPatrick\n\nTo: git@vger.kernel.org\nCc: Jiang Xin \n\nPatrick Steinhardt (2):\n t5504: modernize test by moving heredocs into test bodies\n transport: don't ignore git-receive-pack(1) exit code on atomic push\n\n send-pack.c | 2 +-\n t/t5504-fetch-receive-strict.sh | 36 +++++++++++++++++-------------------\n t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++\n transport.c | 9 ++-------\n 4 files changed, 50 insertions(+), 27 deletions(-)\n\n---\nbase-commit: 25b0f41288718625b18495de23cc066394c09a92\nchange-id: 20241113-pks-push-atomic-respect-exit-code-436c443a657d\n\n"},{"id":"507208","messageId":"20241113-pks-push-atomic-respect-exit-code-v1-1-7965f01e7f4e@pks.im","threadId":"62487","inReplyTo":"20241113-pks-push-atomic-respect-exit-code-v1-0-7965f01e7f4e@pks.im","subject":"[PATCH 1/2] t5504: modernize test by moving heredocs into test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-13T11:24:16Z","receivedAt":"2024-11-13T11:24:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have several heredocs in t5504 located outside of any particular test\nbodies. Move these into the test bodies to match our modern coding\nstyle.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5504-fetch-receive-strict.sh | 35 ++++++++++++++++-------------------\n 1 file changed, 16 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 138e6778a477650ecbe2dc3e480c5fe83d4bb485..0a3883043baf5c4c0fc43b52e8c5fc375f10a56a 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -65,12 +65,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n \t)\n '\n \n-cat >exp <exp <<-EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (missing necessary objects)\n+\tDone\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -95,11 +94,6 @@ test_expect_success 'push with !receive.fsckobjects' '\n \ttest_cmp exp act\n '\n \n-cat >exp <exp <<-EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (unpacker error)\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -130,15 +128,14 @@ test_expect_success 'repair the \"corrupt or missing\" object' '\n \tgit fsck\n '\n \n-cat >bogus-commit < 1234567890 +0000\n-\n-This commit object intentionally broken\n-EOF\n-\n test_expect_success 'setup bogus commit' '\n+\tcat >bogus-commit <<-EOF &&\n+\ttree $EMPTY_TREE\n+\tauthor Bugs Bunny 1234567890 +0000\n+\tcommitter Bugs Bunny 1234567890 +0000\n+\n+\tThis commit object intentionally broken\n+\tEOF\n \tcommit=\"$(git hash-object --literally -t commit -w --stdin \n---\n send-pack.c | 2 +-\n t/t5504-fetch-receive-strict.sh | 1 +\n t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++\n transport.c | 9 ++-------\n 4 files changed, 34 insertions(+), 8 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 6677c44e8acd19f16706ad2d78f72fee889daa55..815c1f206c09da8fb1ffe9be11beb9c5fa29d0c5 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n-\t\t\t\tret = args->porcelain ? 0 : -1;\n+\t\t\t\tret = -1;\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 0a3883043baf5c4c0fc43b52e8c5fc375f10a56a..b3774e56e0f45a91d574a15f0d6b5c50c4da70d4 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -105,6 +105,7 @@ test_expect_success 'push with receive.fsckobjects' '\n \tcat >exp <<-EOF &&\n \tTo dst\n \t!\trefs/heads/main:refs/heads/test\t[remote rejected] (unpacker error)\n+\tDone\n \tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 479d103469527e6b923877cd480825b59e7094d4..1d8f088a004cf6743ade8e55536f04ac04000cf7 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -281,4 +281,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'atomic push reports exit code failure' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict 2>err &&\n+\tcat >expect <<-EOF &&\n+\tTo ../upstream\n+\t * [new branch] HEAD -> no-conflict\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success 'atomic push reports exit code failure with porcelain' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic --porcelain \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict-porcelain 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex 47fda6a7732f4b8cdcb6e750f36b896a988ffd0b..76b0645a25fe936190ca52d595b5f02eedc27d23 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -921,12 +921,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \n \tclose(data->fd[1]);\n \tclose(data->fd[0]);\n-\t/*\n-\t * Atomic push may abort the connection early and close the pipe,\n-\t * which may cause an error for `finish_connect()`. Ignore this error\n-\t * for atomic git-push.\n-\t */\n-\tif (ret || args.atomic)\n+\tif (ret)\n \t\tfinish_connect(data->conn);\n \telse\n \t\tret = finish_connect(data->conn);\n@@ -1503,7 +1498,7 @@ int transport_push(struct repository *r,\n \t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n \t}\n \n-\tif (porcelain && !push_ret)\n+\tif (porcelain)\n \t\tputs(\"Done\");\n \telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n \t\t/* stable plumbing output; do not modify or localize */\n\n-- \n2.47.0.251.gb31fb630c0.dirty\n\n"},{"id":"507259","messageId":"CANYiYbHGdJsTgdzJj1r4sPdTAcZCf3C-qA538fcXbD1jjB1BUw@mail.gmail.com","threadId":"62487","inReplyTo":"20241113-pks-push-atomic-respect-exit-code-v1-2-7965f01e7f4e@pks.im","subject":"Re: [PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-11-14T08:57:55Z","receivedAt":"2024-11-14T08:58:06Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Wed, Nov 13, 2024 at 7:25 PM Patrick Steinhardt wrote:\n>\n> When executing git-push(1) with the \"--porcelain\" flag, then we will\n> print updated references in a machine-readable format that looks like\n> this:\n>\n> To destination\n> = refs/heads/noop:refs/heads/noop [up to date]\n> ! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)\n> ! refs/heads/noff:refs/heads/(off (non-fast-forward)\n> Done\n>\n> The final \"Done\" stanza was introduced via 77555854be (git-push: make\n> git push --porcelain print \"Done\", 2010-02-26), where it was printed\n> \"unless any errors have occurred\". This behaviour was later changed via\n> 7dcbeaa0df (send-pack: fix inconsistent porcelain output, 2020-04-17)\n> such that the stanza will also be printed when there was an error with\n> atomic pushes, which was previously inconsistent with non-atomic pushes.\n> The fixup commit has introduced a new issue though. During atomic pushes\n> it is expected that git-receive-pack(1) may exit early, and that causes\n> us to receive an error on the client-side. We (seemingly) still want to\n> print the \"Done\" stanza, but given that we only do so when the push has\n> been successful we started to ignore the error code by the child process\n> completely when doing an atomic push.\n\nI introduced the commit 7dcbeaa0df (send-pack: fix inconsistent porcelain\noutput, 2020-04-17), because the porcelain output for git push over local\nfile protocol and HTTP protocol are different and was hard to write the\nsome test cases to work for both protocols. I acknowledge the patch was\nnot perfect.\n\nI read the commit 77555854be (git-push: make git push --porcelain\nprint \"Done\", 2010-02-26) and the code path of git push over two\nprotocols a second time, and find something:\n\nThe code snippet from 77555854be:\n\n> - ret = transport->push_refs(transport, remote_refs, flags);\n> + push_ret = transport->push_refs(transport, remote_refs, flags);\n> err = push_had_errors(remote_refs);\n> -\n> - ret |= err;\n> + ret = push_ret | err;\n>\n\nThe return code \"push_ret\" of push_refs() from different transport\nvtable is not consist. For HTTP protocol, \"push_ret\" is zero if no\nconnection error or no protocol errors. So we should consider\n“push_ret\" as a protocol error rather than a reference update error.\n\nIf we want to print \"Done\" in porcelain mode when there are no\nerrors. (In dry run mode, ref-update-errors should not be\nconsidered as errors, but the opposite should be regarded as errors).\n\nInstead of using the following code,\n\n> + if (porcelain && !push_ret)\n> + puts(\"Done\");\n\nWe can use like this (\"pretend\" is the flag for dry-run mode):\n\n+ ret = pretend ? push_ret : (push_ret | err);\n+ if (porcelain && !ret)\n+ puts(\"Done\");\n\nI will send patches follow this patch series.\n\n--\nJiang Xin\n"},{"id":"507283","messageId":"cover.1731603991.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"CANYiYbHGdJsTgdzJj1r4sPdTAcZCf3C-qA538fcXbD1jjB1BUw@mail.gmail.com","subject":"[PATCH v2 0/6] fix behaviors of git-push --porcelain","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-11-14T17:15:31Z","receivedAt":"2024-11-14T17:15:44Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen executing git-push(1) with the \"--porcelain\" flag, then we will\nprint updated references in a machine-readable format that looks\nlike this:\n\n To destination\n = refs/heads/noop:refs/heads/noop [up to date]\n ! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push\n failed)\n ! refs/heads/noff:refs/heads/(off (non-fast-forward)\n Done\n\nThe final \"Done\" stanza was introduced via 77555854be (git-push:\nmake git push --porcelain print \"Done\", 2010-02-26), with the\nfollowing behaviors:\n\n - Show a \"Done\" porcelain message if there are no errors.\n - Fail to update a ref in a --dry-run does not count as an error.\n - Actual rejections in non --dry-run pushes do count as errors.\n - Return a non-zero exit code if there are errors.\n\nThis patch series try to fix the broken behaviors introduced in\ncommit 7dcbeaa0df (send-pack: fix inconsistent porcelain output,\n2020-04-17).\n\n--\n\nJiang Xin (4):\n t5548: new test cases for push --porcelain and --dry-run\n push: fix the behavior of the Done message for porcelain\n push: only ignore finish_connect() for dry-run mode\n push: not send push-options to server with --dry-run\n\nPatrick Steinhardt (2):\n t5504: modernize test by moving heredocs into test bodies\n t5543: atomic push reports exit code failure\n\n send-pack.c | 13 +-\n .../test-0001-standard-git-push--porcelain.sh | 1 -\n ...st-0003-pre-receive-declined--porcelain.sh | 1 -\n t/t5411/test-0012-no-hook-error--porcelain.sh | 2 -\n t/t5411/test-0014-bad-protocol--porcelain.sh | 9 -\n t/t5411/test-0021-report-ng--porcelain.sh | 2 -\n ...est-0023-report-unexpect-ref--porcelain.sh | 1 -\n ...test-0025-report-unknown-ref--porcelain.sh | 1 -\n ...est-0033-report-with-options--porcelain.sh | 1 -\n .../test-0039-report-mixed-refs--porcelain.sh | 1 -\n t/t5504-fetch-receive-strict.sh | 34 +-\n t/t5516-fetch-push.sh | 3 +-\n t/t5534-push-signed.sh | 1 -\n t/t5541-http-push-smart.sh | 1 -\n t/t5543-atomic-push.sh | 30 ++\n t/t5548-push-porcelain.sh | 335 +++++++++++++-----\n transport.c | 21 +-\n 17 files changed, 316 insertions(+), 141 deletions(-)\n\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"507284","messageId":"7cedfd718c4f2d6fed7420a3341e82c9287d2085.1731603991.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v2 1/6] t5548: new test cases for push --porcelain and --dry-run","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-11-14T17:15:32Z","receivedAt":"2024-11-14T17:15:45Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nRefactor t5548 and add new test cases for git-push with both --porcelain\nand --dry-run flags in order to cover the issues reported by Patrick.\n\nWhen executing git-push(1) with the \"--porcelain\" flag, then we will\nprint updated references in a machine-readable format that looks like\nthis:\n\nTo destination\n= refs/heads/noop:refs/heads/noop [up to date]\n! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)\n! refs/heads/noff:refs/heads/(off (non-fast-forward)\nDone\n\nThe final \"Done\" stanza was introduced via 77555854be (git-push: make\ngit push --porcelain print \"Done\", 2010-02-26), where it was printed\n\"unless any errors have occurred\". For the purpose of the \"Done\" line,\nknowing a ref will be rejected in a --dry-run does not count as an\nerror. Actual rejections in non --dry-run pushes do count as errors.\n\nThe new test cases will be used in the next commit to protect the above\nbehaviors of porcelain output of git push.\n\nReported-by: Patrick Steinhardt \nSigned-off-by: Jiang Xin \n---\n t/t5548-push-porcelain.sh | 338 ++++++++++++++++++++++++++++----------\n 1 file changed, 250 insertions(+), 88 deletions(-)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex ecb3877aa4..ca5cf684bc 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -4,6 +4,9 @@\n #\n test_description='Test git push porcelain output'\n \n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n@@ -56,10 +59,10 @@ format_and_save_expect () {\n }\n \n setup_upstream_and_workbench () {\n-\t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n-\t# Workbench after setup : main(A)\n+\t# Upstream after setup: main(B) foo(A) bar(A) baz(A)\n+\t# Workbench after setup: main(A) baz(A) next(A)\n \ttest_expect_success \"setup upstream repository and workbench\" '\n-\t\trm -rf upstream.git workbench &&\n+\t\trm -rf upstream.git upstream-backup.git workbench &&\n \t\tgit init --bare upstream.git &&\n \t\tgit init workbench &&\n \t\tcreate_commits_in workbench A B &&\n@@ -68,19 +71,29 @@ setup_upstream_and_workbench () {\n \t\t\t# Try to make a stable fixed width for abbreviated commit ID,\n \t\t\t# this fixed-width oid will be replaced with \"\".\n \t\t\tgit config core.abbrev 7 &&\n-\t\t\tgit remote add origin ../upstream.git &&\n \t\t\tgit update-ref refs/heads/main $A &&\n+\t\t\tgit update-ref refs/heads/baz $A &&\n+\t\t\tgit update-ref refs/heads/next $A &&\n+\t\t\tgit remote add origin ../upstream.git &&\n \t\t\tgit push origin \\\n \t\t\t\t$B:refs/heads/main \\\n \t\t\t\t$A:refs/heads/foo \\\n \t\t\t\t$A:refs/heads/bar \\\n \t\t\t\t$A:refs/heads/baz\n \t\t) &&\n+\t\tgit clone --mirror upstream.git upstream-backup.git &&\n \t\tgit -C \"workbench\" config advice.pushUpdateRejected false &&\n \t\tupstream=upstream.git\n \t'\n }\n \n+restore_upstream () {\n+\ttest -n \"$upstream\" &&\n+\trm -rf \"$upstream\" &&\n+\tgit clone --mirror upstream-backup.git \"$upstream\" &&\n+\tgit -C \"$upstream\" config http.receivepack true\n+}\n+\n run_git_push_porcelain_output_test() {\n \tcase $1 in\n \thttp)\n@@ -96,19 +109,46 @@ run_git_push_porcelain_output_test() {\n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n-\ttest_expect_success \"porcelain output of successful git-push ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $A &&\n-\t\t\tgit update-ref refs/heads/baz $A &&\n-\t\t\tgit update-ref refs/heads/next $A &&\n-\t\t\tgit push --porcelain --force origin \\\n-\t\t\t\tmain \\\n-\t\t\t\t:refs/heads/foo \\\n-\t\t\t\t$B:bar \\\n-\t\t\t\tbaz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\ttest_expect_success \"git-push --porcelain --dry-run ($PROTOCOL)\" '\n+\t\ttest_must_fail git -C workbench push --porcelain --dry-run origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \"git-push --porcelain --dry-run --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --dry-run --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n \t\t> To \n@@ -121,39 +161,137 @@ run_git_push_porcelain_output_test() {\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \"git-push --porcelain --dry-run --atomic ($PROTOCOL)\" '\n+\t\ttest_must_fail git -C workbench push --porcelain --dry-run --atomic origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[rejected] (atomic push failed)\n+\t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \"git-push --porcelain --dry-run --atomic --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --dry-run --atomic --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \"git-push --porcelain ($PROTOCOL)\" '\n+\t\ttest_when_finished restore_upstream &&\n+\t\ttest_must_fail git -C workbench push --porcelain origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n \t\t refs/heads/bar\n \t\t refs/heads/baz\n-\t\t refs/heads/main\n+\t\t refs/heads/main\n \t\t refs/heads/next\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n \n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"atomic push failed ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $B &&\n-\t\t\tgit update-ref refs/heads/bar $A &&\n-\t\t\ttest_must_fail git push --atomic --porcelain origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \"git-push --porcelain --force ($PROTOCOL)\" '\n+\t\ttest_when_finished restore_upstream &&\n+\t\tgit -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[rejected] (non-fast-forward)\n-\t\t> !\t(delete):refs/heads/baz\t[rejected] (atomic push failed)\n-\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (atomic push failed)\n-\t\tDone\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n@@ -168,34 +306,60 @@ run_git_push_porcelain_output_test() {\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"prepare pre-receive hook ($PROTOCOL)\" '\n-\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF\n-\t\texit 1\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \"git push --porcelain --atomic ($PROTOCOL)\" '\n+\t\ttest_when_finished restore_upstream &&\n+\t\ttest_must_fail git -C workbench push --porcelain --atomic origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[rejected] (atomic push failed)\n+\t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n+\t\t> Done\n \t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n \t'\n \n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"pre-receive hook declined ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $B &&\n-\t\t\tgit update-ref refs/heads/bar $A &&\n-\t\t\ttest_must_fail git push --porcelain --force origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \"git push --porcelain --atomic --force ($PROTOCOL)\" '\n+\t\ttest_when_finished restore_upstream &&\n+\t\tgit -C workbench push --porcelain --atomic --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n-\t\t> !\t:refs/heads/baz\t[remote rejected] (pre-receive hook declined)\n-\t\t> !\trefs/heads/main:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n-\t\tDone\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n@@ -210,39 +374,39 @@ run_git_push_porcelain_output_test() {\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"remove pre-receive hook ($PROTOCOL)\" '\n-\t\trm \"$upstream/hooks/pre-receive\"\n-\t'\n-\n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"non-fastforward push ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\ttest_must_fail git push --porcelain origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \"pre-receive hook declined ($PROTOCOL)\" '\n+\t\ttest_when_finished restore_upstream &&\n+\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF &&\n+\t\t\texit 1\n+\t\tEOF\n+\t\ttest_must_fail git -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> -\t:refs/heads/baz\t[deleted]\n-\t\t> \trefs/heads/main:refs/heads/main\t..\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[rejected] (non-fast-forward)\n-\t\tDone\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\t:refs/heads/foo\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[remote rejected] (pre-receive hook declined)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n \t\t refs/heads/main\n-\t\t refs/heads/next\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n@@ -264,10 +428,8 @@ start_httpd\n setup_upstream_and_workbench\n \n test_expect_success \"setup for http\" '\n-\tgit -C upstream.git config http.receivepack true &&\n \tupstream=\"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\" &&\n-\tmv upstream.git \"$upstream\" &&\n-\n+\trestore_upstream &&\n \tgit -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git\n '\n \n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"507286","messageId":"9dfea9953c2a671600beebbaa0692a1e68afea34.1731603991.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v2 2/6] push: fix the behavior of the Done message for porcelain","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-11-14T17:15:33Z","receivedAt":"2024-11-14T17:15:46Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen executing git-push(1) with the \"--porcelain\" flag, then we will\nprint updated references in a machine-readable format that looks like\nthis:\n\nTo destination\n= refs/heads/noop:refs/heads/noop [up to date]\n! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)\n! refs/heads/noff:refs/heads/(off (non-fast-forward)\nDone\n\nThe final \"Done\" stanza was introduced via 77555854be (git-push: make\ngit push --porcelain print \"Done\", 2010-02-26), with the following\nbehaviors:\n\n - Show a \"Done\" porcelain message if there are no errors.\n - Fail to update a ref in a --dry-run does not count as an error.\n - Actual rejections in non --dry-run pushes do count as errors.\n - Return a non-zero exit code if there are errors.\n\nHowever, the behavior of the \"Done\" message is not consistent when\npushing with different protocols. This is because the return values of\ntransport->vtable->hush_refs() across different protocols are\ninconsistent. For the HTTP protocol, the return value is zero when\nthere are no connection errors or protocol errors. We should reference\nthe return code of push_had_errors() to check for failures in updating\nreferences. Since failing to update a reference in a --dry-run does not\ncount as an error, we should ignore the result of push_had_errors()\nwhen both --porcelain and --dry-run options are set.\n\nReported-by: Patrick Steinhardt \nSigned-off-by: Jiang Xin \n---\n send-pack.c | 11 ++++++-----\n t/t5411/test-0001-standard-git-push--porcelain.sh | 1 -\n .../test-0003-pre-receive-declined--porcelain.sh | 1 -\n t/t5411/test-0012-no-hook-error--porcelain.sh | 2 --\n t/t5411/test-0014-bad-protocol--porcelain.sh | 9 ---------\n t/t5411/test-0021-report-ng--porcelain.sh | 2 --\n .../test-0023-report-unexpect-ref--porcelain.sh | 1 -\n .../test-0025-report-unknown-ref--porcelain.sh | 1 -\n .../test-0033-report-with-options--porcelain.sh | 1 -\n t/t5411/test-0039-report-mixed-refs--porcelain.sh | 1 -\n t/t5504-fetch-receive-strict.sh | 1 -\n t/t5516-fetch-push.sh | 3 +--\n t/t5534-push-signed.sh | 1 -\n t/t5541-http-push-smart.sh | 1 -\n t/t5548-push-porcelain.sh | 7 ++-----\n transport.c | 15 +++++++++++++--\n 16 files changed, 22 insertions(+), 36 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 6677c44e8a..4845f63737 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n-\t\t\t\tret = args->porcelain ? 0 : -1;\n+\t\t\t\tret = (args->porcelain && args->dry_run) ? 0 : -1;\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\n@@ -760,11 +760,12 @@ int send_pack(struct send_pack_args *args,\n \n \tif (ret < 0)\n \t\tgoto out;\n-\n-\tif (args->porcelain) {\n-\t\tret = 0;\n+\telse if (args->porcelain && args->dry_run)\n+\t\t/*\n+\t\t * Knowing a ref will be rejected in a --dry-run does not\n+\t\t * count as an error.\n+\t\t */\n \t\tgoto out;\n-\t}\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\ndiff --git a/t/t5411/test-0001-standard-git-push--porcelain.sh b/t/t5411/test-0001-standard-git-push--porcelain.sh\nindex 373ec3d865..5ff901454a 100644\n--- a/t/t5411/test-0001-standard-git-push--porcelain.sh\n+++ b/t/t5411/test-0001-standard-git-push--porcelain.sh\n@@ -73,7 +73,6 @@ test_expect_success \"non-fast-forward git-push ($PROTOCOL/porcelain)\" '\n \t> To \n \t> \t:refs/heads/next\t..\n \t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \ndiff --git a/t/t5411/test-0003-pre-receive-declined--porcelain.sh b/t/t5411/test-0003-pre-receive-declined--porcelain.sh\nindex 67ca6dc4f8..f4cdf9db42 100644\n--- a/t/t5411/test-0003-pre-receive-declined--porcelain.sh\n+++ b/t/t5411/test-0003-pre-receive-declined--porcelain.sh\n@@ -18,7 +18,6 @@ test_expect_success \"git-push is declined ($PROTOCOL/porcelain)\" '\n \t> To \n \t> !\t:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n \t> !\tHEAD:refs/heads/next\t[remote rejected] (pre-receive hook declined)\n-\tDone\n \tEOF\n \ttest_cmp expect actual &&\n \ndiff --git a/t/t5411/test-0012-no-hook-error--porcelain.sh b/t/t5411/test-0012-no-hook-error--porcelain.sh\nindex 04468b5018..563de67859 100644\n--- a/t/t5411/test-0012-no-hook-error--porcelain.sh\n+++ b/t/t5411/test-0012-no-hook-error--porcelain.sh\n@@ -17,7 +17,6 @@ test_expect_success \"proc-receive: no hook, fail to push special ref ($PROTOCOL/\n \t> To \n \t> *\tHEAD:refs/heads/next\t[new branch]\n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (fail to run proc-receive hook)\n-\tDone\n \tEOF\n \ttest_cmp expect actual &&\n \n@@ -52,7 +51,6 @@ test_expect_success \"proc-receive: no hook, all failed for atomic push ($PROTOCO\n \t> !\t:refs/heads/main\t[remote rejected] (fail to run proc-receive hook)\n \t> !\tHEAD:refs/heads/next\t[remote rejected] (fail to run proc-receive hook)\n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (fail to run proc-receive hook)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \ndiff --git a/t/t5411/test-0014-bad-protocol--porcelain.sh b/t/t5411/test-0014-bad-protocol--porcelain.sh\nindex 298a3d1fec..096f13a832 100644\n--- a/t/t5411/test-0014-bad-protocol--porcelain.sh\n+++ b/t/t5411/test-0014-bad-protocol--porcelain.sh\n@@ -21,7 +21,6 @@ test_expect_success \"proc-receive: bad protocol (unknown version, $PROTOCOL/porc\n \tcat >expect <<-EOF &&\n \tTo \n \t!\tHEAD:refs/for/main/topic\t[remote rejected] (fail to run proc-receive hook)\n-\tDone\n \tEOF\n \ttest_cmp expect actual-report &&\n \n@@ -59,7 +58,6 @@ test_expect_success \"proc-receive: bad protocol (hook --die-read-version, $PROTO\n \tcat >expect <<-EOF &&\n \tTo \n \t!\tHEAD:refs/for/main/topic\t[remote rejected] (fail to run proc-receive hook)\n-\tDone\n \tEOF\n \ttest_cmp expect actual &&\n \tgrep \"remote: fatal: die with the --die-read-version option\" out-$test_count &&\n@@ -90,7 +88,6 @@ test_expect_success \"proc-receive: bad protocol (hook --die-write-version, $PROT\n \tcat >expect <<-EOF &&\n \tTo \n \t!\tHEAD:refs/for/main/topic\t[remote rejected] (fail to run proc-receive hook)\n-\tDone\n \tEOF\n \ttest_cmp expect actual &&\n \tgrep \"remote: fatal: die with the --die-write-version option\" out-$test_count &&\n@@ -121,7 +118,6 @@ test_expect_success \"proc-receive: bad protocol (hook --die-read-commands, $PROT\n \tcat >expect <<-EOF &&\n \tTo \n \t!\tHEAD:refs/for/main/topic\t[remote rejected] (fail to run proc-receive hook)\n-\tDone\n \tEOF\n \ttest_cmp expect actual &&\n \tgrep \"remote: fatal: die with the --die-read-commands option\" out-$test_count &&\n@@ -153,7 +149,6 @@ test_expect_success \"proc-receive: bad protocol (hook --die-read-push-options, $\n \tcat >expect <<-EOF &&\n \tTo \n \t!\tHEAD:refs/for/main/topic\t[remote rejected] (fail to run proc-receive hook)\n-\tDone\n \tEOF\n \ttest_cmp expect actual &&\n \tgrep \"remote: fatal: die with the --die-read-push-options option\" out-$test_count &&\n@@ -183,7 +178,6 @@ test_expect_success \"proc-receive: bad protocol (hook --die-write-report, $PROTO\n \tcat >expect <<-EOF &&\n \tTo \n \t!\tHEAD:refs/for/main/topic\t[remote rejected] (fail to run proc-receive hook)\n-\tDone\n \tEOF\n \ttest_cmp expect actual &&\n \tgrep \"remote: fatal: die with the --die-write-report option\" out-$test_count &&\n@@ -219,7 +213,6 @@ test_expect_success \"proc-receive: bad protocol (no report, $PROTOCOL/porcelain)\n \t> To \n \t> *\tHEAD:refs/heads/next\t[new branch]\n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (proc-receive failed to report status)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \n@@ -260,7 +253,6 @@ test_expect_success \"proc-receive: bad protocol (no ref, $PROTOCOL/porcelain)\" '\n \t> remote: error: proc-receive reported incomplete status line: \"ok\" Z\n \t> To \n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (proc-receive failed to report status)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \n@@ -294,7 +286,6 @@ test_expect_success \"proc-receive: bad protocol (unknown status, $PROTOCOL/porce\n \t> remote: error: proc-receive reported bad status \"xx\" on ref \"refs/for/main/topic\" Z\n \t> To \n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (proc-receive failed to report status)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \ndiff --git a/t/t5411/test-0021-report-ng--porcelain.sh b/t/t5411/test-0021-report-ng--porcelain.sh\nindex 502b34fe3d..c4b1d25562 100644\n--- a/t/t5411/test-0021-report-ng--porcelain.sh\n+++ b/t/t5411/test-0021-report-ng--porcelain.sh\n@@ -22,7 +22,6 @@ test_expect_success \"proc-receive: fail to update (ng, no message, $PROTOCOL/por\n \t> remote: proc-receive> ng refs/for/main/topic Z\n \t> To \n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (failed)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \n@@ -55,7 +54,6 @@ test_expect_success \"proc-receive: fail to update (ng, with message, $PROTOCOL/p\n \t> remote: proc-receive> ng refs/for/main/topic error msg Z\n \t> To \n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (error msg)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \ndiff --git a/t/t5411/test-0023-report-unexpect-ref--porcelain.sh b/t/t5411/test-0023-report-unexpect-ref--porcelain.sh\nindex 6d116ef692..d224e2400e 100644\n--- a/t/t5411/test-0023-report-unexpect-ref--porcelain.sh\n+++ b/t/t5411/test-0023-report-unexpect-ref--porcelain.sh\n@@ -28,7 +28,6 @@ test_expect_success \"proc-receive: report unexpected ref ($PROTOCOL/porcelain)\"\n \t> To \n \t> \t:refs/heads/main\t..\n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (proc-receive failed to report status)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \ndiff --git a/t/t5411/test-0025-report-unknown-ref--porcelain.sh b/t/t5411/test-0025-report-unknown-ref--porcelain.sh\nindex 8b3f5d05a3..278fc597eb 100644\n--- a/t/t5411/test-0025-report-unknown-ref--porcelain.sh\n+++ b/t/t5411/test-0025-report-unknown-ref--porcelain.sh\n@@ -23,7 +23,6 @@ test_expect_success \"proc-receive: report unknown reference ($PROTOCOL/porcelain\n \t> remote: error: proc-receive reported status on unknown ref: refs/for/main/topic Z\n \t> To \n \t> !\tHEAD:refs/for/a/b/c/my/topic\t[remote rejected] (proc-receive failed to report status)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \ndiff --git a/t/t5411/test-0033-report-with-options--porcelain.sh b/t/t5411/test-0033-report-with-options--porcelain.sh\nindex 2e1831b104..2c1117457f 100644\n--- a/t/t5411/test-0033-report-with-options--porcelain.sh\n+++ b/t/t5411/test-0033-report-with-options--porcelain.sh\n@@ -25,7 +25,6 @@ test_expect_success \"proc-receive: report option without matching ok ($PROTOCOL/\n \t> remote: error: proc-receive reported \"option\" without a matching \"ok/ng\" directive Z\n \t> To \n \t> !\tHEAD:refs/for/main/topic\t[remote rejected] (proc-receive failed to report status)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual\n '\ndiff --git a/t/t5411/test-0039-report-mixed-refs--porcelain.sh b/t/t5411/test-0039-report-mixed-refs--porcelain.sh\nindex 40f4c5b1af..e1b64edea9 100644\n--- a/t/t5411/test-0039-report-mixed-refs--porcelain.sh\n+++ b/t/t5411/test-0039-report-mixed-refs--porcelain.sh\n@@ -63,7 +63,6 @@ test_expect_success \"proc-receive: report update of mixed refs ($PROTOCOL/porcel\n \t> \tHEAD:refs/for/main/topic\t..\n \t> !\tHEAD:refs/for/next/topic1\t[remote rejected] (fail to call Web API)\n \t> !\tHEAD:refs/for/next/topic3\t[remote rejected] (proc-receive failed to report status)\n-\t> Done\n \tEOF\n \ttest_cmp expect actual &&\n \ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 138e6778a4..bf33cc69d0 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -68,7 +68,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n cat >exp <.git/foo \"To testrepo\" &&\n \techo >>.git/foo \"!\trefs/heads/main:refs/heads/main\t[remote rejected] (branch is currently checked out)\" &&\n-\techo >>.git/foo \"Done\" &&\n \n \ttest_must_fail git push >.git/bar --porcelain testrepo refs/heads/main:refs/heads/main &&\n \ttest_cmp .git/foo .git/bar\n@@ -1207,7 +1206,7 @@ test_expect_success 'push --porcelain --dry-run rejected' '\n \techo >>.git/foo \"!\trefs/heads/main^:refs/heads/main\t[rejected] (non-fast-forward)\" &&\n \techo >>.git/foo \"Done\" &&\n \n-\ttest_must_fail git push >.git/bar --porcelain --dry-run testrepo refs/heads/main^:refs/heads/main &&\n+\tgit push >.git/bar --porcelain --dry-run testrepo refs/heads/main^:refs/heads/main &&\n \ttest_cmp .git/foo .git/bar\n '\n \ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex d43aee0c32..2e40bad53d 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -385,7 +385,6 @@ test_expect_success GPG 'failed atomic push does not execute GPG' '\n \t=\trefs/heads/noop:refs/heads/noop\t[up to date]\n \t!\trefs/heads/ff:refs/heads/ff\t[rejected] (atomic push failed)\n \t!\trefs/heads/noff:refs/heads/noff\t[rejected] (non-fast-forward)\n-\tDone\n \tEOF\n \ttest_cmp expect out\n '\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 3ad514bbd4..b543c10314 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -499,7 +499,6 @@ test_expect_success 'report error server does not provide ref status' '\n \tcat >expect <<-EOF &&\n \tTo $HTTPD_URL/smart/no_report\n \t!\tHEAD:refs/tags/will-fail\t[remote failure] (remote failed to report status)\n-\tDone\n \tEOF\n \ttest_cmp expect actual\n '\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex ca5cf684bc..424391eadd 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -110,7 +110,7 @@ run_git_push_porcelain_output_test() {\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n \ttest_expect_success \"git-push --porcelain --dry-run ($PROTOCOL)\" '\n-\t\ttest_must_fail git -C workbench push --porcelain --dry-run origin \\\n+\t\tgit -C workbench push --porcelain --dry-run origin \\\n \t\t\tmain \\\n \t\t\t:refs/heads/foo \\\n \t\t\t$B:bar \\\n@@ -176,7 +176,7 @@ run_git_push_porcelain_output_test() {\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n \ttest_expect_success \"git-push --porcelain --dry-run --atomic ($PROTOCOL)\" '\n-\t\ttest_must_fail git -C workbench push --porcelain --dry-run --atomic origin \\\n+\t\tgit -C workbench push --porcelain --dry-run --atomic origin \\\n \t\t\tmain \\\n \t\t\t:refs/heads/foo \\\n \t\t\t$B:bar \\\n@@ -257,7 +257,6 @@ run_git_push_porcelain_output_test() {\n \t\t> -\t:refs/heads/foo\t[deleted]\n \t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n \t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n-\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n@@ -325,7 +324,6 @@ run_git_push_porcelain_output_test() {\n \t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n \t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n \t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n-\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n@@ -396,7 +394,6 @@ run_git_push_porcelain_output_test() {\n \t\t> !\t:refs/heads/foo\t[remote rejected] (pre-receive hook declined)\n \t\t> !\trefs/heads/main:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n \t\t> !\trefs/heads/next:refs/heads/next\t[remote rejected] (pre-receive hook declined)\n-\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \ndiff --git a/transport.c b/transport.c\nindex 47fda6a773..9e03a7148c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1486,7 +1486,18 @@ int transport_push(struct repository *r,\n \t} else\n \t\tpush_ret = 0;\n \terr = push_had_errors(remote_refs);\n-\tret = push_ret | err;\n+\t/*\n+\t * The return values of transport->vtable->hush_refs() across\n+\t * different protocols are inconsistent. For the HTTP protocol,\n+\t * the return value is zero when there are no connection errors\n+\t * or protocol errors. We should reference the return code of\n+\t * push_had_errors() to check for failures in updating references.\n+\t * Since failing to update a reference in a --dry-run does not\n+\t * count as an error, we could ignore the result of\n+\t * push_had_errors() when both --porcelain and --dry-run options\n+\t * are set.\n+\t */\n+\tret = (porcelain && pretend) ? push_ret : (push_ret | err);\n \n \tif (!quiet || err)\n \t\ttransport_print_push_status(transport->url, remote_refs,\n@@ -1503,7 +1514,7 @@ int transport_push(struct repository *r,\n \t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n \t}\n \n-\tif (porcelain && !push_ret)\n+\tif (porcelain && !ret)\n \t\tputs(\"Done\");\n \telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n \t\t/* stable plumbing output; do not modify or localize */\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"507285","messageId":"1ab87d3b30399200e0fb440378b63e634bdbee2d.1731603991.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v2 3/6] t5504: modernize test by moving heredocs into test bodies","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-11-14T17:15:34Z","receivedAt":"2024-11-14T17:15:47Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Patrick Steinhardt \n\nWe have several heredocs in t5504 located outside of any particular test\nbodies. Move these into the test bodies to match our modern coding\nstyle.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5504-fetch-receive-strict.sh | 33 +++++++++++++++------------------\n 1 file changed, 15 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex bf33cc69d0..56b31cf557 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -65,11 +65,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n \t)\n '\n \n-cat >exp <exp <<-EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (missing necessary objects)\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '\n \ttest_cmp exp act\n '\n \n-cat >exp <exp <<-EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (unpacker error)\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -129,15 +127,14 @@ test_expect_success 'repair the \"corrupt or missing\" object' '\n \tgit fsck\n '\n \n-cat >bogus-commit < 1234567890 +0000\n-\n-This commit object intentionally broken\n-EOF\n-\n test_expect_success 'setup bogus commit' '\n+\tcat >bogus-commit <<-EOF &&\n+\ttree $EMPTY_TREE\n+\tauthor Bugs Bunny 1234567890 +0000\n+\tcommitter Bugs Bunny 1234567890 +0000\n+\n+\tThis commit object intentionally broken\n+\tEOF\n \tcommit=\"$(git hash-object --literally -t commit -w --stdin \n\nAdd new test cases in t5543 to avoid ignoring the exit code of\ngit-receive-pack(1) during atomic push with \"--porcelain\" flag.\n\nWe'd typically notice this case because the refs would have their error\nmessage set. But there is an edge case when pushing refs succeeds, but\ngit-receive-pack(1) exits with a non-zero exit code at a later point in\ntime due to another error. An atomic git-push(1) would ignore that error\ncode, and consequently it would return successfully and not print any\nerror message at all.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 479d103469..a40afb949b 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -281,4 +281,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'atomic push reports exit code failure' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict 2>err &&\n+\tcat >expect <<-EOF &&\n+\tTo ../upstream\n+\t * [new branch] HEAD -> no-conflict\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure 'atomic push reports exit code failure with porcelain' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic --porcelain \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict-porcelain 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"507288","messageId":"1ad61a02955fca35c4d1763adfbac3fd36482e09.1731603991.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v2 5/6] push: only ignore finish_connect() for dry-run mode","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-11-14T17:15:36Z","receivedAt":"2024-11-14T17:15:48Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nPatrick reported an issue that the exit code of git-receive-pack(1) is\nignored during atomic push with \"--porcelain\" flag, and added new test\ncases in t5543.\n\nAtomic push may abort the connection early and close the pipe, which\nmay cause an error for `finish_connect()`. Arbitrarily ignoring this\nerror will lead to issues. Modify it to only ignore the finish_connect()\nerror when both the --atomic and --dry-run flags are set.\n\nSigned-off-by: Jiang Xin \n---\n t/t5543-atomic-push.sh | 4 ++--\n transport.c | 6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex a40afb949b..1d8f088a00 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -281,7 +281,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'atomic push reports exit code failure' '\n+test_expect_success 'atomic push reports exit code failure' '\n \twrite_script receive-pack-wrapper <<-\\EOF &&\n \tgit-receive-pack \"$@\"\n \texit 1\n@@ -297,7 +297,7 @@ test_expect_failure 'atomic push reports exit code failure' '\n \ttest_cmp expect err\n '\n \n-test_expect_failure 'atomic push reports exit code failure with porcelain' '\n+test_expect_success 'atomic push reports exit code failure with porcelain' '\n \twrite_script receive-pack-wrapper <<-\\EOF &&\n \tgit-receive-pack \"$@\"\n \texit 1\ndiff --git a/transport.c b/transport.c\nindex 9e03a7148c..186d58e907 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -923,10 +923,10 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tclose(data->fd[0]);\n \t/*\n \t * Atomic push may abort the connection early and close the pipe,\n-\t * which may cause an error for `finish_connect()`. Ignore this error\n-\t * for atomic git-push.\n+\t * which may cause an error for `finish_connect()`. We can ignore\n+\t * this error when both `--atomic` and `--dry-run` flags provided.\n \t */\n-\tif (ret || args.atomic)\n+\tif (ret || (args.atomic && args.dry_run))\n \t\tfinish_connect(data->conn);\n \telse\n \t\tret = finish_connect(data->conn);\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"507289","messageId":"9d11105d0d0ef5717c6c691a3648a1c1bd1aa988.1731603991.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v2 6/6] push: not send push-options to server with --dry-run","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-11-14T17:15:37Z","receivedAt":"2024-11-14T17:15:49Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nSigned-off-by: Jiang Xin \n---\n send-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 4845f63737..89a2d47928 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -678,7 +678,7 @@ int send_pack(struct send_pack_args *args,\n \t\t}\n \t}\n \n-\tif (use_push_options) {\n+\tif (use_push_options && !args->dry_run) {\n \t\tstruct string_list_item *item;\n \n \t\tpacket_buf_flush(&req_buf);\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"507299","messageId":"xmqqsert5qx7.fsf@gitster.g","threadId":"62487","inReplyTo":"CANYiYbHGdJsTgdzJj1r4sPdTAcZCf3C-qA538fcXbD1jjB1BUw@mail.gmail.com","subject":"Re: [PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-14T23:36:20Z","receivedAt":"2024-11-14T23:36:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> If we want to print \"Done\" in porcelain mode when there are no\n> errors. (In dry run mode, ref-update-errors should not be\n> considered as errors, but the opposite should be regarded as errors).\n\nHmph, should we allow dry-run to deliberately behave differently\nfrom the for-real execution? Wouldn't it discard the primary value\nof dry-run, to see if it is likely that an operation will succeed\n(or fail)?\n"},{"id":"508052","messageId":"Z0Q0sSsUXg47gn6Y@pks.im","threadId":"62487","inReplyTo":"7cedfd718c4f2d6fed7420a3341e82c9287d2085.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v2 1/6] t5548: new test cases for push --porcelain and --dry-run","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T08:26:25Z","receivedAt":"2024-11-25T08:26:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 15, 2024 at 01:15:32AM +0800, Jiang Xin wrote:\n> From: Jiang Xin \n> \n> Refactor t5548 and add new test cases for git-push with both --porcelain\n> and --dry-run flags in order to cover the issues reported by Patrick.\n> \n> When executing git-push(1) with the \"--porcelain\" flag, then we will\n> print updated references in a machine-readable format that looks like\n> this:\n> \n> To destination\n> = refs/heads/noop:refs/heads/noop [up to date]\n> ! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)\n> ! refs/heads/noff:refs/heads/(off (non-fast-forward)\n> Done\n> \n> The final \"Done\" stanza was introduced via 77555854be (git-push: make\n> git push --porcelain print \"Done\", 2010-02-26), where it was printed\n> \"unless any errors have occurred\". For the purpose of the \"Done\" line,\n> knowing a ref will be rejected in a --dry-run does not count as an\n> error. Actual rejections in non --dry-run pushes do count as errors.\n> \n> The new test cases will be used in the next commit to protect the above\n> behaviors of porcelain output of git push.\n\nIt's a bit hard to make sense of this commit: does it introduce new\ntests? Does it refactor existing tests? Does it change the test setup?\nIt seems to be a mixture of all of these, which makes it hard to see\nwhat the actual change is.\n\nI'd propose to split up this commit into multiple ones: one that\nintroduces `restore_upstream()`, one that adapts the test data, and one\nthat introduces new tests.\n\nPatrick\n"},{"id":"508053","messageId":"Z0Q0tY8y58yNivFR@pks.im","threadId":"62487","inReplyTo":"9dfea9953c2a671600beebbaa0692a1e68afea34.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v2 2/6] push: fix the behavior of the Done message for porcelain","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T08:26:29Z","receivedAt":"2024-11-25T08:26:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 15, 2024 at 01:15:33AM +0800, Jiang Xin wrote:\n> From: Jiang Xin \n> \n> When executing git-push(1) with the \"--porcelain\" flag, then we will\n> print updated references in a machine-readable format that looks like\n> this:\n> \n> To destination\n> = refs/heads/noop:refs/heads/noop [up to date]\n> ! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)\n> ! refs/heads/noff:refs/heads/(off (non-fast-forward)\n> Done\n> \n> The final \"Done\" stanza was introduced via 77555854be (git-push: make\n> git push --porcelain print \"Done\", 2010-02-26), with the following\n> behaviors:\n> \n> - Show a \"Done\" porcelain message if there are no errors.\n\nIt would be great to update our documentation so that we spell out when\nexactly the user can expect such a \"Done\" message.\n\n> - Fail to update a ref in a --dry-run does not count as an error.\n> - Actual rejections in non --dry-run pushes do count as errors.\n> - Return a non-zero exit code if there are errors.\n\n> However, the behavior of the \"Done\" message is not consistent when\n> pushing with different protocols. This is because the return values of\n> transport->vtable->hush_refs() across different protocols are\n\ns/hush_refs/push_refs/\n\n> diff --git a/send-pack.c b/send-pack.c\n> index 6677c44e8a..4845f63737 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,\n> \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n> \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n> \t\t\t\t ref->name, ref->status);\n> -\t\t\t\tret = args->porcelain ? 0 : -1;\n> +\t\t\t\tret = (args->porcelain && args->dry_run) ? 0 : -1;\n> \t\t\t\tgoto out;\n> \t\t\t}\n> \t\t\t/* else fallthrough */\n\nThis feels weird to me. Why would we ignore an error with `--dry-run`?\nDoesn't that mean that the resulting behaviour is now different\ndepending on whether or not you pass `--dry-run`?\n\n> @@ -760,11 +760,12 @@ int send_pack(struct send_pack_args *args,\n> \n> \tif (ret < 0)\n> \t\tgoto out;\n> -\n> -\tif (args->porcelain) {\n> -\t\tret = 0;\n> +\telse if (args->porcelain && args->dry_run)\n> +\t\t/*\n> +\t\t * Knowing a ref will be rejected in a --dry-run does not\n> +\t\t * count as an error.\n> +\t\t */\n> \t\tgoto out;\n> -\t}\n> \n> \tfor (ref = remote_refs; ref; ref = ref->next) {\n> \t\tswitch (ref->status) {\n\nSame question here.\n\n> diff --git a/t/t5411/test-0001-standard-git-push--porcelain.sh b/t/t5411/test-0001-standard-git-push--porcelain.sh\n> index 373ec3d865..5ff901454a 100644\n> --- a/t/t5411/test-0001-standard-git-push--porcelain.sh\n> +++ b/t/t5411/test-0001-standard-git-push--porcelain.sh\n> @@ -73,7 +73,6 @@ test_expect_success \"non-fast-forward git-push ($PROTOCOL/porcelain)\" '\n> \t> To \n> \t> \t:refs/heads/next\t..\n> \t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n> -\t> Done\n> \tEOF\n> \ttest_cmp expect actual &&\n\n77555854be (git-push: make git push --porcelain print \"Done\",\n2010-02-26) seems to cover this case, where it mentions that a\nconflicting ref update does not count as error with \"--dry-run\", but\ndoes count as error without that option. And this change here seems to\nbe in line with that.\n\nWhether the documented behaviour of that commit is reasonable is a\ndifferent question though. I find it to be of dubious nature to make any\nruntime behaviour (other than committing the actual change) depend on\n\"--dry-run\". The behaviour should be the exact same, as otherwise the\ndry run becomes useless if the behaviour differs.\n\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 331778bd42..b133ab6ffc 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1207,7 +1206,7 @@ test_expect_success 'push --porcelain --dry-run rejected' '\n> \techo >>.git/foo \"!\trefs/heads/main^:refs/heads/main\t[rejected] (non-fast-forward)\" &&\n> \techo >>.git/foo \"Done\" &&\n> \n> -\ttest_must_fail git push >.git/bar --porcelain --dry-run testrepo refs/heads/main^:refs/heads/main &&\n> +\tgit push >.git/bar --porcelain --dry-run testrepo refs/heads/main^:refs/heads/main &&\n> \ttest_cmp .git/foo .git/bar\n> '\n\nHuh. So this is the dubious part I mean. It seems like the push would\nfail without \"--dry-run\", but because we pass it it does not indicate\nthat failure via the exit code. This behaviour does not make any sense\nto me. There are more cases in your change where we stop failing now.\n\n> diff --git a/transport.c b/transport.c\n> index 47fda6a773..9e03a7148c 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1486,7 +1486,18 @@ int transport_push(struct repository *r,\n> \t} else\n> \t\tpush_ret = 0;\n> \terr = push_had_errors(remote_refs);\n> -\tret = push_ret | err;\n> +\t/*\n> +\t * The return values of transport->vtable->hush_refs() across\n\ns/hush_refs/push_refs/\n\n> +\t * different protocols are inconsistent. For the HTTP protocol,\n> +\t * the return value is zero when there are no connection errors\n> +\t * or protocol errors. We should reference the return code of\n> +\t * push_had_errors() to check for failures in updating references.\n> +\t * Since failing to update a reference in a --dry-run does not\n> +\t * count as an error, we could ignore the result of\n> +\t * push_had_errors() when both --porcelain and --dry-run options\n> +\t * are set.\n> +\t */\n> +\tret = (porcelain && pretend) ? push_ret : (push_ret | err);\n> \n> \tif (!quiet || err)\n> \t\ttransport_print_push_status(transport->url, remote_refs,\n> @@ -1503,7 +1514,7 @@ int transport_push(struct repository *r,\n> \t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n> \t}\n> \n> -\tif (porcelain && !push_ret)\n> +\tif (porcelain && !ret)\n> \t\tputs(\"Done\");\n> \telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n> \t\t/* stable plumbing output; do not modify or localize */\n\nI'm not yet convinced that it makes sense to ignore either `ret` or\n`push_ret` in the above two hunks. Making this dependent on `porcelain`\nmay be okay if we want to report errors via the output, but making the\nbehaviour dependent on whether or not we perform a dry run continues to\nfeel wrong to me.\n\nPatrick\n"},{"id":"508054","messageId":"Z0Q0ud0SJ8ZeEvXx@pks.im","threadId":"62487","inReplyTo":"1ad61a02955fca35c4d1763adfbac3fd36482e09.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v2 5/6] push: only ignore finish_connect() for dry-run mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T08:26:33Z","receivedAt":"2024-11-25T08:26:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 15, 2024 at 01:15:36AM +0800, Jiang Xin wrote:\n> diff --git a/transport.c b/transport.c\n> index 9e03a7148c..186d58e907 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -923,10 +923,10 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n> \tclose(data->fd[0]);\n> \t/*\n> \t * Atomic push may abort the connection early and close the pipe,\n> -\t * which may cause an error for `finish_connect()`. Ignore this error\n> -\t * for atomic git-push.\n> +\t * which may cause an error for `finish_connect()`. We can ignore\n> +\t * this error when both `--atomic` and `--dry-run` flags provided.\n> \t */\n> -\tif (ret || args.atomic)\n> +\tif (ret || (args.atomic && args.dry_run))\n> \t\tfinish_connect(data->conn);\n> \telse\n> \t\tret = finish_connect(data->conn);\n\nIn the same spirit as mentioned in other commits I don't think it is a\ngood idea to condition this behaviour on \"--dry-run\".\n\nPatrick\n"},{"id":"508055","messageId":"Z0Q0vnTuo6dQDOox@pks.im","threadId":"62487","inReplyTo":"9d11105d0d0ef5717c6c691a3648a1c1bd1aa988.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v2 6/6] push: not send push-options to server with --dry-run","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T08:26:38Z","receivedAt":"2024-11-25T08:26:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 15, 2024 at 01:15:37AM +0800, Jiang Xin wrote:\n> From: Jiang Xin \n> \n> Signed-off-by: Jiang Xin \n\nIt would be nice to have a summary of what this change intends to do and\nwhy that is a good idea.\n\nPatrick\n"},{"id":"508056","messageId":"Z0Q0wfSTk_cUXH0F@pks.im","threadId":"62487","inReplyTo":"xmqqsert5qx7.fsf@gitster.g","subject":"Re: [PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T08:26:41Z","receivedAt":"2024-11-25T08:26:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 15, 2024 at 08:36:20AM +0900, Junio C Hamano wrote:\n> Jiang Xin writes:\n> \n> > If we want to print \"Done\" in porcelain mode when there are no\n> > errors. (In dry run mode, ref-update-errors should not be\n> > considered as errors, but the opposite should be regarded as errors).\n> \n> Hmph, should we allow dry-run to deliberately behave differently\n> from the for-real execution? Wouldn't it discard the primary value\n> of dry-run, to see if it is likely that an operation will succeed\n> (or fail)?\n\nI agree, I don't think that this is a good idea. \"--dry-run\" should\nbehave as close as possible to the same command executed without that\nflag, the only change should be that the underlying operation is not\nactually applied in the end. But other than that both the output and the\nreturn values should ideally match exactly so that the user can actually\ntell what would happen.\n\nPatrick\n"},{"id":"508524","messageId":"CANYiYbEoa4fJ4TmQVf+brEk74b3-mY13dxtFVgvTvtOD8TO++w@mail.gmail.com","threadId":"62487","inReplyTo":"Z0Q0sSsUXg47gn6Y@pks.im","subject":"Re: [PATCH v2 1/6] t5548: new test cases for push --porcelain and --dry-run","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-03T11:52:56Z","receivedAt":"2024-12-03T11:53:09Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Mon, Nov 25, 2024 at 4:26 PM Patrick Steinhardt wrote:\n>\n> On Fri, Nov 15, 2024 at 01:15:32AM +0800, Jiang Xin wrote:\n> > From: Jiang Xin \n> >\n> > Refactor t5548 and add new test cases for git-push with both --porcelain\n> > and --dry-run flags in order to cover the issues reported by Patrick.\n> >\n> > When executing git-push(1) with the \"--porcelain\" flag, then we will\n> > print updated references in a machine-readable format that looks like\n> > this:\n> >\n> > To destination\n> > = refs/heads/noop:refs/heads/noop [up to date]\n> > ! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)\n> > ! refs/heads/noff:refs/heads/(off (non-fast-forward)\n> > Done\n> >\n> > The final \"Done\" stanza was introduced via 77555854be (git-push: make\n> > git push --porcelain print \"Done\", 2010-02-26), where it was printed\n> > \"unless any errors have occurred\". For the purpose of the \"Done\" line,\n> > knowing a ref will be rejected in a --dry-run does not count as an\n> > error. Actual rejections in non --dry-run pushes do count as errors.\n> >\n> > The new test cases will be used in the next commit to protect the above\n> > behaviors of porcelain output of git push.\n>\n> It's a bit hard to make sense of this commit: does it introduce new\n> tests? Does it refactor existing tests? Does it change the test setup?\n> It seems to be a mixture of all of these, which makes it hard to see\n> what the actual change is.\n>\n> I'd propose to split up this commit into multiple ones: one that\n> introduces `restore_upstream()`, one that adapts the test data, and one\n> that introduces new tests.\n\nI've been quite busy lately, but I plan to revisit this next week,\nincorporating both your and Junio's suggestions. Thank you for your\ninput!\n\n>\n> Patrick\n"},{"id":"508906","messageId":"cover.1733830410.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1731603991.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 0/8] fix behaviors of git-push --porcelain","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T11:36:20Z","receivedAt":"2024-12-10T11:36:35Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nPatrick reported an issue that the exit code of git-receive-pack(1)\nis ignored during atomic push with \"--porcelain\" flag, and added new\ntest cases in t5543.\n\nThis issue originated from commit 7dcbeaa0df (send-pack: fix\ninconsistent porcelain output, 2020-04-17). At that time, I chose to\nignore the exit code of \"finish_connect()\" without investigating the\nroot cause of the abnormal termination of git-receive-pack. That was an\nincorrect solution.\n\nThe root cause is that an atomic push operation terminates early without\nsending a flush packet to git-receive-pack. As a result,\ngit-receive-pack continues waiting for commands without exiting. By\nsending a flush packet at the appropriate location in \"send_pack()\", we\nensure that the git-receive-pack process closes properly, avoiding an\nerroneous exit code for git-push.\n\n\n# Changes since v2\n\n* Split the changes made to t5548 into several commits for clarity.\n\n* Correct the inconsistent return code of \"push_refs()\" across different\n transports to ensure that git push --porcelain behaves consistently.\n\n* Gracefully close the connection for early exist of atomic push to\n resolve to root cause of wrong exit code of atomic git-push.\n\n--\n\nJiang Xin (6):\n t5548: refactor to reuse setup_upstream() function\n t5548: refactor test cases by resetting upstream\n t5548: add new porcelain test cases\n t5548: add porcelain push test cases for dry-run mode\n send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"\n send-pack: gracefully close the connection for atomic push\n\nPatrick Steinhardt (2):\n t5504: modernize test by moving heredocs into test bodies\n t5543: atomic push reports exit code failure\n\n send-pack.c | 10 +-\n send-pack.h | 3 +\n t/t5504-fetch-receive-strict.sh | 35 ++-\n t/t5543-atomic-push.sh | 30 +++\n t/t5548-push-porcelain.sh | 425 +++++++++++++++++++++++---------\n transport.c | 17 +-\n 6 files changed, 374 insertions(+), 146 deletions(-)\n\n-- \n2.47.0.rc1.21.g81e7bd6151\n"},{"id":"508907","messageId":"4e509ef20465dc2a818f479b34e11df415e39906.1733830410.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 1/8] t5504: modernize test by moving heredocs into test bodies","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T11:36:21Z","receivedAt":"2024-12-10T11:36:35Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Patrick Steinhardt \n\nWe have several heredocs in t5504 located outside of any particular test\nbodies. Move these into the test bodies to match our modern coding\nstyle.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5504-fetch-receive-strict.sh | 35 +++++++++++++++------------------\n 1 file changed, 16 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 53dbc8ce3a..488310c340 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n \t)\n '\n \n-cat >exp <exp <<-EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (missing necessary objects)\n+\tDone\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '\n \ttest_cmp exp act\n '\n \n-cat >exp <exp <<-EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (unpacker error)\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -129,15 +127,14 @@ test_expect_success 'repair the \"corrupt or missing\" object' '\n \tgit fsck\n '\n \n-cat >bogus-commit < 1234567890 +0000\n-\n-This commit object intentionally broken\n-EOF\n-\n test_expect_success 'setup bogus commit' '\n+\tcat >bogus-commit <<-EOF &&\n+\ttree $EMPTY_TREE\n+\tauthor Bugs Bunny 1234567890 +0000\n+\tcommitter Bugs Bunny 1234567890 +0000\n+\n+\tThis commit object intentionally broken\n+\tEOF\n \tcommit=\"$(git hash-object --literally -t commit -w --stdin \n\nRefactor the function setup_upstream_and_workbench(), extracting\ncreate_upstream_template() and setup_upstream() from it. The former is\nused to create the upstream repository template, while the latter is\nused to rebuild the upstream repository and will be reused in subsequent\ncommits.\n\nTo ensure that setup_upstream() works properly in both local and HTTP\nprotocols, the HTTP settings have been moved to the setup_upstream() and\nsetup_upstream_and_workbench() functions.\n\nSigned-off-by: Jiang Xin \n---\n t/t5548-push-porcelain.sh | 85 +++++++++++++++++++++++++--------------\n 1 file changed, 55 insertions(+), 30 deletions(-)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex 6282728eaf..a3defd5b75 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -54,29 +54,67 @@ format_and_save_expect () {\n \tsed -e 's/^> //' -e 's/Z$//' >expect\n }\n \n+create_upstream_template () {\n+\tgit init --bare upstream-template.git &&\n+\tgit clone upstream-template.git tmp_work_dir &&\n+\tcreate_commits_in tmp_work_dir A B &&\n+\t(\n+\t\tcd tmp_work_dir &&\n+\t\tgit push origin \\\n+\t\t\t$B:refs/heads/main \\\n+\t\t\t$A:refs/heads/foo \\\n+\t\t\t$A:refs/heads/bar \\\n+\t\t\t$A:refs/heads/baz\n+\t) &&\n+\trm -rf tmp_work_dir\n+}\n+\n+setup_upstream () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tBUG \"location of upstream repository is not provided\"\n+\tfi &&\n+\trm -rf \"$1\" &&\n+\tif ! test -d upstream-template.git\n+\tthen\n+\t\tcreate_upstream_template\n+\tfi &&\n+\tgit clone --mirror upstream-template.git \"$1\" &&\n+\t# The upstream repository provides services using the HTTP protocol.\n+\tif ! test \"$1\" = \"upstream.git\"\n+\tthen\n+\t\tgit -C \"$1\" config http.receivepack true\n+\tfi\n+}\n+\n setup_upstream_and_workbench () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tBUG \"location of upstream repository is not provided\"\n+\tfi &&\n+\t# Assign the first argument to the variable upstream;\n+\t# we will use it in the subsequent test cases.\n+\tupstream=\"$1\"\n+\n \t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n \t# Workbench after setup : main(A)\n \ttest_expect_success \"setup upstream repository and workbench\" '\n-\t\trm -rf upstream.git workbench &&\n-\t\tgit init --bare upstream.git &&\n-\t\tgit init workbench &&\n-\t\tcreate_commits_in workbench A B &&\n+\t\tsetup_upstream \"$upstream\" &&\n+\t\trm -rf workbench &&\n+\t\tgit clone \"$upstream\" workbench &&\n \t\t(\n \t\t\tcd workbench &&\n+\t\t\tgit update-ref refs/heads/main $A &&\n \t\t\t# Try to make a stable fixed width for abbreviated commit ID,\n \t\t\t# this fixed-width oid will be replaced with \"\".\n \t\t\tgit config core.abbrev 7 &&\n-\t\t\tgit remote add origin ../upstream.git &&\n-\t\t\tgit update-ref refs/heads/main $A &&\n-\t\t\tgit push origin \\\n-\t\t\t\t$B:refs/heads/main \\\n-\t\t\t\t$A:refs/heads/foo \\\n-\t\t\t\t$A:refs/heads/bar \\\n-\t\t\t\t$A:refs/heads/baz\n+\t\t\tgit config advice.pushUpdateRejected false\n \t\t) &&\n-\t\tgit -C \"workbench\" config advice.pushUpdateRejected false &&\n-\t\tupstream=upstream.git\n+\t\t# The upstream repository provides services using the HTTP protocol.\n+\t\tif ! test \"$upstream\" = \"upstream.git\"\n+\t\tthen\n+\t\t\tgit -C workbench remote set-url origin \"$HTTPD_URL/smart/upstream.git\"\n+\t\tfi\n \t'\n }\n \n@@ -88,7 +126,7 @@ run_git_push_porcelain_output_test() {\n \t\t;;\n \tfile)\n \t\tPROTOCOL=\"builtin protocol\"\n-\t\tURL_PREFIX=\"\\.\\.\"\n+\t\tURL_PREFIX=\".*\"\n \t\t;;\n \tesac\n \n@@ -247,10 +285,8 @@ run_git_push_porcelain_output_test() {\n \t'\n }\n \n-# Initialize the upstream repository and local workbench.\n-setup_upstream_and_workbench\n+setup_upstream_and_workbench upstream.git\n \n-# Run git-push porcelain test on builtin protocol\n run_git_push_porcelain_output_test file\n \n ROOT_PATH=\"$PWD\"\n@@ -258,21 +294,10 @@ ROOT_PATH=\"$PWD\"\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n start_httpd\n-\n-# Re-initialize the upstream repository and local workbench.\n-setup_upstream_and_workbench\n-\n-test_expect_success \"setup for http\" '\n-\tgit -C upstream.git config http.receivepack true &&\n-\tupstream=\"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\" &&\n-\tmv upstream.git \"$upstream\" &&\n-\n-\tgit -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git\n-'\n-\n setup_askpass_helper\n \n-# Run git-push porcelain test on HTTP protocol\n+setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n+\n run_git_push_porcelain_output_test http\n \n test_done\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"508909","messageId":"db43ce7b75efab324ad5c348f70b851f841c9df0.1733830410.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 3/8] t5548: refactor test cases by resetting upstream","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T11:36:23Z","receivedAt":"2024-12-10T11:36:37Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nRefactor the test cases with the following changes:\n\n - Calling setup_upstream() to reset upstream after running each test\n case.\n\n - Change the initial branch tips of the workspace to reduce the branch\n setup operations in the workspace.\n\n - Reduced the two steps of setting up and cleaning up the pre-receive\n hook by moving the operations into the corresponding test case,\n\nSigned-off-by: Jiang Xin \n---\n t/t5548-push-porcelain.sh | 149 +++++++++++++++++---------------------\n 1 file changed, 67 insertions(+), 82 deletions(-)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex a3defd5b75..ededd8edb9 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -96,8 +96,8 @@ setup_upstream_and_workbench () {\n \t# we will use it in the subsequent test cases.\n \tupstream=\"$1\"\n \n-\t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n-\t# Workbench after setup : main(A)\n+\t# Upstream after setup: main(B) foo(A) bar(A) baz(A)\n+\t# Workbench after setup: main(A) baz(A) next(A)\n \ttest_expect_success \"setup upstream repository and workbench\" '\n \t\tsetup_upstream \"$upstream\" &&\n \t\trm -rf workbench &&\n@@ -105,6 +105,8 @@ setup_upstream_and_workbench () {\n \t\t(\n \t\t\tcd workbench &&\n \t\t\tgit update-ref refs/heads/main $A &&\n+\t\t\tgit update-ref refs/heads/baz $A &&\n+\t\t\tgit update-ref refs/heads/next $A &&\n \t\t\t# Try to make a stable fixed width for abbreviated commit ID,\n \t\t\t# this fixed-width oid will be replaced with \"\".\n \t\t\tgit config core.abbrev 7 &&\n@@ -133,19 +135,14 @@ run_git_push_porcelain_output_test() {\n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n-\ttest_expect_success \"porcelain output of successful git-push ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $A &&\n-\t\t\tgit update-ref refs/heads/baz $A &&\n-\t\t\tgit update-ref refs/heads/next $A &&\n-\t\t\tgit push --porcelain --force origin \\\n-\t\t\t\tmain \\\n-\t\t\t\t:refs/heads/foo \\\n-\t\t\t\t$B:bar \\\n-\t\t\t\tbaz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\ttest_expect_success \".. git-push --porcelain --force ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\tgit -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n \t\t> To \n@@ -169,115 +166,103 @@ run_git_push_porcelain_output_test() {\n \t\ttest_cmp expect actual\n \t'\n \n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"atomic push failed ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $B &&\n-\t\t\tgit update-ref refs/heads/bar $A &&\n-\t\t\ttest_must_fail git push --atomic --porcelain origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git push --porcelain --atomic ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_must_fail git -C workbench push --porcelain --atomic origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[rejected] (non-fast-forward)\n-\t\t> !\t(delete):refs/heads/baz\t[rejected] (atomic push failed)\n-\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (atomic push failed)\n-\t\tDone\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[rejected] (atomic push failed)\n+\t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n \t\t refs/heads/baz\n-\t\t refs/heads/main\n-\t\t refs/heads/next\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"prepare pre-receive hook ($PROTOCOL)\" '\n-\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF\n-\t\texit 1\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. pre-receive hook declined ($PROTOCOL)\" '\n+\t\ttest_when_finished \"rm -f \\\"$upstream/hooks/pre-receive\\\" &&\n+\t\t\tsetup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF &&\n+\t\t\texit 1\n \t\tEOF\n-\t'\n-\n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"pre-receive hook declined ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $B &&\n-\t\t\tgit update-ref refs/heads/bar $A &&\n-\t\t\ttest_must_fail git push --porcelain --force origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t\ttest_must_fail git -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n-\t\t> !\t:refs/heads/baz\t[remote rejected] (pre-receive hook declined)\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\t:refs/heads/foo\t[remote rejected] (pre-receive hook declined)\n \t\t> !\trefs/heads/main:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n-\t\tDone\n+\t\t> !\trefs/heads/next:refs/heads/next\t[remote rejected] (pre-receive hook declined)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n \t\t refs/heads/baz\n-\t\t refs/heads/main\n-\t\t refs/heads/next\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"remove pre-receive hook ($PROTOCOL)\" '\n-\t\trm \"$upstream/hooks/pre-receive\"\n-\t'\n-\n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"non-fastforward push ($PROTOCOL)\" '\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) next(A)\n+\ttest_expect_success \".. non-fastforward push ($PROTOCOL)\" '\n \t\t(\n \t\t\tcd workbench &&\n \t\t\ttest_must_fail git push --porcelain origin \\\n \t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n \t\t\t\tnext\n \t\t) >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> -\t:refs/heads/baz\t[deleted]\n-\t\t> \trefs/heads/main:refs/heads/main\t..\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[rejected] (non-fast-forward)\n-\t\tDone\n+\t\t> To \n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n \t\t refs/heads/main\n \t\t refs/heads/next\n \t\tEOF\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"508910","messageId":"26248735e1b81df8d34cb692ddd9d5fb55e22329.1733830410.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 4/8] t5548: add new porcelain test cases","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T11:36:24Z","receivedAt":"2024-12-10T11:36:38Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nChange order of test cases and add new test cases:\n\n - git push --porcelain # failed: non-fast-forward\n - git push --porcelain --force\n - git push --porcelain --atomic # failed: non-fast-forward\n - git push --porcelain --atomic --force\n - git push --porcelain --force # failed: pre-receive declined\n\nSigned-off-by: Jiang Xin \n---\n t/t5548-push-porcelain.sh | 90 ++++++++++++++++++++++++++++-----------\n 1 file changed, 65 insertions(+), 25 deletions(-)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex ededd8edb9..799f6066a3 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -4,6 +4,9 @@\n #\n test_description='Test git push porcelain output'\n \n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n . ./test-lib.sh\n \n # Create commits in and assign each commit's oid to shell variables\n@@ -132,6 +135,40 @@ run_git_push_porcelain_output_test() {\n \t\t;;\n \tesac\n \n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_must_fail git -C workbench push --porcelain origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/main\n+\t\t refs/heads/next\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n@@ -203,13 +240,9 @@ run_git_push_porcelain_output_test() {\n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n-\ttest_expect_success \".. pre-receive hook declined ($PROTOCOL)\" '\n-\t\ttest_when_finished \"rm -f \\\"$upstream/hooks/pre-receive\\\" &&\n-\t\t\tsetup_upstream \\\"$upstream\\\"\" &&\n-\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF &&\n-\t\t\texit 1\n-\t\tEOF\n-\t\ttest_must_fail git -C workbench push --porcelain --force origin \\\n+\ttest_expect_success \".. git push --porcelain --atomic --force ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\tgit -C workbench push --porcelain --atomic --force origin \\\n \t\t\tmain \\\n \t\t\t:refs/heads/foo \\\n \t\t\t$B:bar \\\n@@ -219,10 +252,10 @@ run_git_push_porcelain_output_test() {\n \t\tformat_and_save_expect <<-EOF &&\n \t\t> To \n \t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n-\t\t> !\t:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n-\t\t> !\t:refs/heads/foo\t[remote rejected] (pre-receive hook declined)\n-\t\t> !\trefs/heads/main:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n-\t\t> !\trefs/heads/next:refs/heads/next\t[remote rejected] (pre-receive hook declined)\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n \t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n@@ -230,29 +263,37 @@ run_git_push_porcelain_output_test() {\n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n \t\t refs/heads/baz\n-\t\t refs/heads/foo\n-\t\t refs/heads/main\n+\t\t refs/heads/main\n+\t\t refs/heads/next\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n \n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n-\t# git-push : main(A) next(A)\n-\ttest_expect_success \".. non-fastforward push ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\ttest_must_fail git push --porcelain origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. pre-receive hook declined ($PROTOCOL)\" '\n+\t\ttest_when_finished \"rm -f \\\"$upstream/hooks/pre-receive\\\" &&\n+\t\t\tsetup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF &&\n+\t\t\texit 1\n+\t\tEOF\n+\t\ttest_must_fail git -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n \t\t> To \n-\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n-\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\t:refs/heads/foo\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[remote rejected] (pre-receive hook declined)\n \t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n@@ -264,7 +305,6 @@ run_git_push_porcelain_output_test() {\n \t\t refs/heads/baz\n \t\t refs/heads/foo\n \t\t refs/heads/main\n-\t\t refs/heads/next\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"508911","messageId":"93123988ae00d865d0251ca0667c4f4584772021.1733830410.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 5/8] t5548: add porcelain push test cases for dry-run mode","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T11:36:25Z","receivedAt":"2024-12-10T11:36:39Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nNew dry-run test cases:\n\n - git push --porcelain --dry-run\n - git push --porcelain --dry-run --force\n - git push --porcelain --dry-run --atomic\n - git push --porcelain --dry-run --atomic --force\n\nSigned-off-by: Jiang Xin \n---\n t/t5548-push-porcelain.sh | 153 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 153 insertions(+)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex 799f6066a3..ba68808459 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -310,10 +310,159 @@ run_git_push_porcelain_output_test() {\n \t'\n }\n \n+run_git_push_dry_run_porcelain_output_test() {\n+\tcase $1 in\n+\thttp)\n+\t\tPROTOCOL=\"HTTP protocol\"\n+\t\tURL_PREFIX=\"http://.*\"\n+\t\t;;\n+\tfile)\n+\t\tPROTOCOL=\"builtin protocol\"\n+\t\tURL_PREFIX=\"/.*\"\n+\t\t;;\n+\tesac\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run ($PROTOCOL)\" '\n+\t\ttest_must_fail git -C workbench push --porcelain --dry-run origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --dry-run --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --atomic ($PROTOCOL)\" '\n+\t\ttest_must_fail git -C workbench push --porcelain --dry-run --atomic origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[rejected] (atomic push failed)\n+\t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --atomic --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --dry-run --atomic --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+}\n+\n setup_upstream_and_workbench upstream.git\n \n run_git_push_porcelain_output_test file\n \n+setup_upstream_and_workbench upstream.git\n+\n+run_git_push_dry_run_porcelain_output_test file\n+\n ROOT_PATH=\"$PWD\"\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n@@ -325,4 +474,8 @@ setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n \n run_git_push_porcelain_output_test http\n \n+setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n+\n+run_git_push_dry_run_porcelain_output_test http\n+\n test_done\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"508912","messageId":"77f5e128fc1b66a9c8b0df3ce274a98921800685.1733830410.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 6/8] send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T11:36:26Z","receivedAt":"2024-12-10T11:36:40Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nThe \"push_refs\" function in the transport_vtable is the handler for\ngit-push operation. All the \"push_refs\" functions for different\ntransports (protocols) should have the same behavior, but the behavior\nof \"git_transport_push()\" function for builtin_smart_vtable in\n\"transport.c\" (which calls \"send_pack()\" in \"send-pack.c\") differs from\nthe handler of the HTTP protocol.\n\nThe \"push_refs()\" function for the HTTP protocol which calls the\n\"push_refs_with_push()\" function in \"transport-helper.c\" will return 0\neven when a bad REF_STATUS (such as REF_STATUS_REJECT_NONFASTFORWARD)\nwas found. But \"send_pack()\" for Git smart protocol will return -1 for\na bad REF_STATUS.\n\nWe cannot ignore bad REF_STATUS directly in the \"send_pack()\" function,\nbecause the function is also used in \"builtin/send-pack.c\". So we add a\nnew non-zero error code \"SEND_PACK_ERROR_REF_STATUS\" for \"send_pack()\".\nWe can choose to ignore the specific error code in the\n\"git_transport_push()\" function to have the same behavior as\n\"push_refs()\" for HTTP protocol.\n\nSigned-off-by: Jiang Xin \n---\n send-pack.c | 9 ++-------\n send-pack.h | 3 +++\n transport.c | 7 +++++++\n 3 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 6677c44e8a..f1556dd53c 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n-\t\t\t\tret = args->porcelain ? 0 : -1;\n+\t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\n@@ -761,11 +761,6 @@ int send_pack(struct send_pack_args *args,\n \tif (ret < 0)\n \t\tgoto out;\n \n-\tif (args->porcelain) {\n-\t\tret = 0;\n-\t\tgoto out;\n-\t}\n-\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\n@@ -773,7 +768,7 @@ int send_pack(struct send_pack_args *args,\n \t\tcase REF_STATUS_OK:\n \t\t\tbreak;\n \t\tdefault:\n-\t\t\tret = -1;\n+\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n \t\t\tgoto out;\n \t\t}\n \t}\ndiff --git a/send-pack.h b/send-pack.h\nindex 7edb80596c..ee88f9fe9f 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -12,6 +12,9 @@ struct ref;\n #define SEND_PACK_PUSH_CERT_IF_ASKED 1\n #define SEND_PACK_PUSH_CERT_ALWAYS 2\n \n+/* Custom exit code from send_pack. */\n+#define ERROR_SEND_PACK_BAD_REF_STATUS 1\n+\n struct send_pack_args {\n \tconst char *url;\n \tunsigned verbose:1,\ndiff --git a/transport.c b/transport.c\nindex 47fda6a773..454d7f21a9 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -914,6 +914,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tcase protocol_v0:\n \t\tret = send_pack(&args, data->fd, data->conn, remote_refs,\n \t\t\t\t&data->extra_have);\n+\t\t/*\n+\t\t * Ignore the specific error code to maintain consistent behavior\n+\t\t * with the \"push_refs()\" function across different transports,\n+\t\t * such as \"push_refs_with_push()\" for HTTP protocol.\n+\t\t */\n+\t\tif (ret == ERROR_SEND_PACK_BAD_REF_STATUS)\n+\t\t\tret = 0;\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"508913","messageId":"7c78dbeffb24fc84e5e6d5e01e77c58324bf60c9.1733830410.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 7/8] t5543: atomic push reports exit code failure","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T11:36:27Z","receivedAt":"2024-12-10T11:36:41Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Patrick Steinhardt \n\nAdd new test cases in t5543 to avoid ignoring the exit code of\ngit-receive-pack(1) during atomic push with \"--porcelain\" flag.\n\nWe'd typically notice this case because the refs would have their error\nmessage set. But there is an edge case when pushing refs succeeds, but\ngit-receive-pack(1) exits with a non-zero exit code at a later point in\ntime due to another error. An atomic git-push(1) would ignore that error\ncode, and consequently it would return successfully and not print any\nerror message at all.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 04b47ad84a..32181b9afb 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -280,4 +280,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'atomic push reports exit code failure' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict 2>err &&\n+\tcat >expect <<-EOF &&\n+\tTo ../upstream\n+\t * [new branch] HEAD -> no-conflict\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure 'atomic push reports exit code failure with porcelain' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic --porcelain \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict-porcelain 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"508914","messageId":"9f49278ef108f08d512ee13128ed8277e12c002d.1733830410.git.zhiyou.jx@alibaba-inc.com","threadId":"62487","inReplyTo":"cover.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 8/8] send-pack: gracefully close the connection for atomic push","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T11:36:28Z","receivedAt":"2024-12-10T11:36:41Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nPatrick reported an issue that the exit code of git-receive-pack(1) is\nignored during atomic push with \"--porcelain\" flag, and added new test\ncases in t5543.\n\nThis issue originated from commit 7dcbeaa0df (send-pack: fix\ninconsistent porcelain output, 2020-04-17). At that time, I chose to\nignore the exit code of \"finish_connect()\" without investigating the\nroot cause of the abnormal termination of git-receive-pack. That was an\nincorrect solution.\n\nThe root cause is that an atomic push operation terminates early without\nsending a flush packet to git-receive-pack. As a result,\ngit-receive-pack continues waiting for commands without exiting. By\nsending a flush packet at the appropriate location in \"send_pack()\", we\nensure that the git-receive-pack process closes properly, avoiding an\nerroneous exit code for git-push. At the same time, revert the changes\nto the \"transport.c\" file made in commit 7dcbeaa0df.\n\nReported-by: Patrick Steinhardt \nSigned-off-by: Jiang Xin \n---\n send-pack.c | 1 +\n t/t5543-atomic-push.sh | 4 ++--\n transport.c | 10 +---------\n 3 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex f1556dd53c..439b249c79 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -631,6 +631,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n \t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n+\t\t\t\tpacket_flush(out);\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 32181b9afb..3a700b0676 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -280,7 +280,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'atomic push reports exit code failure' '\n+test_expect_success 'atomic push reports exit code failure' '\n \twrite_script receive-pack-wrapper <<-\\EOF &&\n \tgit-receive-pack \"$@\"\n \texit 1\n@@ -296,7 +296,7 @@ test_expect_failure 'atomic push reports exit code failure' '\n \ttest_cmp expect err\n '\n \n-test_expect_failure 'atomic push reports exit code failure with porcelain' '\n+test_expect_success 'atomic push reports exit code failure with porcelain' '\n \twrite_script receive-pack-wrapper <<-\\EOF &&\n \tgit-receive-pack \"$@\"\n \texit 1\ndiff --git a/transport.c b/transport.c\nindex 454d7f21a9..afcbca293b 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -928,15 +928,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \n \tclose(data->fd[1]);\n \tclose(data->fd[0]);\n-\t/*\n-\t * Atomic push may abort the connection early and close the pipe,\n-\t * which may cause an error for `finish_connect()`. Ignore this error\n-\t * for atomic git-push.\n-\t */\n-\tif (ret || args.atomic)\n-\t\tfinish_connect(data->conn);\n-\telse\n-\t\tret = finish_connect(data->conn);\n+\tret |= finish_connect(data->conn);\n \tdata->conn = NULL;\n \tdata->finished_handshake = 0;\n \n-- \n2.47.0.rc1.21.g81e7bd6151\n\n"},{"id":"508924","messageId":"CANYiYbHQVsm=Q-DXfmYu-Ar+NYpbv4vcSeWO413iD7qYzu0zNg@mail.gmail.com","threadId":"62487","inReplyTo":"93123988ae00d865d0251ca0667c4f4584772021.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v3 5/8] t5548: add porcelain push test cases for dry-run mode","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-12-10T12:19:35Z","receivedAt":"2024-12-10T12:19:47Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Tue, Dec 10, 2024 at 7:36 PM Jiang Xin wrote:\n>\n> From: Jiang Xin \n>\n> New dry-run test cases:\n>\n> - git push --porcelain --dry-run\n> - git push --porcelain --dry-run --force\n> - git push --porcelain --dry-run --atomic\n> - git push --porcelain --dry-run --atomic --force\n>\n> Signed-off-by: Jiang Xin \n> ---\n> t/t5548-push-porcelain.sh | 153 ++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 153 insertions(+)\n>\n> diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\n> index 799f6066a3..ba68808459 100755\n> --- a/t/t5548-push-porcelain.sh\n> +++ b/t/t5548-push-porcelain.sh\n> @@ -310,10 +310,159 @@ run_git_push_porcelain_output_test() {\n> '\n> }\n>\n> +run_git_push_dry_run_porcelain_output_test() {\n> + case $1 in\n> + http)\n> + PROTOCOL=\"HTTP protocol\"\n> + URL_PREFIX=\"http://.*\"\n> + ;;\n> + file)\n> + PROTOCOL=\"builtin protocol\"\n> + URL_PREFIX=\"/.*\"\n\nThis line breaks CI on Windows, see:\nhttps://github.com/jiangxin/git/actions/runs/12255233680/job/34188220514\n Should remove the leading \"/\". as follows:\n\n1: 93123988ae ! 1: 9e764b6faf t5548: add porcelain push test cases\nfor dry-run mode\n @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {\n + ;;\n + file)\n + PROTOCOL=\"builtin protocol\"\n -+ URL_PREFIX=\"/.*\"\n ++ URL_PREFIX=\".*\"\n + ;;\n + esac\n +\n"},{"id":"509146","messageId":"Z1_mgG7sqNtHWFnQ@pks.im","threadId":"62487","inReplyTo":"77f5e128fc1b66a9c8b0df3ce274a98921800685.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v3 6/8] send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-16T08:36:23Z","receivedAt":"2024-12-16T08:36:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 10, 2024 at 07:36:26PM +0800, Jiang Xin wrote:\n> diff --git a/send-pack.c b/send-pack.c\n> index 6677c44e8a..f1556dd53c 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,\n> \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n> \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n> \t\t\t\t ref->name, ref->status);\n> -\t\t\t\tret = args->porcelain ? 0 : -1;\n> +\t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n> \t\t\t\tgoto out;\n> \t\t\t}\n> \t\t\t/* else fallthrough */\n\nOkay, this change looks sensible -- instead of having different\nbehaviour with `--porcelain` we now have the exact same behaviour with\nand without that flag set, which is good.\n\n> @@ -761,11 +761,6 @@ int send_pack(struct send_pack_args *args,\n> \tif (ret < 0)\n> \t\tgoto out;\n> \n> -\tif (args->porcelain) {\n> -\t\tret = 0;\n> -\t\tgoto out;\n> -\t}\n\nWe don't reset the error code here anymore.\n\n> \tfor (ref = remote_refs; ref; ref = ref->next) {\n> \t\tswitch (ref->status) {\n> \t\tcase REF_STATUS_NONE:\n> @@ -773,7 +768,7 @@ int send_pack(struct send_pack_args *args,\n> \t\tcase REF_STATUS_OK:\n> \t\t\tbreak;\n> \t\tdefault:\n> -\t\t\tret = -1;\n> +\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n> \t\t\tgoto out;\n> \t\t}\n> \t}\n\nAnd this is the equivalent change without `--atomic`.\n\nAlso, none of the existing code paths used to set `ret = 1`. We do call\n`ret = receive_status()` and bubble that up, but neither that nor any of\nits transitive calls set `ret = 1`, either. So this should be fine and\nwe don't seem to have conflicting error codes.\n\n> diff --git a/send-pack.h b/send-pack.h\n> index 7edb80596c..ee88f9fe9f 100644\n> --- a/send-pack.h\n> +++ b/send-pack.h\n> @@ -12,6 +12,9 @@ struct ref;\n> #define SEND_PACK_PUSH_CERT_IF_ASKED 1\n> #define SEND_PACK_PUSH_CERT_ALWAYS 2\n> \n> +/* Custom exit code from send_pack. */\n> +#define ERROR_SEND_PACK_BAD_REF_STATUS 1\n> +\n> struct send_pack_args {\n> \tconst char *url;\n> \tunsigned verbose:1,\n\nIt might help to document `send_pack()` accordingly and point out the\nspecial return code.\n\n> diff --git a/transport.c b/transport.c\n> index 47fda6a773..454d7f21a9 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -914,6 +914,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n> \tcase protocol_v0:\n> \t\tret = send_pack(&args, data->fd, data->conn, remote_refs,\n> \t\t\t\t&data->extra_have);\n> +\t\t/*\n> +\t\t * Ignore the specific error code to maintain consistent behavior\n> +\t\t * with the \"push_refs()\" function across different transports,\n> +\t\t * such as \"push_refs_with_push()\" for HTTP protocol.\n> +\t\t */\n> +\t\tif (ret == ERROR_SEND_PACK_BAD_REF_STATUS)\n> +\t\t\tret = 0;\n> \t\tbreak;\n> \tcase protocol_unknown_version:\n> \t\tBUG(\"unknown protocol version\");\n\nAnd here we ignore the error code to retain compatibility.\n\nIs there any case where this causes a user-visible change in behaviour\nalready? If so, we can add a test for this?\n\nPatrick\n"},{"id":"509147","messageId":"Z1_mjOdrUwb-XeDs@pks.im","threadId":"62487","inReplyTo":"9f49278ef108f08d512ee13128ed8277e12c002d.1733830410.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v3 8/8] send-pack: gracefully close the connection for atomic push","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-16T08:36:28Z","receivedAt":"2024-12-16T08:36:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 10, 2024 at 07:36:28PM +0800, Jiang Xin wrote:\n> diff --git a/send-pack.c b/send-pack.c\n> index f1556dd53c..439b249c79 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -631,6 +631,7 @@ int send_pack(struct send_pack_args *args,\n> \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n> \t\t\t\t ref->name, ref->status);\n> \t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n> +\t\t\t\tpacket_flush(out);\n> \t\t\t\tgoto out;\n> \t\t\t}\n> \t\t\t/* else fallthrough */\n\nNice and easy fix.\n\nPatrick\n"},{"id":"511537","messageId":"20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im","threadId":"62487","inReplyTo":"20241113-pks-push-atomic-respect-exit-code-v1-0-7965f01e7f4e@pks.im","subject":"[PATCH v4 0/8] transport: don't ignore git-receive-pack(1) exit code on atomic push","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-31T10:53:24Z","receivedAt":"2025-01-31T10:53:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwe've hit an edge case at GitLab where an atomic push will not notice an\nerror when git-receive-pack(1) updates the refs, but otherwise fails\nwith a non-zero exit code. The push would be successful and no error\nwould be printed even though some things have gone wrong on the remote\nside.\n\nAs promised last week, I've now adopted the patch series from Jiang Xin\nto make some progress on the issue. The series is now based on the\nlatest master branch at 3b0d05c4a7 (The fifth batch, 2025-01-29).\n\nChanges in v4:\n - Rewrite the commit that adds two new tests for `--porcelain`.\n Previously, the commit both reordered existing tests and added new\n ones, which made it hard to see what actually changed. The reorder\n wasn't necessary though, so I've adapted it to only add new tests\n now.\n - Fix the URL prefix in t5548 to also work on Windows.\n - Document why it's fine to start ignoring the return value of\n `git_transport_push()`.\n - Improve comments to document behaviour better.\n - Link to v3: https://lore.kernel.org/r/cover.1733830410.git.zhiyou.jx@alibaba-inc.com\n\nThanks!\n\nPatrick\n\n---\nJiang Xin (5):\n t5548: refactor to reuse setup_upstream() function\n t5548: refactor test cases by resetting upstream\n t5548: add porcelain push test cases for dry-run mode\n send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"\n send-pack: gracefully close the connection for atomic push\n\nPatrick Steinhardt (3):\n t5504: modernize test by moving heredocs into test bodies\n t5548: add new porcelain test cases\n t5543: atomic push reports exit code failure\n\n send-pack.c | 10 +-\n send-pack.h | 13 ++\n t/t5504-fetch-receive-strict.sh | 35 ++--\n t/t5543-atomic-push.sh | 30 +++\n t/t5548-push-porcelain.sh | 443 ++++++++++++++++++++++++++++++----------\n transport.c | 17 +-\n 6 files changed, 407 insertions(+), 141 deletions(-)\n\nRange-diff versus v3:\n\n1: 5001da5515 = 1: 5a68112d63 t5504: modernize test by moving heredocs into test bodies\n2: de1b7eab6a ! 2: bb3b9cd8b7 t5548: refactor to reuse setup_upstream() function\n @@ Commit message\n setup_upstream_and_workbench() functions.\n \n Signed-off-by: Jiang Xin \n + Signed-off-by: Patrick Steinhardt \n \n ## t/t5548-push-porcelain.sh ##\n @@ t/t5548-push-porcelain.sh: format_and_save_expect () {\n3: be1daa20cd ! 3: 7268ab4fdf t5548: refactor test cases by resetting upstream\n @@ Commit message\n hook by moving the operations into the corresponding test case,\n \n Signed-off-by: Jiang Xin \n + Signed-off-by: Patrick Steinhardt \n \n ## t/t5548-push-porcelain.sh ##\n @@ t/t5548-push-porcelain.sh: setup_upstream_and_workbench () {\n4: d353cc13d0 < -: ---------- t5548: add new porcelain test cases\n-: ---------- > 4: 1b721338eb t5548: add new porcelain test cases\n5: 2f4723d34f ! 5: 7be03b6fdd t5548: add porcelain push test cases for dry-run mode\n @@ Commit message\n - git push --porcelain --dry-run --atomic --force\n \n Signed-off-by: Jiang Xin \n + Signed-off-by: Patrick Steinhardt \n \n ## t/t5548-push-porcelain.sh ##\n @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {\n @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {\n +\t\t;;\n +\tfile)\n +\t\tPROTOCOL=\"builtin protocol\"\n -+\t\tURL_PREFIX=\"/.*\"\n ++\t\tURL_PREFIX=\".*\"\n +\t\t;;\n +\tesac\n +\n6: 73728ec873 ! 6: ea0e885613 send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"\n @@ Commit message\n We cannot ignore bad REF_STATUS directly in the \"send_pack()\" function,\n because the function is also used in \"builtin/send-pack.c\". So we add a\n new non-zero error code \"SEND_PACK_ERROR_REF_STATUS\" for \"send_pack()\".\n - We can choose to ignore the specific error code in the\n - \"git_transport_push()\" function to have the same behavior as\n - \"push_refs()\" for HTTP protocol.\n +\n + Ignore the specific error code in the \"git_transport_push()\" function to\n + have the same behavior as \"push_refs()\" for HTTP protocol. Note that\n + even though we ignore the error here, we'll ultimately still end up\n + detecting that a subset of refs was not pushed in `transport_push()`\n + because we eventually call `push_had_errors()` on the remote refs.\n \n Signed-off-by: Jiang Xin \n + Signed-off-by: Patrick Steinhardt \n \n ## send-pack.c ##\n -@@ send-pack.c: int send_pack(struct send_pack_args *args,\n +@@ send-pack.c: int send_pack(struct repository *r,\n \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\n -@@ send-pack.c: int send_pack(struct send_pack_args *args,\n +@@ send-pack.c: int send_pack(struct repository *r,\n \tif (ret < 0)\n \t\tgoto out;\n \n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\n -@@ send-pack.c: int send_pack(struct send_pack_args *args,\n +@@ send-pack.c: int send_pack(struct repository *r,\n \t\tcase REF_STATUS_OK:\n \t\t\tbreak;\n \t\tdefault:\n @@ send-pack.c: int send_pack(struct send_pack_args *args,\n \t}\n \n ## send-pack.h ##\n -@@ send-pack.h: struct ref;\n +@@ send-pack.h: struct repository;\n #define SEND_PACK_PUSH_CERT_IF_ASKED 1\n #define SEND_PACK_PUSH_CERT_ALWAYS 2\n \n -+/* Custom exit code from send_pack. */\n ++/* At least one reference has been rejected by the remote side. */\n +#define ERROR_SEND_PACK_BAD_REF_STATUS 1\n +\n struct send_pack_args {\n \tconst char *url;\n \tunsigned verbose:1,\n +@@ send-pack.h: struct option;\n + int option_parse_push_signed(const struct option *opt,\n + \t\t\t const char *arg, int unset);\n + \n ++/*\n ++ * Compute a packfile and write it to a file descriptor. The `fd` array needs\n ++ * to contain two file descriptors: `fd[0]` is the file descriptor used as\n ++ * input for the packet reader, whereas `fd[1]` is the file descriptor the\n ++ * packfile will be written to.\n ++ *\n ++ * Returns 0 on success, non-zero otherwise. Negative return values indicate a\n ++ * generic error, whereas positive return values indicate specific error\n ++ * conditions as documented with the `ERROR_SEND_PACK_*` constants.\n ++ */\n + int send_pack(struct repository *r, struct send_pack_args *args,\n + \t int fd[], struct child_process *conn,\n + \t struct ref *remote_refs, struct oid_array *extra_have);\n \n ## transport.c ##\n @@ transport.c: static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tcase protocol_v0:\n - \t\tret = send_pack(&args, data->fd, data->conn, remote_refs,\n + \t\tret = send_pack(the_repository, &args, data->fd, data->conn, remote_refs,\n \t\t\t\t&data->extra_have);\n +\t\t/*\n +\t\t * Ignore the specific error code to maintain consistent behavior\n7: b26774fee4 = 7: 6930d7ec2b t5543: atomic push reports exit code failure\n8: ab4d0906f6 ! 8: c2c82673fd send-pack: gracefully close the connection for atomic push\n @@ Commit message\n \n Reported-by: Patrick Steinhardt \n Signed-off-by: Jiang Xin \n + Signed-off-by: Patrick Steinhardt \n \n ## send-pack.c ##\n -@@ send-pack.c: int send_pack(struct send_pack_args *args,\n +@@ send-pack.c: int send_pack(struct repository *r,\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n \t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n\n---\nbase-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08\nchange-id: 20241113-pks-push-atomic-respect-exit-code-436c443a657d\n\n"},{"id":"511538","messageId":"20250131-pks-push-atomic-respect-exit-code-v4-1-a8b41f01a676@pks.im","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im","subject":"[PATCH v4 1/8] t5504: modernize test by moving heredocs into test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-31T10:53:25Z","receivedAt":"2025-01-31T10:53:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have several heredocs in t5504 located outside of any particular test\nbodies. Move these into the test bodies to match our modern coding\nstyle.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5504-fetch-receive-strict.sh | 35 ++++++++++++++++-------------------\n 1 file changed, 16 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex e273ab29c7..29f18841c3 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n \t)\n '\n \n-cat >exp <exp <<-EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (missing necessary objects)\n+\tDone\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '\n \ttest_cmp exp act\n '\n \n-cat >exp <exp <<-EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (unpacker error)\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -129,15 +127,14 @@ test_expect_success 'repair the \"corrupt or missing\" object' '\n \tgit fsck\n '\n \n-cat >bogus-commit < 1234567890 +0000\n-\n-This commit object intentionally broken\n-EOF\n-\n test_expect_success 'setup bogus commit' '\n+\tcat >bogus-commit <<-EOF &&\n+\ttree $EMPTY_TREE\n+\tauthor Bugs Bunny 1234567890 +0000\n+\tcommitter Bugs Bunny 1234567890 +0000\n+\n+\tThis commit object intentionally broken\n+\tEOF\n \tcommit=\"$(git hash-object --literally -t commit -w --stdin \n\nRefactor the function setup_upstream_and_workbench(), extracting\ncreate_upstream_template() and setup_upstream() from it. The former is\nused to create the upstream repository template, while the latter is\nused to rebuild the upstream repository and will be reused in subsequent\ncommits.\n\nTo ensure that setup_upstream() works properly in both local and HTTP\nprotocols, the HTTP settings have been moved to the setup_upstream() and\nsetup_upstream_and_workbench() functions.\n\nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n t/t5548-push-porcelain.sh | 85 ++++++++++++++++++++++++++++++-----------------\n 1 file changed, 55 insertions(+), 30 deletions(-)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex 6282728eaf..a3defd5b75 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -54,29 +54,67 @@ format_and_save_expect () {\n \tsed -e 's/^> //' -e 's/Z$//' >expect\n }\n \n+create_upstream_template () {\n+\tgit init --bare upstream-template.git &&\n+\tgit clone upstream-template.git tmp_work_dir &&\n+\tcreate_commits_in tmp_work_dir A B &&\n+\t(\n+\t\tcd tmp_work_dir &&\n+\t\tgit push origin \\\n+\t\t\t$B:refs/heads/main \\\n+\t\t\t$A:refs/heads/foo \\\n+\t\t\t$A:refs/heads/bar \\\n+\t\t\t$A:refs/heads/baz\n+\t) &&\n+\trm -rf tmp_work_dir\n+}\n+\n+setup_upstream () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tBUG \"location of upstream repository is not provided\"\n+\tfi &&\n+\trm -rf \"$1\" &&\n+\tif ! test -d upstream-template.git\n+\tthen\n+\t\tcreate_upstream_template\n+\tfi &&\n+\tgit clone --mirror upstream-template.git \"$1\" &&\n+\t# The upstream repository provides services using the HTTP protocol.\n+\tif ! test \"$1\" = \"upstream.git\"\n+\tthen\n+\t\tgit -C \"$1\" config http.receivepack true\n+\tfi\n+}\n+\n setup_upstream_and_workbench () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tBUG \"location of upstream repository is not provided\"\n+\tfi &&\n+\t# Assign the first argument to the variable upstream;\n+\t# we will use it in the subsequent test cases.\n+\tupstream=\"$1\"\n+\n \t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n \t# Workbench after setup : main(A)\n \ttest_expect_success \"setup upstream repository and workbench\" '\n-\t\trm -rf upstream.git workbench &&\n-\t\tgit init --bare upstream.git &&\n-\t\tgit init workbench &&\n-\t\tcreate_commits_in workbench A B &&\n+\t\tsetup_upstream \"$upstream\" &&\n+\t\trm -rf workbench &&\n+\t\tgit clone \"$upstream\" workbench &&\n \t\t(\n \t\t\tcd workbench &&\n+\t\t\tgit update-ref refs/heads/main $A &&\n \t\t\t# Try to make a stable fixed width for abbreviated commit ID,\n \t\t\t# this fixed-width oid will be replaced with \"\".\n \t\t\tgit config core.abbrev 7 &&\n-\t\t\tgit remote add origin ../upstream.git &&\n-\t\t\tgit update-ref refs/heads/main $A &&\n-\t\t\tgit push origin \\\n-\t\t\t\t$B:refs/heads/main \\\n-\t\t\t\t$A:refs/heads/foo \\\n-\t\t\t\t$A:refs/heads/bar \\\n-\t\t\t\t$A:refs/heads/baz\n+\t\t\tgit config advice.pushUpdateRejected false\n \t\t) &&\n-\t\tgit -C \"workbench\" config advice.pushUpdateRejected false &&\n-\t\tupstream=upstream.git\n+\t\t# The upstream repository provides services using the HTTP protocol.\n+\t\tif ! test \"$upstream\" = \"upstream.git\"\n+\t\tthen\n+\t\t\tgit -C workbench remote set-url origin \"$HTTPD_URL/smart/upstream.git\"\n+\t\tfi\n \t'\n }\n \n@@ -88,7 +126,7 @@ run_git_push_porcelain_output_test() {\n \t\t;;\n \tfile)\n \t\tPROTOCOL=\"builtin protocol\"\n-\t\tURL_PREFIX=\"\\.\\.\"\n+\t\tURL_PREFIX=\".*\"\n \t\t;;\n \tesac\n \n@@ -247,10 +285,8 @@ run_git_push_porcelain_output_test() {\n \t'\n }\n \n-# Initialize the upstream repository and local workbench.\n-setup_upstream_and_workbench\n+setup_upstream_and_workbench upstream.git\n \n-# Run git-push porcelain test on builtin protocol\n run_git_push_porcelain_output_test file\n \n ROOT_PATH=\"$PWD\"\n@@ -258,21 +294,10 @@ ROOT_PATH=\"$PWD\"\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n start_httpd\n-\n-# Re-initialize the upstream repository and local workbench.\n-setup_upstream_and_workbench\n-\n-test_expect_success \"setup for http\" '\n-\tgit -C upstream.git config http.receivepack true &&\n-\tupstream=\"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\" &&\n-\tmv upstream.git \"$upstream\" &&\n-\n-\tgit -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git\n-'\n-\n setup_askpass_helper\n \n-# Run git-push porcelain test on HTTP protocol\n+setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n+\n run_git_push_porcelain_output_test http\n \n test_done\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511540","messageId":"20250131-pks-push-atomic-respect-exit-code-v4-4-a8b41f01a676@pks.im","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im","subject":"[PATCH v4 4/8] t5548: add new porcelain test cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-31T10:53:28Z","receivedAt":"2025-01-31T10:53:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add two more test cases exercising git-push(1) with `--procelain`, one\nexercising a non-atomic and one exercising an atomic push.\n\nBased-on-patch-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n t/t5548-push-porcelain.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 68 insertions(+)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex ededd8edb9..b510a2e6bb 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -132,6 +132,40 @@ run_git_push_porcelain_output_test() {\n \t\t;;\n \tesac\n \n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_must_fail git -C workbench push --porcelain origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/main\n+\t\t refs/heads/next\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n@@ -242,6 +276,7 @@ run_git_push_porcelain_output_test() {\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) next(A)\n \ttest_expect_success \".. non-fastforward push ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n \t\t(\n \t\t\tcd workbench &&\n \t\t\ttest_must_fail git push --porcelain origin \\\n@@ -268,6 +303,39 @@ run_git_push_porcelain_output_test() {\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git push --porcelain --atomic --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --atomic --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/main\n+\t\t refs/heads/next\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n }\n \n setup_upstream_and_workbench upstream.git\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511541","messageId":"20250131-pks-push-atomic-respect-exit-code-v4-5-a8b41f01a676@pks.im","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im","subject":"[PATCH v4 5/8] t5548: add porcelain push test cases for dry-run mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-31T10:53:29Z","receivedAt":"2025-01-31T10:53:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jiang Xin \n\nNew dry-run test cases:\n\n - git push --porcelain --dry-run\n - git push --porcelain --dry-run --force\n - git push --porcelain --dry-run --atomic\n - git push --porcelain --dry-run --atomic --force\n\nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n t/t5548-push-porcelain.sh | 153 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 153 insertions(+)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex b510a2e6bb..c20e4ddc01 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -338,10 +338,159 @@ run_git_push_porcelain_output_test() {\n \t'\n }\n \n+run_git_push_dry_run_porcelain_output_test() {\n+\tcase $1 in\n+\thttp)\n+\t\tPROTOCOL=\"HTTP protocol\"\n+\t\tURL_PREFIX=\"http://.*\"\n+\t\t;;\n+\tfile)\n+\t\tPROTOCOL=\"builtin protocol\"\n+\t\tURL_PREFIX=\".*\"\n+\t\t;;\n+\tesac\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run ($PROTOCOL)\" '\n+\t\ttest_must_fail git -C workbench push --porcelain --dry-run origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --dry-run --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --atomic ($PROTOCOL)\" '\n+\t\ttest_must_fail git -C workbench push --porcelain --dry-run --atomic origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[rejected] (atomic push failed)\n+\t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --atomic --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --dry-run --atomic --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+}\n+\n setup_upstream_and_workbench upstream.git\n \n run_git_push_porcelain_output_test file\n \n+setup_upstream_and_workbench upstream.git\n+\n+run_git_push_dry_run_porcelain_output_test file\n+\n ROOT_PATH=\"$PWD\"\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n@@ -353,4 +502,8 @@ setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n \n run_git_push_porcelain_output_test http\n \n+setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n+\n+run_git_push_dry_run_porcelain_output_test http\n+\n test_done\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511542","messageId":"20250131-pks-push-atomic-respect-exit-code-v4-3-a8b41f01a676@pks.im","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im","subject":"[PATCH v4 3/8] t5548: refactor test cases by resetting upstream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-31T10:53:27Z","receivedAt":"2025-01-31T10:53:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jiang Xin \n\nRefactor the test cases with the following changes:\n\n - Calling setup_upstream() to reset upstream after running each test\n case.\n\n - Change the initial branch tips of the workspace to reduce the branch\n setup operations in the workspace.\n\n - Reduced the two steps of setting up and cleaning up the pre-receive\n hook by moving the operations into the corresponding test case,\n\nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n t/t5548-push-porcelain.sh | 149 +++++++++++++++++++++-------------------------\n 1 file changed, 67 insertions(+), 82 deletions(-)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex a3defd5b75..ededd8edb9 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -96,8 +96,8 @@ setup_upstream_and_workbench () {\n \t# we will use it in the subsequent test cases.\n \tupstream=\"$1\"\n \n-\t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n-\t# Workbench after setup : main(A)\n+\t# Upstream after setup: main(B) foo(A) bar(A) baz(A)\n+\t# Workbench after setup: main(A) baz(A) next(A)\n \ttest_expect_success \"setup upstream repository and workbench\" '\n \t\tsetup_upstream \"$upstream\" &&\n \t\trm -rf workbench &&\n@@ -105,6 +105,8 @@ setup_upstream_and_workbench () {\n \t\t(\n \t\t\tcd workbench &&\n \t\t\tgit update-ref refs/heads/main $A &&\n+\t\t\tgit update-ref refs/heads/baz $A &&\n+\t\t\tgit update-ref refs/heads/next $A &&\n \t\t\t# Try to make a stable fixed width for abbreviated commit ID,\n \t\t\t# this fixed-width oid will be replaced with \"\".\n \t\t\tgit config core.abbrev 7 &&\n@@ -133,19 +135,14 @@ run_git_push_porcelain_output_test() {\n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n-\ttest_expect_success \"porcelain output of successful git-push ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $A &&\n-\t\t\tgit update-ref refs/heads/baz $A &&\n-\t\t\tgit update-ref refs/heads/next $A &&\n-\t\t\tgit push --porcelain --force origin \\\n-\t\t\t\tmain \\\n-\t\t\t\t:refs/heads/foo \\\n-\t\t\t\t$B:bar \\\n-\t\t\t\tbaz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\ttest_expect_success \".. git-push --porcelain --force ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\tgit -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n \t\t> To \n@@ -169,115 +166,103 @@ run_git_push_porcelain_output_test() {\n \t\ttest_cmp expect actual\n \t'\n \n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"atomic push failed ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $B &&\n-\t\t\tgit update-ref refs/heads/bar $A &&\n-\t\t\ttest_must_fail git push --atomic --porcelain origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git push --porcelain --atomic ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_must_fail git -C workbench push --porcelain --atomic origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[rejected] (non-fast-forward)\n-\t\t> !\t(delete):refs/heads/baz\t[rejected] (atomic push failed)\n-\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (atomic push failed)\n-\t\tDone\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[rejected] (atomic push failed)\n+\t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n \t\t refs/heads/baz\n-\t\t refs/heads/main\n-\t\t refs/heads/next\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"prepare pre-receive hook ($PROTOCOL)\" '\n-\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF\n-\t\texit 1\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. pre-receive hook declined ($PROTOCOL)\" '\n+\t\ttest_when_finished \"rm -f \\\"$upstream/hooks/pre-receive\\\" &&\n+\t\t\tsetup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF &&\n+\t\t\texit 1\n \t\tEOF\n-\t'\n-\n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"pre-receive hook declined ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $B &&\n-\t\t\tgit update-ref refs/heads/bar $A &&\n-\t\t\ttest_must_fail git push --porcelain --force origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t\ttest_must_fail git -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n-\t\t> !\t:refs/heads/baz\t[remote rejected] (pre-receive hook declined)\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\t:refs/heads/foo\t[remote rejected] (pre-receive hook declined)\n \t\t> !\trefs/heads/main:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n-\t\tDone\n+\t\t> !\trefs/heads/next:refs/heads/next\t[remote rejected] (pre-receive hook declined)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n \t\t refs/heads/baz\n-\t\t refs/heads/main\n-\t\t refs/heads/next\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"remove pre-receive hook ($PROTOCOL)\" '\n-\t\trm \"$upstream/hooks/pre-receive\"\n-\t'\n-\n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"non-fastforward push ($PROTOCOL)\" '\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) next(A)\n+\ttest_expect_success \".. non-fastforward push ($PROTOCOL)\" '\n \t\t(\n \t\t\tcd workbench &&\n \t\t\ttest_must_fail git push --porcelain origin \\\n \t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n \t\t\t\tnext\n \t\t) >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> -\t:refs/heads/baz\t[deleted]\n-\t\t> \trefs/heads/main:refs/heads/main\t..\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[rejected] (non-fast-forward)\n-\t\tDone\n+\t\t> To \n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n \t\t refs/heads/main\n \t\t refs/heads/next\n \t\tEOF\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511543","messageId":"20250131-pks-push-atomic-respect-exit-code-v4-7-a8b41f01a676@pks.im","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im","subject":"[PATCH v4 7/8] t5543: atomic push reports exit code failure","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-31T10:53:31Z","receivedAt":"2025-01-31T10:53:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add new test cases in t5543 to avoid ignoring the exit code of\ngit-receive-pack(1) during atomic push with \"--porcelain\" flag.\n\nWe'd typically notice this case because the refs would have their error\nmessage set. But there is an edge case when pushing refs succeeds, but\ngit-receive-pack(1) exits with a non-zero exit code at a later point in\ntime due to another error. An atomic git-push(1) would ignore that error\ncode, and consequently it would return successfully and not print any\nerror message at all.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 04b47ad84a..32181b9afb 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -280,4 +280,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'atomic push reports exit code failure' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict 2>err &&\n+\tcat >expect <<-EOF &&\n+\tTo ../upstream\n+\t * [new branch] HEAD -> no-conflict\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure 'atomic push reports exit code failure with porcelain' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic --porcelain \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict-porcelain 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511544","messageId":"20250131-pks-push-atomic-respect-exit-code-v4-6-a8b41f01a676@pks.im","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im","subject":"[PATCH v4 6/8] send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-31T10:53:30Z","receivedAt":"2025-01-31T10:53:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jiang Xin \n\nThe \"push_refs\" function in the transport_vtable is the handler for\ngit-push operation. All the \"push_refs\" functions for different\ntransports (protocols) should have the same behavior, but the behavior\nof \"git_transport_push()\" function for builtin_smart_vtable in\n\"transport.c\" (which calls \"send_pack()\" in \"send-pack.c\") differs from\nthe handler of the HTTP protocol.\n\nThe \"push_refs()\" function for the HTTP protocol which calls the\n\"push_refs_with_push()\" function in \"transport-helper.c\" will return 0\neven when a bad REF_STATUS (such as REF_STATUS_REJECT_NONFASTFORWARD)\nwas found. But \"send_pack()\" for Git smart protocol will return -1 for\na bad REF_STATUS.\n\nWe cannot ignore bad REF_STATUS directly in the \"send_pack()\" function,\nbecause the function is also used in \"builtin/send-pack.c\". So we add a\nnew non-zero error code \"SEND_PACK_ERROR_REF_STATUS\" for \"send_pack()\".\n\nIgnore the specific error code in the \"git_transport_push()\" function to\nhave the same behavior as \"push_refs()\" for HTTP protocol. Note that\neven though we ignore the error here, we'll ultimately still end up\ndetecting that a subset of refs was not pushed in `transport_push()`\nbecause we eventually call `push_had_errors()` on the remote refs.\n\nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n send-pack.c | 9 ++-------\n send-pack.h | 13 +++++++++++++\n transport.c | 7 +++++++\n 3 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 772c7683a0..4448c081cc 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -632,7 +632,7 @@ int send_pack(struct repository *r,\n \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n-\t\t\t\tret = args->porcelain ? 0 : -1;\n+\t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\n@@ -763,11 +763,6 @@ int send_pack(struct repository *r,\n \tif (ret < 0)\n \t\tgoto out;\n \n-\tif (args->porcelain) {\n-\t\tret = 0;\n-\t\tgoto out;\n-\t}\n-\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\n@@ -775,7 +770,7 @@ int send_pack(struct repository *r,\n \t\tcase REF_STATUS_OK:\n \t\t\tbreak;\n \t\tdefault:\n-\t\t\tret = -1;\n+\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n \t\t\tgoto out;\n \t\t}\n \t}\ndiff --git a/send-pack.h b/send-pack.h\nindex d256715681..c5ded2d200 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -13,6 +13,9 @@ struct repository;\n #define SEND_PACK_PUSH_CERT_IF_ASKED 1\n #define SEND_PACK_PUSH_CERT_ALWAYS 2\n \n+/* At least one reference has been rejected by the remote side. */\n+#define ERROR_SEND_PACK_BAD_REF_STATUS 1\n+\n struct send_pack_args {\n \tconst char *url;\n \tunsigned verbose:1,\n@@ -36,6 +39,16 @@ struct option;\n int option_parse_push_signed(const struct option *opt,\n \t\t\t const char *arg, int unset);\n \n+/*\n+ * Compute a packfile and write it to a file descriptor. The `fd` array needs\n+ * to contain two file descriptors: `fd[0]` is the file descriptor used as\n+ * input for the packet reader, whereas `fd[1]` is the file descriptor the\n+ * packfile will be written to.\n+ *\n+ * Returns 0 on success, non-zero otherwise. Negative return values indicate a\n+ * generic error, whereas positive return values indicate specific error\n+ * conditions as documented with the `ERROR_SEND_PACK_*` constants.\n+ */\n int send_pack(struct repository *r, struct send_pack_args *args,\n \t int fd[], struct child_process *conn,\n \t struct ref *remote_refs, struct oid_array *extra_have);\ndiff --git a/transport.c b/transport.c\nindex 81ae8243b9..d064aff33e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -934,6 +934,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tcase protocol_v0:\n \t\tret = send_pack(the_repository, &args, data->fd, data->conn, remote_refs,\n \t\t\t\t&data->extra_have);\n+\t\t/*\n+\t\t * Ignore the specific error code to maintain consistent behavior\n+\t\t * with the \"push_refs()\" function across different transports,\n+\t\t * such as \"push_refs_with_push()\" for HTTP protocol.\n+\t\t */\n+\t\tif (ret == ERROR_SEND_PACK_BAD_REF_STATUS)\n+\t\t\tret = 0;\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511545","messageId":"20250131-pks-push-atomic-respect-exit-code-v4-8-a8b41f01a676@pks.im","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im","subject":"[PATCH v4 8/8] send-pack: gracefully close the connection for atomic push","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-31T10:53:32Z","receivedAt":"2025-01-31T10:53:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jiang Xin \n\nPatrick reported an issue that the exit code of git-receive-pack(1) is\nignored during atomic push with \"--porcelain\" flag, and added new test\ncases in t5543.\n\nThis issue originated from commit 7dcbeaa0df (send-pack: fix\ninconsistent porcelain output, 2020-04-17). At that time, I chose to\nignore the exit code of \"finish_connect()\" without investigating the\nroot cause of the abnormal termination of git-receive-pack. That was an\nincorrect solution.\n\nThe root cause is that an atomic push operation terminates early without\nsending a flush packet to git-receive-pack. As a result,\ngit-receive-pack continues waiting for commands without exiting. By\nsending a flush packet at the appropriate location in \"send_pack()\", we\nensure that the git-receive-pack process closes properly, avoiding an\nerroneous exit code for git-push. At the same time, revert the changes\nto the \"transport.c\" file made in commit 7dcbeaa0df.\n\nReported-by: Patrick Steinhardt \nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n send-pack.c | 1 +\n t/t5543-atomic-push.sh | 4 ++--\n transport.c | 10 +---------\n 3 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 4448c081cc..856a65d5f5 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -633,6 +633,7 @@ int send_pack(struct repository *r,\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n \t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n+\t\t\t\tpacket_flush(out);\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 32181b9afb..3a700b0676 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -280,7 +280,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'atomic push reports exit code failure' '\n+test_expect_success 'atomic push reports exit code failure' '\n \twrite_script receive-pack-wrapper <<-\\EOF &&\n \tgit-receive-pack \"$@\"\n \texit 1\n@@ -296,7 +296,7 @@ test_expect_failure 'atomic push reports exit code failure' '\n \ttest_cmp expect err\n '\n \n-test_expect_failure 'atomic push reports exit code failure with porcelain' '\n+test_expect_success 'atomic push reports exit code failure with porcelain' '\n \twrite_script receive-pack-wrapper <<-\\EOF &&\n \tgit-receive-pack \"$@\"\n \texit 1\ndiff --git a/transport.c b/transport.c\nindex d064aff33e..b0c6c339f4 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -948,15 +948,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \n \tclose(data->fd[1]);\n \tclose(data->fd[0]);\n-\t/*\n-\t * Atomic push may abort the connection early and close the pipe,\n-\t * which may cause an error for `finish_connect()`. Ignore this error\n-\t * for atomic git-push.\n-\t */\n-\tif (ret || args.atomic)\n-\t\tfinish_connect(data->conn);\n-\telse\n-\t\tret = finish_connect(data->conn);\n+\tret |= finish_connect(data->conn);\n \tdata->conn = NULL;\n \tdata->finished_handshake = 0;\n \n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511560","messageId":"CAPig+cRnm9z06YsCRyTJGqTCF_208BuD+3QoEKqwO7eWTfesAw@mail.gmail.com","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-1-a8b41f01a676@pks.im","subject":"Re: [PATCH v4 1/8] t5504: modernize test by moving heredocs into test bodies","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-01-31T14:28:01Z","receivedAt":"2025-01-31T14:28:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt wrote:\n> We have several heredocs in t5504 located outside of any particular test\n> bodies. Move these into the test bodies to match our modern coding\n> style.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n> @@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n> -cat >exp < -To dst\n> -! refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)\n> -Done\n> -EOF\n> -\n> test_expect_success 'push without strict' '\n> @@ -78,6 +72,11 @@ test_expect_success 'push without strict' '\n> + cat >exp <<-EOF &&\n> + To dst\n> + ! refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)\n> + Done\n> + EOF\n\nIt's minor, but to make this conform to modern style even more, it\nwould use `\\EOF` rather than `EOF`.\n\n(Probably not worth a reroll on its own.)\n\n> @@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '\n> -cat >exp < -To dst\n> -! refs/heads/main:refs/heads/test [remote rejected] (unpacker error)\n> -EOF\n> -\n> test_expect_success 'push with receive.fsckobjects' '\n> @@ -107,6 +101,10 @@ test_expect_success 'push with receive.fsckobjects' '\n> + cat >exp <<-EOF &&\n> + To dst\n> + ! refs/heads/main:refs/heads/test [remote rejected] (unpacker error)\n> + EOF\n\nDitto.\n\n> @@ -129,15 +127,14 @@ test_expect_success 'repair the \"corrupt or missing\" object' '\n> -cat >bogus-commit < -tree $EMPTY_TREE\n> -author Bugs Bunny 1234567890 +0000\n> -committer Bugs Bunny 1234567890 +0000\n> -\n> -This commit object intentionally broken\n> -EOF\n> -\n> test_expect_success 'setup bogus commit' '\n> + cat >bogus-commit <<-EOF &&\n> + tree $EMPTY_TREE\n> + author Bugs Bunny 1234567890 +0000\n> + committer Bugs Bunny 1234567890 +0000\n> +\n> + This commit object intentionally broken\n> + EOF\n\nThis one is correctly using `EOF` since it's interpolating variables.\n"},{"id":"511563","messageId":"CAPig+cQLdyuhK4bgf=HDuX8nDdBv8Voqf0-EtQO-8f15sPOVkw@mail.gmail.com","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-2-a8b41f01a676@pks.im","subject":"Re: [PATCH v4 2/8] t5548: refactor to reuse setup_upstream() function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-01-31T14:35:32Z","receivedAt":"2025-01-31T14:35:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt wrote:\n> Refactor the function setup_upstream_and_workbench(), extracting\n> create_upstream_template() and setup_upstream() from it. The former is\n> used to create the upstream repository template, while the latter is\n> used to rebuild the upstream repository and will be reused in subsequent\n> commits.\n>\n> To ensure that setup_upstream() works properly in both local and HTTP\n> protocols, the HTTP settings have been moved to the setup_upstream() and\n> setup_upstream_and_workbench() functions.\n>\n> Signed-off-by: Jiang Xin \n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\n> @@ -54,29 +54,67 @@ format_and_save_expect () {\n> setup_upstream_and_workbench () {\n> + if test $# -ne 1\n> + then\n> + BUG \"location of upstream repository is not provided\"\n> + fi &&\n\nIt's not clear why &&-chaining is being used here considering that\nthis function is not called by any tests and is itself not linked into\nany &&-chains, hence the use of && is superfluous and misleading.\n\n> + # Assign the first argument to the variable upstream;\n> + # we will use it in the subsequent test cases.\n> + upstream=\"$1\"\n\nComment doesn't seem to add any value.\n\n(Neither of these are worth a reroll on their own.)\n"},{"id":"511565","messageId":"CAPig+cRu20put-2WM=OfJeMmDe+1_jDKPeThbC9Vsw1EmHAp3w@mail.gmail.com","threadId":"62487","inReplyTo":"20250131-pks-push-atomic-respect-exit-code-v4-4-a8b41f01a676@pks.im","subject":"Re: [PATCH v4 4/8] t5548: add new porcelain test cases","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-01-31T14:46:13Z","receivedAt":"2025-01-31T14:46:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt wrote:\n> Add two more test cases exercising git-push(1) with `--procelain`, one\n> exercising a non-atomic and one exercising an atomic push.\n>\n> Based-on-patch-by: Jiang Xin \n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\n> @@ -132,6 +132,40 @@ run_git_push_porcelain_output_test() {\n> + # Refs of upstream : main(B) foo(A) bar(A) baz(A)\n> + # Refs of workbench: main(A) baz(A) next(A)\n> + # git-push : main(A) NULL (B) baz(A) next(A)\n> + test_expect_success \".. git-push --porcelain ($PROTOCOL)\" '\n> + test_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n> + test_must_fail git -C workbench push --porcelain origin \\\n> + main \\\n> + :refs/heads/foo \\\n> + $B:bar \\\n> + baz \\\n> + next >out &&\n> + make_user_friendly_and_stable_output actual &&\n> + format_and_save_expect <<-EOF &&\n> + > To \n> + > = refs/heads/baz:refs/heads/baz [up to date]\n> + > :refs/heads/bar ..\n> + > - :refs/heads/foo [deleted]\n> + > * refs/heads/next:refs/heads/next [new branch]\n> + > ! refs/heads/main:refs/heads/main [rejected] (non-fast-forward)\n> + > Done\n> + EOF\n\nUsing '\\EOF' rather than bare 'EOF' would be appropriate here and in\nthe other new heredocs added by this patch.\n\n(Not worth a reroll on its own.)\n"},{"id":"511578","messageId":"xmqqa5b79cgx.fsf@gitster.g","threadId":"62487","inReplyTo":"CAPig+cRnm9z06YsCRyTJGqTCF_208BuD+3QoEKqwO7eWTfesAw@mail.gmail.com","subject":"Re: [PATCH v4 1/8] t5504: modernize test by moving heredocs into test bodies","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-31T16:26:06Z","receivedAt":"2025-01-31T16:26:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> @@ -78,6 +72,11 @@ test_expect_success 'push without strict' '\n>> + cat >exp <<-EOF &&\n>> + To dst\n>> + ! refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)\n>> + Done\n>> + EOF\n>\n> It's minor, but to make this conform to modern style even more, it\n> would use `\\EOF` rather than `EOF`.\n>\n> (Probably not worth a reroll on its own.)\n\nYup. The rule of thumb is to quote the end-of-heredoc marker when\nthe here-doc does not need interpolation, to serve as a hint to tell\nthe readers that the here-doc is a literal text.\n"},{"id":"511677","messageId":"Z6Bch3I1UvL-qTcQ@pks.im","threadId":"62487","inReplyTo":"CAPig+cRnm9z06YsCRyTJGqTCF_208BuD+3QoEKqwO7eWTfesAw@mail.gmail.com","subject":"Re: [PATCH v4 1/8] t5504: modernize test by moving heredocs into test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:04:55Z","receivedAt":"2025-02-03T06:05:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 31, 2025 at 09:28:01AM -0500, Eric Sunshine wrote:\n> On Fri, Jan 31, 2025 at 5:53 AM Patrick Steinhardt wrote:\n> > We have several heredocs in t5504 located outside of any particular test\n> > bodies. Move these into the test bodies to match our modern coding\n> > style.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n> > @@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n> > -cat >exp < > -To dst\n> > -! refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)\n> > -Done\n> > -EOF\n> > -\n> > test_expect_success 'push without strict' '\n> > @@ -78,6 +72,11 @@ test_expect_success 'push without strict' '\n> > + cat >exp <<-EOF &&\n> > + To dst\n> > + ! refs/heads/main:refs/heads/test [remote rejected] (missing necessary objects)\n> > + Done\n> > + EOF\n> \n> It's minor, but to make this conform to modern style even more, it\n> would use `\\EOF` rather than `EOF`.\n\nYup, indeed. Will fix, thanks for your review!\n\nPatrick\n"},{"id":"511679","messageId":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","threadId":"62487","inReplyTo":"20241113-pks-push-atomic-respect-exit-code-v1-0-7965f01e7f4e@pks.im","subject":"[PATCH v5 0/8] transport: don't ignore git-receive-pack(1) exit code on atomic push","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:29:30Z","receivedAt":"2025-02-03T06:29:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwe've hit an edge case at GitLab where an atomic push will not notice an\nerror when git-receive-pack(1) updates the refs, but otherwise fails\nwith a non-zero exit code. The push would be successful and no error\nwould be printed even though some things have gone wrong on the remote\nside.\n\nAs promised last week, I've now adopted the patch series from Jiang Xin\nto make some progress on the issue. The series is now based on the\nlatest master branch at 3b0d05c4a7 (The fifth batch, 2025-01-29).\n\nChanges in v4:\n - Rewrite the commit that adds two new tests for `--porcelain`.\n Previously, the commit both reordered existing tests and added new\n ones, which made it hard to see what actually changed. The reorder\n wasn't necessary though, so I've adapted it to only add new tests\n now.\n - Fix the URL prefix in t5548 to also work on Windows.\n - Document why it's fine to start ignoring the return value of\n `git_transport_push()`.\n - Improve comments to document behaviour better.\n - Link to v3: https://lore.kernel.org/r/cover.1733830410.git.zhiyou.jx@alibaba-inc.com\n\nChanges in v5:\n - Escape heredocs where possible.\n - Link to v4: https://lore.kernel.org/r/20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im\n\nThanks!\n\nPatrick\n\n---\nJiang Xin (5):\n t5548: refactor to reuse setup_upstream() function\n t5548: refactor test cases by resetting upstream\n t5548: add porcelain push test cases for dry-run mode\n send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"\n send-pack: gracefully close the connection for atomic push\n\nPatrick Steinhardt (3):\n t5504: modernize test by moving heredocs into test bodies\n t5548: add new porcelain test cases\n t5543: atomic push reports exit code failure\n\n send-pack.c | 10 +-\n send-pack.h | 13 ++\n t/t5504-fetch-receive-strict.sh | 35 ++--\n t/t5543-atomic-push.sh | 30 +++\n t/t5548-push-porcelain.sh | 443 ++++++++++++++++++++++++++++++----------\n transport.c | 17 +-\n 6 files changed, 406 insertions(+), 142 deletions(-)\n\nRange-diff versus v4:\n\n1: ef3732a280 ! 1: 2d04ec6ca3 t5504: modernize test by moving heredocs into test bodies\n @@ t/t5504-fetch-receive-strict.sh: test_expect_success 'push without strict' '\n \t\tgit config fetch.fsckobjects false &&\n \t\tgit config transfer.fsckobjects false\n \t) &&\n -+\tcat >exp <<-EOF &&\n ++\tcat >exp <<-\\EOF &&\n +\tTo dst\n +\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (missing necessary objects)\n +\tDone\n @@ t/t5504-fetch-receive-strict.sh: test_expect_success 'push with receive.fsckobje\n \t\tgit config receive.fsckobjects true &&\n \t\tgit config transfer.fsckobjects false\n \t) &&\n -+\tcat >exp <<-EOF &&\n ++\tcat >exp <<-\\EOF &&\n +\tTo dst\n +\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (unpacker error)\n +\tEOF\n2: c4a57bf457 ! 2: 31c8b10e7d t5548: refactor to reuse setup_upstream() function\n @@ t/t5548-push-porcelain.sh: format_and_save_expect () {\n +\tif test $# -ne 1\n +\tthen\n +\t\tBUG \"location of upstream repository is not provided\"\n -+\tfi &&\n -+\t# Assign the first argument to the variable upstream;\n -+\t# we will use it in the subsequent test cases.\n ++\tfi\n +\tupstream=\"$1\"\n +\n \t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n3: 67928693cc ! 3: a8de197677 t5548: refactor test cases by resetting upstream\n @@ Commit message\n \n ## t/t5548-push-porcelain.sh ##\n @@ t/t5548-push-porcelain.sh: setup_upstream_and_workbench () {\n - \t# we will use it in the subsequent test cases.\n + \tfi\n \tupstream=\"$1\"\n \n -\t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n4: 6e0f0f791f ! 4: 9e4f3be9e0 t5548: add new porcelain test cases\n @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {\n +\t\t\tbaz \\\n +\t\t\tnext >out &&\n +\t\tmake_user_friendly_and_stable_output actual &&\n -+\t\tformat_and_save_expect <<-EOF &&\n ++\t\tformat_and_save_expect <<-\\EOF &&\n +\t\t> To \n +\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n +\t\t> \t:refs/heads/bar\t..\n @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {\n +\t\t\tbaz \\\n +\t\t\tnext >out &&\n +\t\tmake_user_friendly_and_stable_output actual &&\n -+\t\tformat_and_save_expect <<-EOF &&\n ++\t\tformat_and_save_expect <<-\\EOF &&\n +\t\t> To \n +\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n +\t\t> \t:refs/heads/bar\t..\n5: cb985baec9 = 5: 093b50d785 t5548: add porcelain push test cases for dry-run mode\n6: 68ae698e4b = 6: 9a1851f06e send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"\n7: 525cefd4f2 = 7: e789913922 t5543: atomic push reports exit code failure\n8: 19cdbf991f = 8: 52101a1f14 send-pack: gracefully close the connection for atomic push\n\n---\nbase-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08\nchange-id: 20241113-pks-push-atomic-respect-exit-code-436c443a657d\n\n"},{"id":"511680","messageId":"20250203-pks-push-atomic-respect-exit-code-v5-1-d66481e36622@pks.im","threadId":"62487","inReplyTo":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","subject":"[PATCH v5 1/8] t5504: modernize test by moving heredocs into test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:29:31Z","receivedAt":"2025-02-03T06:29:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have several heredocs in t5504 located outside of any particular test\nbodies. Move these into the test bodies to match our modern coding\nstyle.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5504-fetch-receive-strict.sh | 35 ++++++++++++++++-------------------\n 1 file changed, 16 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex e273ab29c7..58074506c5 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -64,12 +64,6 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n \t)\n '\n \n-cat >exp <exp <<-\\EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (missing necessary objects)\n+\tDone\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -94,11 +93,6 @@ test_expect_success 'push with !receive.fsckobjects' '\n \ttest_cmp exp act\n '\n \n-cat >exp <exp <<-\\EOF &&\n+\tTo dst\n+\t!\trefs/heads/main:refs/heads/test\t[remote rejected] (unpacker error)\n+\tEOF\n \ttest_must_fail git push --porcelain dst main:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n@@ -129,15 +127,14 @@ test_expect_success 'repair the \"corrupt or missing\" object' '\n \tgit fsck\n '\n \n-cat >bogus-commit < 1234567890 +0000\n-\n-This commit object intentionally broken\n-EOF\n-\n test_expect_success 'setup bogus commit' '\n+\tcat >bogus-commit <<-EOF &&\n+\ttree $EMPTY_TREE\n+\tauthor Bugs Bunny 1234567890 +0000\n+\tcommitter Bugs Bunny 1234567890 +0000\n+\n+\tThis commit object intentionally broken\n+\tEOF\n \tcommit=\"$(git hash-object --literally -t commit -w --stdin \n\nRefactor the function setup_upstream_and_workbench(), extracting\ncreate_upstream_template() and setup_upstream() from it. The former is\nused to create the upstream repository template, while the latter is\nused to rebuild the upstream repository and will be reused in subsequent\ncommits.\n\nTo ensure that setup_upstream() works properly in both local and HTTP\nprotocols, the HTTP settings have been moved to the setup_upstream() and\nsetup_upstream_and_workbench() functions.\n\nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n t/t5548-push-porcelain.sh | 83 ++++++++++++++++++++++++++++++-----------------\n 1 file changed, 53 insertions(+), 30 deletions(-)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex 6282728eaf..1bf4d48cd9 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -54,29 +54,65 @@ format_and_save_expect () {\n \tsed -e 's/^> //' -e 's/Z$//' >expect\n }\n \n+create_upstream_template () {\n+\tgit init --bare upstream-template.git &&\n+\tgit clone upstream-template.git tmp_work_dir &&\n+\tcreate_commits_in tmp_work_dir A B &&\n+\t(\n+\t\tcd tmp_work_dir &&\n+\t\tgit push origin \\\n+\t\t\t$B:refs/heads/main \\\n+\t\t\t$A:refs/heads/foo \\\n+\t\t\t$A:refs/heads/bar \\\n+\t\t\t$A:refs/heads/baz\n+\t) &&\n+\trm -rf tmp_work_dir\n+}\n+\n+setup_upstream () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tBUG \"location of upstream repository is not provided\"\n+\tfi &&\n+\trm -rf \"$1\" &&\n+\tif ! test -d upstream-template.git\n+\tthen\n+\t\tcreate_upstream_template\n+\tfi &&\n+\tgit clone --mirror upstream-template.git \"$1\" &&\n+\t# The upstream repository provides services using the HTTP protocol.\n+\tif ! test \"$1\" = \"upstream.git\"\n+\tthen\n+\t\tgit -C \"$1\" config http.receivepack true\n+\tfi\n+}\n+\n setup_upstream_and_workbench () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tBUG \"location of upstream repository is not provided\"\n+\tfi\n+\tupstream=\"$1\"\n+\n \t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n \t# Workbench after setup : main(A)\n \ttest_expect_success \"setup upstream repository and workbench\" '\n-\t\trm -rf upstream.git workbench &&\n-\t\tgit init --bare upstream.git &&\n-\t\tgit init workbench &&\n-\t\tcreate_commits_in workbench A B &&\n+\t\tsetup_upstream \"$upstream\" &&\n+\t\trm -rf workbench &&\n+\t\tgit clone \"$upstream\" workbench &&\n \t\t(\n \t\t\tcd workbench &&\n+\t\t\tgit update-ref refs/heads/main $A &&\n \t\t\t# Try to make a stable fixed width for abbreviated commit ID,\n \t\t\t# this fixed-width oid will be replaced with \"\".\n \t\t\tgit config core.abbrev 7 &&\n-\t\t\tgit remote add origin ../upstream.git &&\n-\t\t\tgit update-ref refs/heads/main $A &&\n-\t\t\tgit push origin \\\n-\t\t\t\t$B:refs/heads/main \\\n-\t\t\t\t$A:refs/heads/foo \\\n-\t\t\t\t$A:refs/heads/bar \\\n-\t\t\t\t$A:refs/heads/baz\n+\t\t\tgit config advice.pushUpdateRejected false\n \t\t) &&\n-\t\tgit -C \"workbench\" config advice.pushUpdateRejected false &&\n-\t\tupstream=upstream.git\n+\t\t# The upstream repository provides services using the HTTP protocol.\n+\t\tif ! test \"$upstream\" = \"upstream.git\"\n+\t\tthen\n+\t\t\tgit -C workbench remote set-url origin \"$HTTPD_URL/smart/upstream.git\"\n+\t\tfi\n \t'\n }\n \n@@ -88,7 +124,7 @@ run_git_push_porcelain_output_test() {\n \t\t;;\n \tfile)\n \t\tPROTOCOL=\"builtin protocol\"\n-\t\tURL_PREFIX=\"\\.\\.\"\n+\t\tURL_PREFIX=\".*\"\n \t\t;;\n \tesac\n \n@@ -247,10 +283,8 @@ run_git_push_porcelain_output_test() {\n \t'\n }\n \n-# Initialize the upstream repository and local workbench.\n-setup_upstream_and_workbench\n+setup_upstream_and_workbench upstream.git\n \n-# Run git-push porcelain test on builtin protocol\n run_git_push_porcelain_output_test file\n \n ROOT_PATH=\"$PWD\"\n@@ -258,21 +292,10 @@ ROOT_PATH=\"$PWD\"\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n start_httpd\n-\n-# Re-initialize the upstream repository and local workbench.\n-setup_upstream_and_workbench\n-\n-test_expect_success \"setup for http\" '\n-\tgit -C upstream.git config http.receivepack true &&\n-\tupstream=\"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\" &&\n-\tmv upstream.git \"$upstream\" &&\n-\n-\tgit -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git\n-'\n-\n setup_askpass_helper\n \n-# Run git-push porcelain test on HTTP protocol\n+setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n+\n run_git_push_porcelain_output_test http\n \n test_done\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511682","messageId":"20250203-pks-push-atomic-respect-exit-code-v5-3-d66481e36622@pks.im","threadId":"62487","inReplyTo":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","subject":"[PATCH v5 3/8] t5548: refactor test cases by resetting upstream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:29:33Z","receivedAt":"2025-02-03T06:29:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jiang Xin \n\nRefactor the test cases with the following changes:\n\n - Calling setup_upstream() to reset upstream after running each test\n case.\n\n - Change the initial branch tips of the workspace to reduce the branch\n setup operations in the workspace.\n\n - Reduced the two steps of setting up and cleaning up the pre-receive\n hook by moving the operations into the corresponding test case,\n\nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n t/t5548-push-porcelain.sh | 149 +++++++++++++++++++++-------------------------\n 1 file changed, 67 insertions(+), 82 deletions(-)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex 1bf4d48cd9..f9aeb5a9d9 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -94,8 +94,8 @@ setup_upstream_and_workbench () {\n \tfi\n \tupstream=\"$1\"\n \n-\t# Upstream after setup : main(B) foo(A) bar(A) baz(A)\n-\t# Workbench after setup : main(A)\n+\t# Upstream after setup: main(B) foo(A) bar(A) baz(A)\n+\t# Workbench after setup: main(A) baz(A) next(A)\n \ttest_expect_success \"setup upstream repository and workbench\" '\n \t\tsetup_upstream \"$upstream\" &&\n \t\trm -rf workbench &&\n@@ -103,6 +103,8 @@ setup_upstream_and_workbench () {\n \t\t(\n \t\t\tcd workbench &&\n \t\t\tgit update-ref refs/heads/main $A &&\n+\t\t\tgit update-ref refs/heads/baz $A &&\n+\t\t\tgit update-ref refs/heads/next $A &&\n \t\t\t# Try to make a stable fixed width for abbreviated commit ID,\n \t\t\t# this fixed-width oid will be replaced with \"\".\n \t\t\tgit config core.abbrev 7 &&\n@@ -131,19 +133,14 @@ run_git_push_porcelain_output_test() {\n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n-\ttest_expect_success \"porcelain output of successful git-push ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $A &&\n-\t\t\tgit update-ref refs/heads/baz $A &&\n-\t\t\tgit update-ref refs/heads/next $A &&\n-\t\t\tgit push --porcelain --force origin \\\n-\t\t\t\tmain \\\n-\t\t\t\t:refs/heads/foo \\\n-\t\t\t\t$B:bar \\\n-\t\t\t\tbaz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\ttest_expect_success \".. git-push --porcelain --force ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\tgit -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n \t\t> To \n@@ -167,115 +164,103 @@ run_git_push_porcelain_output_test() {\n \t\ttest_cmp expect actual\n \t'\n \n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"atomic push failed ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $B &&\n-\t\t\tgit update-ref refs/heads/bar $A &&\n-\t\t\ttest_must_fail git push --atomic --porcelain origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git push --porcelain --atomic ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_must_fail git -C workbench push --porcelain --atomic origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[rejected] (non-fast-forward)\n-\t\t> !\t(delete):refs/heads/baz\t[rejected] (atomic push failed)\n-\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (atomic push failed)\n-\t\tDone\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[rejected] (atomic push failed)\n+\t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n \t\t refs/heads/baz\n-\t\t refs/heads/main\n-\t\t refs/heads/next\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"prepare pre-receive hook ($PROTOCOL)\" '\n-\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF\n-\t\texit 1\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. pre-receive hook declined ($PROTOCOL)\" '\n+\t\ttest_when_finished \"rm -f \\\"$upstream/hooks/pre-receive\\\" &&\n+\t\t\tsetup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_hook --setup -C \"$upstream\" pre-receive <<-EOF &&\n+\t\t\texit 1\n \t\tEOF\n-\t'\n-\n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"pre-receive hook declined ($PROTOCOL)\" '\n-\t\t(\n-\t\t\tcd workbench &&\n-\t\t\tgit update-ref refs/heads/main $B &&\n-\t\t\tgit update-ref refs/heads/bar $A &&\n-\t\t\ttest_must_fail git push --porcelain --force origin \\\n-\t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n-\t\t\t\tnext\n-\t\t) >out &&\n+\t\ttest_must_fail git -C workbench push --porcelain --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n-\t\t> !\t:refs/heads/baz\t[remote rejected] (pre-receive hook declined)\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[remote rejected] (pre-receive hook declined)\n+\t\t> !\t:refs/heads/foo\t[remote rejected] (pre-receive hook declined)\n \t\t> !\trefs/heads/main:refs/heads/main\t[remote rejected] (pre-receive hook declined)\n-\t\tDone\n+\t\t> !\trefs/heads/next:refs/heads/next\t[remote rejected] (pre-receive hook declined)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n \t\t refs/heads/baz\n-\t\t refs/heads/main\n-\t\t refs/heads/next\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"remove pre-receive hook ($PROTOCOL)\" '\n-\t\trm \"$upstream/hooks/pre-receive\"\n-\t'\n-\n-\t# Refs of upstream : main(A) bar(B) baz(A) next(A)\n-\t# Refs of workbench: main(B) bar(A) baz(A) next(A)\n-\t# git-push : main(B) bar(A) NULL next(A)\n-\ttest_expect_success \"non-fastforward push ($PROTOCOL)\" '\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) next(A)\n+\ttest_expect_success \".. non-fastforward push ($PROTOCOL)\" '\n \t\t(\n \t\t\tcd workbench &&\n \t\t\ttest_must_fail git push --porcelain origin \\\n \t\t\t\tmain \\\n-\t\t\t\tbar \\\n-\t\t\t\t:baz \\\n \t\t\t\tnext\n \t\t) >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tformat_and_save_expect <<-EOF &&\n-\t\tTo \n-\t\t> =\trefs/heads/next:refs/heads/next\t[up to date]\n-\t\t> -\t:refs/heads/baz\t[deleted]\n-\t\t> \trefs/heads/main:refs/heads/main\t..\n-\t\t> !\trefs/heads/bar:refs/heads/bar\t[rejected] (non-fast-forward)\n-\t\tDone\n+\t\t> To \n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n \t\tgit -C \"$upstream\" show-ref >out &&\n \t\tmake_user_friendly_and_stable_output actual &&\n \t\tcat >expect <<-EOF &&\n-\t\t refs/heads/bar\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n \t\t refs/heads/main\n \t\t refs/heads/next\n \t\tEOF\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511683","messageId":"20250203-pks-push-atomic-respect-exit-code-v5-4-d66481e36622@pks.im","threadId":"62487","inReplyTo":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","subject":"[PATCH v5 4/8] t5548: add new porcelain test cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:29:34Z","receivedAt":"2025-02-03T06:29:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add two more test cases exercising git-push(1) with `--procelain`, one\nexercising a non-atomic and one exercising an atomic push.\n\nBased-on-patch-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n t/t5548-push-porcelain.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 68 insertions(+)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex f9aeb5a9d9..5d724145d2 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -130,6 +130,40 @@ run_git_push_porcelain_output_test() {\n \t\t;;\n \tesac\n \n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n+\t\ttest_must_fail git -C workbench push --porcelain origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-\\EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/main\n+\t\t refs/heads/next\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n \t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) NULL (B) baz(A) next(A)\n@@ -240,6 +274,7 @@ run_git_push_porcelain_output_test() {\n \t# Refs of workbench: main(A) baz(A) next(A)\n \t# git-push : main(A) next(A)\n \ttest_expect_success \".. non-fastforward push ($PROTOCOL)\" '\n+\t\ttest_when_finished \"setup_upstream \\\"$upstream\\\"\" &&\n \t\t(\n \t\t\tcd workbench &&\n \t\t\ttest_must_fail git push --porcelain origin \\\n@@ -266,6 +301,39 @@ run_git_push_porcelain_output_test() {\n \t\tEOF\n \t\ttest_cmp expect actual\n \t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git push --porcelain --atomic --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --atomic --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-\\EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/main\n+\t\t refs/heads/next\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n }\n \n setup_upstream_and_workbench upstream.git\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511685","messageId":"20250203-pks-push-atomic-respect-exit-code-v5-5-d66481e36622@pks.im","threadId":"62487","inReplyTo":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","subject":"[PATCH v5 5/8] t5548: add porcelain push test cases for dry-run mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:29:35Z","receivedAt":"2025-02-03T06:29:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jiang Xin \n\nNew dry-run test cases:\n\n - git push --porcelain --dry-run\n - git push --porcelain --dry-run --force\n - git push --porcelain --dry-run --atomic\n - git push --porcelain --dry-run --atomic --force\n\nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n t/t5548-push-porcelain.sh | 153 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 153 insertions(+)\n\ndiff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh\nindex 5d724145d2..4c19404ebe 100755\n--- a/t/t5548-push-porcelain.sh\n+++ b/t/t5548-push-porcelain.sh\n@@ -336,10 +336,159 @@ run_git_push_porcelain_output_test() {\n \t'\n }\n \n+run_git_push_dry_run_porcelain_output_test() {\n+\tcase $1 in\n+\thttp)\n+\t\tPROTOCOL=\"HTTP protocol\"\n+\t\tURL_PREFIX=\"http://.*\"\n+\t\t;;\n+\tfile)\n+\t\tPROTOCOL=\"builtin protocol\"\n+\t\tURL_PREFIX=\".*\"\n+\t\t;;\n+\tesac\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run ($PROTOCOL)\" '\n+\t\ttest_must_fail git -C workbench push --porcelain --dry-run origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --dry-run --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# git-push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --atomic ($PROTOCOL)\" '\n+\t\ttest_must_fail git -C workbench push --porcelain --dry-run --atomic origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> !\t:refs/heads/bar\t[rejected] (atomic push failed)\n+\t\t> !\t(delete):refs/heads/foo\t[rejected] (atomic push failed)\n+\t\t> !\trefs/heads/main:refs/heads/main\t[rejected] (non-fast-forward)\n+\t\t> !\trefs/heads/next:refs/heads/next\t[rejected] (atomic push failed)\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\t# Refs of upstream : main(B) foo(A) bar(A) baz(A)\n+\t# Refs of workbench: main(A) baz(A) next(A)\n+\t# push : main(A) NULL (B) baz(A) next(A)\n+\ttest_expect_success \".. git-push --porcelain --dry-run --atomic --force ($PROTOCOL)\" '\n+\t\tgit -C workbench push --porcelain --dry-run --atomic --force origin \\\n+\t\t\tmain \\\n+\t\t\t:refs/heads/foo \\\n+\t\t\t$B:bar \\\n+\t\t\tbaz \\\n+\t\t\tnext >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tformat_and_save_expect <<-EOF &&\n+\t\t> To \n+\t\t> =\trefs/heads/baz:refs/heads/baz\t[up to date]\n+\t\t> \t:refs/heads/bar\t..\n+\t\t> -\t:refs/heads/foo\t[deleted]\n+\t\t> +\trefs/heads/main:refs/heads/main\t... (forced update)\n+\t\t> *\trefs/heads/next:refs/heads/next\t[new branch]\n+\t\t> Done\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit -C \"$upstream\" show-ref >out &&\n+\t\tmake_user_friendly_and_stable_output actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t refs/heads/bar\n+\t\t refs/heads/baz\n+\t\t refs/heads/foo\n+\t\t refs/heads/main\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t'\n+}\n+\n setup_upstream_and_workbench upstream.git\n \n run_git_push_porcelain_output_test file\n \n+setup_upstream_and_workbench upstream.git\n+\n+run_git_push_dry_run_porcelain_output_test file\n+\n ROOT_PATH=\"$PWD\"\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n@@ -351,4 +500,8 @@ setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n \n run_git_push_porcelain_output_test http\n \n+setup_upstream_and_workbench \"$HTTPD_DOCUMENT_ROOT_PATH/upstream.git\"\n+\n+run_git_push_dry_run_porcelain_output_test http\n+\n test_done\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511684","messageId":"20250203-pks-push-atomic-respect-exit-code-v5-6-d66481e36622@pks.im","threadId":"62487","inReplyTo":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","subject":"[PATCH v5 6/8] send-pack: new return code \"ERROR_SEND_PACK_BAD_REF_STATUS\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:29:36Z","receivedAt":"2025-02-03T06:29:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jiang Xin \n\nThe \"push_refs\" function in the transport_vtable is the handler for\ngit-push operation. All the \"push_refs\" functions for different\ntransports (protocols) should have the same behavior, but the behavior\nof \"git_transport_push()\" function for builtin_smart_vtable in\n\"transport.c\" (which calls \"send_pack()\" in \"send-pack.c\") differs from\nthe handler of the HTTP protocol.\n\nThe \"push_refs()\" function for the HTTP protocol which calls the\n\"push_refs_with_push()\" function in \"transport-helper.c\" will return 0\neven when a bad REF_STATUS (such as REF_STATUS_REJECT_NONFASTFORWARD)\nwas found. But \"send_pack()\" for Git smart protocol will return -1 for\na bad REF_STATUS.\n\nWe cannot ignore bad REF_STATUS directly in the \"send_pack()\" function,\nbecause the function is also used in \"builtin/send-pack.c\". So we add a\nnew non-zero error code \"SEND_PACK_ERROR_REF_STATUS\" for \"send_pack()\".\n\nIgnore the specific error code in the \"git_transport_push()\" function to\nhave the same behavior as \"push_refs()\" for HTTP protocol. Note that\neven though we ignore the error here, we'll ultimately still end up\ndetecting that a subset of refs was not pushed in `transport_push()`\nbecause we eventually call `push_had_errors()` on the remote refs.\n\nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n send-pack.c | 9 ++-------\n send-pack.h | 13 +++++++++++++\n transport.c | 7 +++++++\n 3 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 772c7683a0..4448c081cc 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -632,7 +632,7 @@ int send_pack(struct repository *r,\n \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n-\t\t\t\tret = args->porcelain ? 0 : -1;\n+\t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\n@@ -763,11 +763,6 @@ int send_pack(struct repository *r,\n \tif (ret < 0)\n \t\tgoto out;\n \n-\tif (args->porcelain) {\n-\t\tret = 0;\n-\t\tgoto out;\n-\t}\n-\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\n@@ -775,7 +770,7 @@ int send_pack(struct repository *r,\n \t\tcase REF_STATUS_OK:\n \t\t\tbreak;\n \t\tdefault:\n-\t\t\tret = -1;\n+\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n \t\t\tgoto out;\n \t\t}\n \t}\ndiff --git a/send-pack.h b/send-pack.h\nindex d256715681..c5ded2d200 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -13,6 +13,9 @@ struct repository;\n #define SEND_PACK_PUSH_CERT_IF_ASKED 1\n #define SEND_PACK_PUSH_CERT_ALWAYS 2\n \n+/* At least one reference has been rejected by the remote side. */\n+#define ERROR_SEND_PACK_BAD_REF_STATUS 1\n+\n struct send_pack_args {\n \tconst char *url;\n \tunsigned verbose:1,\n@@ -36,6 +39,16 @@ struct option;\n int option_parse_push_signed(const struct option *opt,\n \t\t\t const char *arg, int unset);\n \n+/*\n+ * Compute a packfile and write it to a file descriptor. The `fd` array needs\n+ * to contain two file descriptors: `fd[0]` is the file descriptor used as\n+ * input for the packet reader, whereas `fd[1]` is the file descriptor the\n+ * packfile will be written to.\n+ *\n+ * Returns 0 on success, non-zero otherwise. Negative return values indicate a\n+ * generic error, whereas positive return values indicate specific error\n+ * conditions as documented with the `ERROR_SEND_PACK_*` constants.\n+ */\n int send_pack(struct repository *r, struct send_pack_args *args,\n \t int fd[], struct child_process *conn,\n \t struct ref *remote_refs, struct oid_array *extra_have);\ndiff --git a/transport.c b/transport.c\nindex 81ae8243b9..d064aff33e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -934,6 +934,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tcase protocol_v0:\n \t\tret = send_pack(the_repository, &args, data->fd, data->conn, remote_refs,\n \t\t\t\t&data->extra_have);\n+\t\t/*\n+\t\t * Ignore the specific error code to maintain consistent behavior\n+\t\t * with the \"push_refs()\" function across different transports,\n+\t\t * such as \"push_refs_with_push()\" for HTTP protocol.\n+\t\t */\n+\t\tif (ret == ERROR_SEND_PACK_BAD_REF_STATUS)\n+\t\t\tret = 0;\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511687","messageId":"20250203-pks-push-atomic-respect-exit-code-v5-7-d66481e36622@pks.im","threadId":"62487","inReplyTo":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","subject":"[PATCH v5 7/8] t5543: atomic push reports exit code failure","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:29:37Z","receivedAt":"2025-02-03T06:29:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add new test cases in t5543 to avoid ignoring the exit code of\ngit-receive-pack(1) during atomic push with \"--porcelain\" flag.\n\nWe'd typically notice this case because the refs would have their error\nmessage set. But there is an edge case when pushing refs succeeds, but\ngit-receive-pack(1) exits with a non-zero exit code at a later point in\ntime due to another error. An atomic git-push(1) would ignore that error\ncode, and consequently it would return successfully and not print any\nerror message at all.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 04b47ad84a..32181b9afb 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -280,4 +280,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'atomic push reports exit code failure' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict 2>err &&\n+\tcat >expect <<-EOF &&\n+\tTo ../upstream\n+\t * [new branch] HEAD -> no-conflict\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure 'atomic push reports exit code failure with porcelain' '\n+\twrite_script receive-pack-wrapper <<-\\EOF &&\n+\tgit-receive-pack \"$@\"\n+\texit 1\n+\tEOF\n+\ttest_must_fail git -C workbench push --atomic --porcelain \\\n+\t\t--receive-pack=\"${SQ}$(pwd)${SQ}/receive-pack-wrapper\" \\\n+\t\tup HEAD:refs/heads/no-conflict-porcelain 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: failed to push some refs to ${SQ}../upstream${SQ}\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511686","messageId":"20250203-pks-push-atomic-respect-exit-code-v5-8-d66481e36622@pks.im","threadId":"62487","inReplyTo":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","subject":"[PATCH v5 8/8] send-pack: gracefully close the connection for atomic push","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T06:29:38Z","receivedAt":"2025-02-03T06:29:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jiang Xin \n\nPatrick reported an issue that the exit code of git-receive-pack(1) is\nignored during atomic push with \"--porcelain\" flag, and added new test\ncases in t5543.\n\nThis issue originated from commit 7dcbeaa0df (send-pack: fix\ninconsistent porcelain output, 2020-04-17). At that time, I chose to\nignore the exit code of \"finish_connect()\" without investigating the\nroot cause of the abnormal termination of git-receive-pack. That was an\nincorrect solution.\n\nThe root cause is that an atomic push operation terminates early without\nsending a flush packet to git-receive-pack. As a result,\ngit-receive-pack continues waiting for commands without exiting. By\nsending a flush packet at the appropriate location in \"send_pack()\", we\nensure that the git-receive-pack process closes properly, avoiding an\nerroneous exit code for git-push. At the same time, revert the changes\nto the \"transport.c\" file made in commit 7dcbeaa0df.\n\nReported-by: Patrick Steinhardt \nSigned-off-by: Jiang Xin \nSigned-off-by: Patrick Steinhardt \n---\n send-pack.c | 1 +\n t/t5543-atomic-push.sh | 4 ++--\n transport.c | 10 +---------\n 3 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 4448c081cc..856a65d5f5 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -633,6 +633,7 @@ int send_pack(struct repository *r,\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\",\n \t\t\t\t ref->name, ref->status);\n \t\t\t\tret = ERROR_SEND_PACK_BAD_REF_STATUS;\n+\t\t\t\tpacket_flush(out);\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 32181b9afb..3a700b0676 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -280,7 +280,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'atomic push reports exit code failure' '\n+test_expect_success 'atomic push reports exit code failure' '\n \twrite_script receive-pack-wrapper <<-\\EOF &&\n \tgit-receive-pack \"$@\"\n \texit 1\n@@ -296,7 +296,7 @@ test_expect_failure 'atomic push reports exit code failure' '\n \ttest_cmp expect err\n '\n \n-test_expect_failure 'atomic push reports exit code failure with porcelain' '\n+test_expect_success 'atomic push reports exit code failure with porcelain' '\n \twrite_script receive-pack-wrapper <<-\\EOF &&\n \tgit-receive-pack \"$@\"\n \texit 1\ndiff --git a/transport.c b/transport.c\nindex d064aff33e..b0c6c339f4 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -948,15 +948,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \n \tclose(data->fd[1]);\n \tclose(data->fd[0]);\n-\t/*\n-\t * Atomic push may abort the connection early and close the pipe,\n-\t * which may cause an error for `finish_connect()`. Ignore this error\n-\t * for atomic git-push.\n-\t */\n-\tif (ret || args.atomic)\n-\t\tfinish_connect(data->conn);\n-\telse\n-\t\tret = finish_connect(data->conn);\n+\tret |= finish_connect(data->conn);\n \tdata->conn = NULL;\n \tdata->finished_handshake = 0;\n \n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511766","messageId":"xmqqh65azk3a.fsf@gitster.g","threadId":"62487","inReplyTo":"20250203-pks-push-atomic-respect-exit-code-v5-0-d66481e36622@pks.im","subject":"Re: [PATCH v5 0/8] transport: don't ignore git-receive-pack(1) exit code on atomic push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-03T23:26:01Z","receivedAt":"2025-02-03T23:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Changes in v5:\n> - Escape heredocs where possible.\n> - Link to v4: https://lore.kernel.org/r/20250131-pks-push-atomic-respect-exit-code-v4-0-a8b41f01a676@pks.im\n>\n> Thanks!\n\nChanges since the last round looked trivially correct ;-)\n\nWill replace.\n"}]}