threads / patch / 33769

patch, 2 partstransport-helper: fixes

Subject: [PATCH 0/2] transport-helper: fixes

## tl;dr

7 messages between May 10, 2013 and May 10, 2013. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Felipe Contreras· May 10, 2013, 12:08 UTC · lore
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
Felipe Contreras· May 10, 2013, 12:08 UTC · re: Felipe Contreras · lore

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t5801-remote-helpers.sh +1 −1
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
Felipe Contreras· May 10, 2013, 12:08 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to 3 files +20 −2

git-remote-testgit.sh, t/t5801-remote-helpers.sh, transport-helper.c

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
Junio C Hamano· May 10, 2013, 14:35 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 8 quoted lines
> 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.

Junio C Hamano· May 10, 2013, 20:28 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 8 quoted lines
> +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 ... &&
Show 21 quoted lines
> +	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,
Felipe Contreras· May 10, 2013, 20:36 UTC · re: Junio C Hamano · lore

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

On Fri, May 10, 2013 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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
Junio C Hamano· May 10, 2013, 20:55 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 16 quoted lines
> 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.

← back to recent threads