send-pack: fix inconsistent porcelain output

The porcelain output of a failed `git-push` command is inconsistent for
different protocols.  For example, the following `git-push` command
may fail due to the failure of the `pre-receive` hook.

    git push --porcelain origin HEAD:refs/heads/master

For SSH protocol, the porcelain output does not end with a "Done"
message:

	To <URL/of/upstream.git>
	!  HEAD:refs/heads/master  [remote rejected] (pre-receive hook declined)

While for HTTP protocol, the porcelain output does end with a "Done"
message:

	To <URL/of/upstream.git>
	!  HEAD:refs/heads/master  [remote rejected] (pre-receive hook declined)
	Done

The following code at the end of function `send_pack()` indicates that
`send_pack()` should not return an error if some references are rejected
in porcelain mode.

    int send_pack(...)
        ... ...

        if (args->porcelain)
            return 0;

        for (ref = remote_refs; ref; ref = ref->next) {
            switch (ref->status) {
            case REF_STATUS_NONE:
            case REF_STATUS_UPTODATE:
            case REF_STATUS_OK:
                break;
            default:
                return -1;
            }
        }
        return 0;
    }

So if atomic push failed, must check the porcelain mode before return
an error.  And `receive_status()` should not return an error for a
failed updated reference, because `send_pack()` will check them instead.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/send-pack.c b/send-pack.c
index 0407841ae87af99254f254b65bb53815790fec9a..1835cd5582342db7a8c82161f9a9787838c7d948 100644 (file)
--- a/send-pack.c
+++ b/send-pack.c
@@ -190,10 +190,8 @@ static int receive_status(struct packet_reader *reader, struct ref *refs)
 
                if (reader->line[0] == 'o' && reader->line[1] == 'k')
                        hint->status = REF_STATUS_OK;
-               else {
+               else
                        hint->status = REF_STATUS_REMOTE_REJECT;
-                       ret = -1;
-               }
                hint->remote_status = xstrdup_or_null(msg);
                /* start our next search from the next ref */
                hint = hint->next;
@@ -489,7 +487,8 @@ int send_pack(struct send_pack_args *args,
                        if (use_atomic) {
                                strbuf_release(&req_buf);
                                strbuf_release(&cap_buf);
-                               return atomic_push_failure(args, remote_refs, ref);
+                               atomic_push_failure(args, remote_refs, ref);
+                               return args->porcelain ? 0 : -1;
                        }
                        /* else fallthrough */
                default:

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 645b4c78d356971cdf096456d7e90463f58cd0e3..a32efe2b6cdd84690bdb5193609105204ff1dd39 100755 (executable)
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -65,6 +65,7 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 cat >exp <<EOF
 To dst
 !      refs/heads/master:refs/heads/test       [remote rejected] (missing necessary objects)
+Done
 EOF
 
 test_expect_success 'push without strict' '

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 9ff041a093e71aac932f5e563d4922930bc62ff2..9c6218f568ea3bc4c61a1377fc4b2be150b4be1d 100755 (executable)
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1066,6 +1066,7 @@ test_expect_success 'push --porcelain rejected' '
 
        echo >.git/foo  "To testrepo"  &&
        echo >>.git/foo "!      refs/heads/master:refs/heads/master     [remote rejected] (branch is currently checked out)" &&
+       echo >>.git/foo "Done" &&
 
        test_must_fail git push >.git/bar --porcelain  testrepo refs/heads/master:refs/heads/master &&
        test_cmp .git/foo .git/bar

diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
new file mode 100755 (executable)
index 0000000..96ba449
--- /dev/null
+++ b/t/t5548-push-porcelain.sh
@@ -0,0 +1,280 @@
+#!/bin/sh
+#
+# Copyright (c) 2020 Jiang Xin
+#
+test_description='Test git push porcelain output'
+
+. ./test-lib.sh
+
+# Create commits in <repo> and assign each commit's oid to shell variables
+# given in the arguments (A, B, and C). E.g.:
+#
+#     create_commits_in <repo> A B C
+#
+# NOTE: Never calling this function from a subshell since variable
+# assignments will disappear when subshell exits.
+create_commits_in () {
+       repo="$1" &&
+       if ! parent=$(git -C "$repo" rev-parse HEAD^{} --)
+       then
+               parent=
+       fi &&
+       T=$(git -C "$repo" write-tree) &&
+       shift &&
+       while test $# -gt 0
+       do
+               name=$1 &&
+               test_tick &&
+               if test -z "$parent"
+               then
+                       oid=$(echo $name | git -C "$repo" commit-tree $T)
+               else
+                       oid=$(echo $name | git -C "$repo" commit-tree -p $parent $T)
+               fi &&
+               eval $name=$oid &&
+               parent=$oid &&
+               shift ||
+               return 1
+       done &&
+       git -C "$repo" update-ref refs/heads/master $oid
+}
+
+# Format the output of git-push, git-show-ref and other commands to make a
+# user-friendly and stable text.  We can easily prepare the expect text
+# without having to worry about future changes of the commit ID and spaces
+# of the output.
+make_user_friendly_and_stable_output () {
+       sed \
+               -e "s/  *\$//" \
+               -e "s/   */ /g" \
+               -e "s/  /    /g" \
+               -e "s/$A/<COMMIT-A>/g" \
+               -e "s/$B/<COMMIT-B>/g" \
+               -e "s/$ZERO_OID/<ZERO-OID>/g" \
+               -e "s/$(echo $A | cut -c1-7)[0-9a-f]*/<OID-A>/g" \
+               -e "s/$(echo $B | cut -c1-7)[0-9a-f]*/<OID-B>/g" \
+               -e "s#To $URL_PREFIX/upstream.git#To <URL/of/upstream.git>#"
+}
+
+setup_upstream_and_workbench () {
+       # Upstream  after setup : master(B)  foo(A)  bar(A)  baz(A)
+       # Workbench after setup : master(A)
+       test_expect_success "setup upstream repository and workbench" '
+               rm -rf upstream.git workbench &&
+               git init --bare upstream.git &&
+               git init workbench &&
+               create_commits_in workbench A B &&
+               (
+                       cd workbench &&
+                       # Try to make a stable fixed width for abbreviated commit ID,
+                       # this fixed-width oid will be replaced with "<OID>".
+                       git config core.abbrev 7 &&
+                       git remote add origin ../upstream.git &&
+                       git update-ref refs/heads/master $A &&
+                       git push origin \
+                               $B:refs/heads/master \
+                               $A:refs/heads/foo \
+                               $A:refs/heads/bar \
+                               $A:refs/heads/baz
+               ) &&
+               git -C "workbench" config advice.pushUpdateRejected false &&
+               upstream=upstream.git
+       '
+}
+
+run_git_push_porcelain_output_test() {
+       case $1 in
+       http)
+               PROTOCOL="HTTP protocol"
+               URL_PREFIX="http://.*"
+               ;;
+       file)
+               PROTOCOL="builtin protocol"
+               URL_PREFIX="\.\."
+               ;;
+       esac
+
+       # Refs of upstream : master(B)  foo(A)  bar(A)  baz(A)
+       # Refs of workbench: master(A)                  baz(A)  next(A)
+       # git-push         : master(A)  NULL    (B)     baz(A)  next(A)
+       test_expect_success "porcelain output of successful git-push ($PROTOCOL)" '
+               (
+                       cd workbench &&
+                       git update-ref refs/heads/master $A &&
+                       git update-ref refs/heads/baz $A &&
+                       git update-ref refs/heads/next $A &&
+                       git push --porcelain --force origin \
+                               master \
+                               :refs/heads/foo \
+                               $B:bar \
+                               baz \
+                               next
+               ) >out &&
+               make_user_friendly_and_stable_output <out >actual &&
+               cat >expect <<-EOF &&
+               To <URL/of/upstream.git>
+               =    refs/heads/baz:refs/heads/baz    [up to date]
+                    <COMMIT-B>:refs/heads/bar    <OID-A>..<OID-B>
+               -    :refs/heads/foo    [deleted]
+               +    refs/heads/master:refs/heads/master    <OID-B>...<OID-A> (forced update)
+               *    refs/heads/next:refs/heads/next    [new branch]
+               Done
+               EOF
+               test_cmp expect actual &&
+
+               git -C "$upstream" show-ref >out &&
+               make_user_friendly_and_stable_output <out >actual &&
+               cat >expect <<-EOF &&
+               <COMMIT-B> refs/heads/bar
+               <COMMIT-A> refs/heads/baz
+               <COMMIT-A> refs/heads/master
+               <COMMIT-A> refs/heads/next
+               EOF
+               test_cmp expect actual
+       '
+
+       # Refs of upstream : master(A)  bar(B)  baz(A)  next(A)
+       # Refs of workbench: master(B)  bar(A)  baz(A)  next(A)
+       # git-push         : master(B)  bar(A)  NULL    next(A)
+       test_expect_success "atomic push failed ($PROTOCOL)" '
+               (
+                       cd workbench &&
+                       git update-ref refs/heads/master $B &&
+                       git update-ref refs/heads/bar $A &&
+                       test_must_fail git push --atomic --porcelain origin \
+                               master \
+                               bar \
+                               :baz \
+                               next
+               ) >out &&
+               make_user_friendly_and_stable_output <out >actual &&
+               cat >expect <<-EOF &&
+               To <URL/of/upstream.git>
+               !    refs/heads/bar:refs/heads/bar    [rejected] (non-fast-forward)
+               !    (delete):refs/heads/baz    [rejected] (atomic push failed)
+               !    refs/heads/master:refs/heads/master    [rejected] (atomic push failed)
+               !    refs/heads/next:refs/heads/next    [rejected] (atomic push failed)
+               Done
+               EOF
+               test_cmp expect actual &&
+
+               git -C "$upstream" show-ref >out &&
+               make_user_friendly_and_stable_output <out >actual &&
+               cat >expect <<-EOF &&
+               <COMMIT-B> refs/heads/bar
+               <COMMIT-A> refs/heads/baz
+               <COMMIT-A> refs/heads/master
+               <COMMIT-A> refs/heads/next
+               EOF
+               test_cmp expect actual
+       '
+
+       test_expect_success "prepare pre-receive hook ($PROTOCOL)" '
+               write_script "$upstream/hooks/pre-receive" <<-EOF
+               exit 1
+               EOF
+       '
+
+       # Refs of upstream : master(A)  bar(B)  baz(A)  next(A)
+       # Refs of workbench: master(B)  bar(A)  baz(A)  next(A)
+       # git-push         : master(B)  bar(A)  NULL    next(A)
+       test_expect_success "pre-receive hook declined ($PROTOCOL)" '
+               (
+                       cd workbench &&
+                       git update-ref refs/heads/master $B &&
+                       git update-ref refs/heads/bar $A &&
+                       test_must_fail git push --porcelain --force origin \
+                               master \
+                               bar \
+                               :baz \
+                               next
+               ) >out &&
+               make_user_friendly_and_stable_output <out >actual &&
+               cat >expect <<-EOF &&
+               To <URL/of/upstream.git>
+               =    refs/heads/next:refs/heads/next    [up to date]
+               !    refs/heads/bar:refs/heads/bar    [remote rejected] (pre-receive hook declined)
+               !    :refs/heads/baz    [remote rejected] (pre-receive hook declined)
+               !    refs/heads/master:refs/heads/master    [remote rejected] (pre-receive hook declined)
+               Done
+               EOF
+               test_cmp expect actual &&
+
+               git -C "$upstream" show-ref >out &&
+               make_user_friendly_and_stable_output <out >actual &&
+               cat >expect <<-EOF &&
+               <COMMIT-B> refs/heads/bar
+               <COMMIT-A> refs/heads/baz
+               <COMMIT-A> refs/heads/master
+               <COMMIT-A> refs/heads/next
+               EOF
+               test_cmp expect actual
+       '
+
+       test_expect_success "remove pre-receive hook ($PROTOCOL)" '
+               rm "$upstream/hooks/pre-receive"
+       '
+
+       # Refs of upstream : master(A)  bar(B)  baz(A)  next(A)
+       # Refs of workbench: master(B)  bar(A)  baz(A)  next(A)
+       # git-push         : master(B)  bar(A)  NULL    next(A)
+       test_expect_success "non-fastforward push ($PROTOCOL)" '
+               (
+                       cd workbench &&
+                       test_must_fail git push --porcelain origin \
+                               master \
+                               bar \
+                               :baz \
+                               next
+               ) >out &&
+               make_user_friendly_and_stable_output <out >actual &&
+               cat >expect <<-EOF &&
+               To <URL/of/upstream.git>
+               =    refs/heads/next:refs/heads/next    [up to date]
+               -    :refs/heads/baz    [deleted]
+                    refs/heads/master:refs/heads/master    <OID-A>..<OID-B>
+               !    refs/heads/bar:refs/heads/bar    [rejected] (non-fast-forward)
+               Done
+               EOF
+               test_cmp expect actual &&
+
+               git -C "$upstream" show-ref >out &&
+               make_user_friendly_and_stable_output <out >actual &&
+               cat >expect <<-EOF &&
+               <COMMIT-B> refs/heads/bar
+               <COMMIT-B> refs/heads/master
+               <COMMIT-A> refs/heads/next
+               EOF
+               test_cmp expect actual
+       '
+}
+
+# Initialize the upstream repository and local workbench.
+setup_upstream_and_workbench
+
+# Run git-push porcelain test on builtin protocol
+run_git_push_porcelain_output_test file
+
+ROOT_PATH="$PWD"
+. "$TEST_DIRECTORY"/lib-gpg.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
+start_httpd
+
+# Re-initialize the upstream repository and local workbench.
+setup_upstream_and_workbench
+
+test_expect_success "setup for http" '
+       git -C upstream.git config http.receivepack true &&
+       upstream="$HTTPD_DOCUMENT_ROOT_PATH/upstream.git" &&
+       mv upstream.git "$upstream" &&
+
+       git -C workbench remote set-url origin $HTTPD_URL/smart/upstream.git
+'
+
+setup_askpass_helper
+
+# Run git-push porcelain test on HTTP protocol
+run_git_push_porcelain_output_test http
+
+test_done

diff --git a/transport.c b/transport.c
index 1fdc7dac1a6230bdf3492cbebd6642fc825dac5f..13d638d5febf1824d90838afb9516db71a33d141 100644 (file)
--- a/transport.c
+++ b/transport.c
@@ -715,7 +715,15 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 
        close(data->fd[1]);
        close(data->fd[0]);
-       ret |= finish_connect(data->conn);
+       /*
+        * 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);
        data->conn = NULL;
        data->got_remote_heads = 0;