[PATCH 0/2] transport-helper: fixes

[PATCH 0/2] transport-helper: fixes

[Felipe Contreras]

Hi,

I found potentially serious issue with one of my patches to transport-helper.

Felipe Contreras (2):
  test: remote-helper: add missing and
  transport-helper: fix remote helper namespace regression

 git-remote-testgit.sh     |  7 ++++++-
 t/t5801-remote-helpers.sh | 15 ++++++++++++++-
 transport-helper.c        |  2 +-
 3 files changed, 21 insertions(+), 3 deletions(-)

-- 
1.8.3.rc1.579.g184e698

[PATCH 1/2] test: remote-helper: add missing and

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 61479c3..352115c 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -175,7 +175,7 @@ test_expect_success 'push update refs' '
 	git checkout -b update master &&
 	echo update >>file &&
 	git commit -a -m update &&
-	git push origin update
+	git push origin update &&
 	git rev-parse --verify remotes/origin/update >expect &&
 	git rev-parse --verify testgit/origin/heads/update >actual &&
 	test_cmp expect actual
-- 
1.8.3.rc1.579.g184e698

[PATCH 2/2] transport-helper: fix remote helper namespace regression

[Felipe Contreras]

Commit 664059f (transport-helper: update remote helper namespace)
updates the namespace when the push succeeds or not; we should do it
only when it succeeded.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

The regression is in 'next' so far.

 git-remote-testgit.sh     |  7 ++++++-
 t/t5801-remote-helpers.sh | 13 +++++++++++++
 transport-helper.c        |  2 +-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
index e83315f..2109070 100755
--- a/git-remote-testgit.sh
+++ b/git-remote-testgit.sh
@@ -104,7 +104,12 @@ do
 			*" $ref $a "*)
 				continue ;;	# unchanged
 			esac
-			echo "ok $ref"
+			if test -z "$GIT_REMOTE_TESTGIT_PUSH_ERROR"
+			then
+				echo "ok $ref"
+			else
+				echo "error $ref $GIT_REMOTE_TESTGIT_PUSH_ERROR"
+			fi
 		done
 
 		echo
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 352115c..fa72181 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -182,6 +182,19 @@ test_expect_success 'push update refs' '
 	)
 '
 
+test_expect_success 'push update refs failure' '
+	(cd local &&
+	git checkout update &&
+	echo "update fail" >>file &&
+	git commit -a -m "update fail" &&
+	git rev-parse --verify testgit/origin/heads/update >expect &&
+	GIT_REMOTE_TESTGIT_PUSH_ERROR="non-fast forward" \
+	test_expect_code 1 git push origin update &&
+	git rev-parse --verify testgit/origin/heads/update >actual &&
+	test_cmp expect actual
+	)
+'
+
 test_expect_success 'proper failure checks for fetching' '
 	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
 	export GIT_REMOTE_TESTGIT_FAILURE &&
diff --git a/transport-helper.c b/transport-helper.c
index f11d78a..2f5ac3f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -705,7 +705,7 @@ static int push_update_ref_status(struct strbuf *buf,
 
 	(*ref)->status = status;
 	(*ref)->remote_status = msg;
-	return 0;
+	return !(status == REF_STATUS_OK);
 }
 
 static void push_update_refs_status(struct helper_data *data,
-- 
1.8.3.rc1.579.g184e698

Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Commit 664059f (transport-helper: update remote helper namespace)
> updates the namespace when the push succeeds or not; we should do it
> only when it succeeded.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>
> The regression is in 'next' so far.

Thanks.

As you may well know, anything in 'next' will stay there until 1.8.3
and then 'next' will rewound and rebuilt after that.  If you prefer,
we can do without "oops, the previous one was wrong and this is a
fix" follow-up patch like this and instead replace the faulty change
in the series when that happens.

Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> +test_expect_success 'push update refs failure' '
> +	(cd local &&
> +	git checkout update &&
> +	echo "update fail" >>file &&
> +	git commit -a -m "update fail" &&
> +	git rev-parse --verify testgit/origin/heads/update >expect &&
> +	GIT_REMOTE_TESTGIT_PUSH_ERROR="non-fast forward" \
> +	test_expect_code 1 git push origin update &&

This is not portable; as the remainder of this subshell does not
mind having the environment, a simple fix may be something like:

	GIT_REMOTE_TESTGIT_PUSH_ERROR="..." &&
        exoprt GIT_REMOTE_TESTGIT_PUSH_ERROR &&
        test_expect_code 1 git push ... &&

> +	git rev-parse --verify testgit/origin/heads/update >actual &&
> +	test_cmp expect actual
> +	)
> +'
> +
>  test_expect_success 'proper failure checks for fetching' '
>  	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
>  	export GIT_REMOTE_TESTGIT_FAILURE &&
> diff --git a/transport-helper.c b/transport-helper.c
> index f11d78a..2f5ac3f 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -705,7 +705,7 @@ static int push_update_ref_status(struct strbuf *buf,
>  
>  	(*ref)->status = status;
>  	(*ref)->remote_status = msg;
> -	return 0;
> +	return !(status == REF_STATUS_OK);
>  }
>  
>  static void push_update_refs_status(struct helper_data *data,

Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression

[Felipe Contreras]

On Fri, May 10, 2013 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> +test_expect_success 'push update refs failure' '
>> +     (cd local &&
>> +     git checkout update &&
>> +     echo "update fail" >>file &&
>> +     git commit -a -m "update fail" &&
>> +     git rev-parse --verify testgit/origin/heads/update >expect &&
>> +     GIT_REMOTE_TESTGIT_PUSH_ERROR="non-fast forward" \
>> +     test_expect_code 1 git push origin update &&
>
> This is not portable

Why not? Other parts of this script run commands with environment
variables like this.

-- 
Felipe Contreras

Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Fri, May 10, 2013 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> +test_expect_success 'push update refs failure' '
>>> +     (cd local &&
>>> +     git checkout update &&
>>> +     echo "update fail" >>file &&
>>> +     git commit -a -m "update fail" &&
>>> +     git rev-parse --verify testgit/origin/heads/update >expect &&
>>> +     GIT_REMOTE_TESTGIT_PUSH_ERROR="non-fast forward" \
>>> +     test_expect_code 1 git push origin update &&
>>
>> This is not portable
>
> Why not? Other parts of this script run commands with environment
> variables like this.

There is a difference between a proper command and a shell function
with respect to the single-shot environment assignment.

	VAR=VAL command args

is fine, but

	VAR=VAL shell_function args

is not.

People often make this mistake and we had to fix it number of times
in t/ directory.