| Message ID | 20241113-pks-push-atomic-respect-exit-code-v1-2-7965f01e7f4e@pks.im (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | transport: don't ignore git-receive-pack(1) exit code on atomic push | expand |
On Wed, Nov 13, 2024 at 7:25 PM Patrick Steinhardt <ps@pks.im> wrote: > > When executing git-push(1) with the "--porcelain" flag, then we will > print updated references in a machine-readable format that looks like > this: > > To destination > = refs/heads/noop:refs/heads/noop [up to date] > ! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed) > ! refs/heads/noff:refs/heads/(off (non-fast-forward) > Done > > The final "Done" stanza was introduced via 77555854be (git-push: make > git push --porcelain print "Done", 2010-02-26), where it was printed > "unless any errors have occurred". This behaviour was later changed via > 7dcbeaa0df (send-pack: fix inconsistent porcelain output, 2020-04-17) > such that the stanza will also be printed when there was an error with > atomic pushes, which was previously inconsistent with non-atomic pushes. > The fixup commit has introduced a new issue though. During atomic pushes > it is expected that git-receive-pack(1) may exit early, and that causes > us to receive an error on the client-side. We (seemingly) still want to > print the "Done" stanza, but given that we only do so when the push has > been successful we started to ignore the error code by the child process > completely when doing an atomic push. I introduced the commit 7dcbeaa0df (send-pack: fix inconsistent porcelain output, 2020-04-17), because the porcelain output for git push over local file protocol and HTTP protocol are different and was hard to write the some test cases to work for both protocols. I acknowledge the patch was not perfect. I read the commit 77555854be (git-push: make git push --porcelain print "Done", 2010-02-26) and the code path of git push over two protocols a second time, and find something: The code snippet from 77555854be: > - ret = transport->push_refs(transport, remote_refs, flags); > + push_ret = transport->push_refs(transport, remote_refs, flags); > err = push_had_errors(remote_refs); > - > - ret |= err; > + ret = push_ret | err; > The return code "push_ret" of push_refs() from different transport vtable is not consist. For HTTP protocol, "push_ret" is zero if no connection error or no protocol errors. So we should consider “push_ret" as a protocol error rather than a reference update error. If we want to print "Done" in porcelain mode when there are no errors. (In dry run mode, ref-update-errors should not be considered as errors, but the opposite should be regarded as errors). Instead of using the following code, > + if (porcelain && !push_ret) > + puts("Done"); We can use like this ("pretend" is the flag for dry-run mode): + ret = pretend ? push_ret : (push_ret | err); + if (porcelain && !ret) + puts("Done"); I will send patches follow this patch series. -- Jiang Xin
diff --git a/send-pack.c b/send-pack.c index 6677c44e8acd19f16706ad2d78f72fee889daa55..815c1f206c09da8fb1ffe9be11beb9c5fa29d0c5 100644 --- a/send-pack.c +++ b/send-pack.c @@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args, reject_atomic_push(remote_refs, args->send_mirror); error("atomic push failed for ref %s. status: %d", ref->name, ref->status); - ret = args->porcelain ? 0 : -1; + ret = -1; goto out; } /* else fallthrough */ diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh index 0a3883043baf5c4c0fc43b52e8c5fc375f10a56a..b3774e56e0f45a91d574a15f0d6b5c50c4da70d4 100755 --- a/t/t5504-fetch-receive-strict.sh +++ b/t/t5504-fetch-receive-strict.sh @@ -105,6 +105,7 @@ test_expect_success 'push with receive.fsckobjects' ' cat >exp <<-EOF && To dst ! refs/heads/main:refs/heads/test [remote rejected] (unpacker error) + Done EOF test_must_fail git push --porcelain dst main:refs/heads/test >act && test_cmp exp act diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh index 479d103469527e6b923877cd480825b59e7094d4..1d8f088a004cf6743ade8e55536f04ac04000cf7 100755 --- a/t/t5543-atomic-push.sh +++ b/t/t5543-atomic-push.sh @@ -281,4 +281,34 @@ test_expect_success 'atomic push reports (reject by non-ff)' ' test_cmp expect actual ' +test_expect_success 'atomic push reports exit code failure' ' + write_script receive-pack-wrapper <<-\EOF && + git-receive-pack "$@" + exit 1 + EOF + test_must_fail git -C workbench push --atomic \ + --receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \ + up HEAD:refs/heads/no-conflict 2>err && + cat >expect <<-EOF && + To ../upstream + * [new branch] HEAD -> no-conflict + error: failed to push some refs to ${SQ}../upstream${SQ} + EOF + test_cmp expect err +' + +test_expect_success 'atomic push reports exit code failure with porcelain' ' + write_script receive-pack-wrapper <<-\EOF && + git-receive-pack "$@" + exit 1 + EOF + test_must_fail git -C workbench push --atomic --porcelain \ + --receive-pack="${SQ}$(pwd)${SQ}/receive-pack-wrapper" \ + up HEAD:refs/heads/no-conflict-porcelain 2>err && + cat >expect <<-EOF && + error: failed to push some refs to ${SQ}../upstream${SQ} + EOF + test_cmp expect err +' + test_done diff --git a/transport.c b/transport.c index 47fda6a7732f4b8cdcb6e750f36b896a988ffd0b..76b0645a25fe936190ca52d595b5f02eedc27d23 100644 --- a/transport.c +++ b/transport.c @@ -921,12 +921,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re close(data->fd[1]); close(data->fd[0]); - /* - * Atomic push may abort the connection early and close the pipe, - * which may cause an error for `finish_connect()`. Ignore this error - * for atomic git-push. - */ - if (ret || args.atomic) + if (ret) finish_connect(data->conn); else ret = finish_connect(data->conn); @@ -1503,7 +1498,7 @@ int transport_push(struct repository *r, transport_update_tracking_ref(transport->remote, ref, verbose); } - if (porcelain && !push_ret) + if (porcelain) puts("Done"); else if (!quiet && !ret && !transport_refs_pushed(remote_refs)) /* stable plumbing output; do not modify or localize */
When executing git-push(1) with the "--porcelain" flag, then we will print updated references in a machine-readable format that looks like this: To destination = refs/heads/noop:refs/heads/noop [up to date] ! refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed) ! refs/heads/noff:refs/heads/(off (non-fast-forward) Done The final "Done" stanza was introduced via 77555854be (git-push: make git push --porcelain print "Done", 2010-02-26), where it was printed "unless any errors have occurred". This behaviour was later changed via 7dcbeaa0df (send-pack: fix inconsistent porcelain output, 2020-04-17) such that the stanza will also be printed when there was an error with atomic pushes, which was previously inconsistent with non-atomic pushes. The fixup commit has introduced a new issue though. During atomic pushes it is expected that git-receive-pack(1) may exit early, and that causes us to receive an error on the client-side. We (seemingly) still want to print the "Done" stanza, but given that we only do so when the push has been successful we started to ignore the error code by the child process completely when doing an atomic push. We'd typically notice this case because the refs would have their error message set. But there is an edge case when pushing refs succeeds, but git-receive-pack(1) exits with a non-zero exit code at a later point in time due to another error. An atomic git-push(1) would ignore that error code, and consequently it would return successfully and not print any error message at all. Now it is somewhat unclear what the correct fix is in this case, mostly because the exact format of the porcelain output of git-push(1) is not specified to its full extent. What is clear though is that ignoring the error code is definitely not the correct thing to do. Adapt the code such that we honor the error code and unconditionally print the "Done" stanza even when pushing refs has failed. This ensures that git-push(1) notices the error condition and exits with an error, but slightly changes the output format. This requires a change to t5504, where we previously didn't print "Done" at the end of the push. As said, it is hard to say what the correct behaviour is in this case. But two test cases further up we have another test that fails in a similar way, and that test expects a final "Done" stanza. So if nothing else this at least seems to make the behaviour more consistent with other error cases. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- send-pack.c | 2 +- t/t5504-fetch-receive-strict.sh | 1 + t/t5543-atomic-push.sh | 30 ++++++++++++++++++++++++++++++ transport.c | 9 ++------- 4 files changed, 34 insertions(+), 8 deletions(-)