transport: don't ignore git-receive-pack(1) exit code on atomic push

When executing git-push(1) with the "--porcelain" flag, then we will
print updated references in a machine-readable format that looks like
this:

    To destination
    =	refs/heads/noop:refs/heads/noop	[up to date]
    !	refs/heads/rejected:refs/heads/rejected	[rejected] (atomic push failed)
    !	refs/heads/noff:refs/heads/(off (non-fast-forward)
    Done

The final "Done" stanza was introduced via 7755585 (git-push: make
git push --porcelain print "Done", 2010-02-26), where it was printed
"unless any errors have occurred". This behaviour was later changed via
7dcbeaa (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 <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: pks-t

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

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

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

transport.c

@@ -918,12 +918,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);
@@ -1488,7 +1483,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 */