[PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Patrick Steinhardt <ps@pks.im>]

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(-)

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

-- 
2.47.0.251.gb31fb630c0.dirty