send-pack: gracefully close the connection for atomic push

Patrick reported an issue that the exit code of git-receive-pack(1) is
ignored during atomic push with "--porcelain" flag, and added new test
cases in t5543.

This issue originated from commit 7dcbeaa0df (send-pack: fix
inconsistent porcelain output, 2020-04-17). At that time, I chose to
ignore the exit code of "finish_connect()" without investigating the
root cause of the abnormal termination of git-receive-pack. That was an
incorrect solution.

The root cause is that an atomic push operation terminates early without
sending a flush packet to git-receive-pack. As a result,
git-receive-pack continues waiting for commands without exiting. By
sending a flush packet at the appropriate location in "send_pack()", we
ensure that the git-receive-pack process closes properly, avoiding an
erroneous exit code for git-push. At the same time, revert the changes
to the "transport.c" file made in commit 7dcbeaa0df.

Reported-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/send-pack.c b/send-pack.c
index 4448c081cca5367b973a85019a721f38ae5a4467..856a65d5f5abcef3490a480932e9a31f0b6efbff 100644 (file)
--- a/send-pack.c
+++ b/send-pack.c
@@ -633,6 +633,7 @@ int send_pack(struct repository *r,
                                error("atomic push failed for ref %s. status: %d",
                                      ref->name, ref->status);
                                ret = ERROR_SEND_PACK_BAD_REF_STATUS;
+                               packet_flush(out);
                                goto out;
                        }
                        /* else fallthrough */

diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 32181b9afb71174cc8b5d7d8d48e50644df395df..3a700b06761db43768a5333142ca920bd850a903 100755 (executable)
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -280,7 +280,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
        test_cmp expect actual
 '
 
-test_expect_failure 'atomic push reports exit code failure' '
+test_expect_success 'atomic push reports exit code failure' '
        write_script receive-pack-wrapper <<-\EOF &&
        git-receive-pack "$@"
        exit 1
@@ -296,7 +296,7 @@ test_expect_failure 'atomic push reports exit code failure' '
        test_cmp expect err
 '
 
-test_expect_failure 'atomic push reports exit code failure with porcelain' '
+test_expect_success 'atomic push reports exit code failure with porcelain' '
        write_script receive-pack-wrapper <<-\EOF &&
        git-receive-pack "$@"
        exit 1

diff --git a/transport.c b/transport.c
index d064aff33e3ecce7cd562dbc2f087a33a47b0c4b..b0c6c339f4b47dbecf66dc349b39b6f8afecc031 100644 (file)
--- a/transport.c
+++ b/transport.c
@@ -948,15 +948,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)
-               finish_connect(data->conn);
-       else
-               ret = finish_connect(data->conn);
+       ret |= finish_connect(data->conn);
        data->conn = NULL;
        data->finished_handshake = 0;