# [PATCH 0/2] transport-helper: fixes 7 messages from 2013-05-10 to 2013-05-10. Participants: Felipe Contreras, Junio C Hamano. Thread: https://gitlist.dev/t/33769 ## Felipe Contreras, 2013-05-10 12:08 Subject: [PATCH 0/2] transport-helper: fixes Message-ID: <1368187710-4434-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1368187710-4434-1-git-send-email-felipe.contreras%40gmail.com ``` 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, 2013-05-10 12:08 Subject: [PATCH 1/2] test: remote-helper: add missing and Message-ID: <1368187710-4434-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1368187710-4434-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1368187710-4434-1-git-send-email-felipe.contreras@gmail.com> ``` Signed-off-by: Felipe Contreras --- 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 ``` ## Felipe Contreras, 2013-05-10 12:08 Subject: [PATCH 2/2] transport-helper: fix remote helper namespace regression Message-ID: <1368187710-4434-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1368187710-4434-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1368187710-4434-1-git-send-email-felipe.contreras@gmail.com> ``` 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 --- 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 ``` ## Junio C Hamano, 2013-05-10 14:35 Subject: Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression Message-ID: <7vr4he52ef.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr4he52ef.fsf%40alter.siamese.dyndns.org In-Reply-To: <1368187710-4434-3-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras 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 > --- > > 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, 2013-05-10 20:28 Subject: Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression Message-ID: <7vsj1uzijo.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsj1uzijo.fsf%40alter.siamese.dyndns.org In-Reply-To: <1368187710-4434-3-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras 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, ``` ## Felipe Contreras, 2013-05-10 20:36 Subject: Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression Message-ID: URL: https://gitlist.dev/e/CAMP44s2QHd7b2t654hnMw_xca0OJKjUjEgP2yU4PUe-w4z-zBg%40mail.gmail.com In-Reply-To: <7vsj1uzijo.fsf@alter.siamese.dyndns.org> ``` On Fri, May 10, 2013 at 3:28 PM, Junio C Hamano wrote: > Felipe Contreras 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, 2013-05-10 20:55 Subject: Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression Message-ID: <7vli7mzha6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vli7mzha6.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Felipe Contreras writes: > On Fri, May 10, 2013 at 3:28 PM, Junio C Hamano wrote: >> Felipe Contreras 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. ```