diff mbox series

[v4,1/5] send-pack: fix inconsistent porcelain output

Message ID 20200417094536.9463-2-worldhello.net@gmail.com (mailing list archive)
State New, archived
Headers show
Series fix git-push porcelain output and atomic report issue | expand

Commit Message

Jiang Xin April 17, 2020, 9:45 a.m. UTC 
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

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>
---
 send-pack.c                     |   7 +-
 t/t5504-fetch-receive-strict.sh |   1 +
 t/t5516-fetch-push.sh           |   1 +
 t/t5548-push-porcelain.sh       | 280 ++++++++++++++++++++++++++++++++
 transport.c                     |  10 +-
 5 files changed, 294 insertions(+), 5 deletions(-)
 create mode 100755 t/t5548-push-porcelain.sh

Comments

Junio C Hamano April 17, 2020, 7:51 p.m. UTC | #1 
Jiang Xin <worldhello.net@gmail.com> writes:

> diff --git a/transport.c b/transport.c
> index 1fdc7dac1a..13d638d5fe 100644
> --- 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);

Which means that the return value from this function under the
atomic mode is what we got from an earlier call to send_pack().

Which may have failed, in which case we do want to relay the error
to our caller, or may have succeeded, which is also fine to relay to
our caller.

> 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.

Under porcelain mode, if we already saw an error before we get to
the "if porcelain, do not bother checking the remote status", we
do relay the error to our caller.  That happens in the part you
omitted from your quote:

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

	if (ret < 0)
		return ret;

>
>         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 it is not like we are disabling all errors under the porcelain
mode.

> diff --git a/send-pack.c b/send-pack.c
> index 0407841ae8..1835cd5582 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -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;
>  			}

And this is in line with the way how rejected pushes are handled, I guess.

Sounds a bit convoluted but correct ;-)
diff mbox series

Patch

diff --git a/send-pack.c b/send-pack.c
index 0407841ae8..1835cd5582 100644
--- 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 645b4c78d3..a32efe2b6c 100755
--- 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 9ff041a093..9c6218f568 100755
--- 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
index 0000000000..96ba449a38
--- /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 1fdc7dac1a..13d638d5fe 100644
--- 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;