{"thread":{"id":"33769","subject":"[PATCH 0/2] transport-helper: fixes","startedAt":"2013-05-10T12:08:28Z","lastAt":"2013-05-10T20:55:29Z","messageCount":7,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"216867","messageId":"1368187710-4434-1-git-send-email-felipe.contreras@gmail.com","threadId":"33769","inReplyTo":null,"subject":"[PATCH 0/2] transport-helper: fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-10T12:08:28Z","receivedAt":"2013-05-10T12:08:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nI found potentially serious issue with one of my patches to transport-helper.\n\nFelipe Contreras (2):\n test: remote-helper: add missing and\n transport-helper: fix remote helper namespace regression\n\n git-remote-testgit.sh | 7 ++++++-\n t/t5801-remote-helpers.sh | 15 ++++++++++++++-\n transport-helper.c | 2 +-\n 3 files changed, 21 insertions(+), 3 deletions(-)\n\n-- \n1.8.3.rc1.579.g184e698\n"},{"id":"216868","messageId":"1368187710-4434-2-git-send-email-felipe.contreras@gmail.com","threadId":"33769","inReplyTo":"1368187710-4434-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/2] test: remote-helper: add missing and","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-10T12:08:29Z","receivedAt":"2013-05-10T12:08:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n t/t5801-remote-helpers.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 61479c3..352115c 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -175,7 +175,7 @@ test_expect_success 'push update refs' '\n \tgit checkout -b update master &&\n \techo update >>file &&\n \tgit commit -a -m update &&\n-\tgit push origin update\n+\tgit push origin update &&\n \tgit rev-parse --verify remotes/origin/update >expect &&\n \tgit rev-parse --verify testgit/origin/heads/update >actual &&\n \ttest_cmp expect actual\n-- \n1.8.3.rc1.579.g184e698\n"},{"id":"216869","messageId":"1368187710-4434-3-git-send-email-felipe.contreras@gmail.com","threadId":"33769","inReplyTo":"1368187710-4434-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/2] transport-helper: fix remote helper namespace regression","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-10T12:08:30Z","receivedAt":"2013-05-10T12:08:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Commit 664059f (transport-helper: update remote helper namespace)\nupdates the namespace when the push succeeds or not; we should do it\nonly when it succeeded.\n\nSigned-off-by: Felipe Contreras \n---\n\nThe regression is in 'next' so far.\n\n git-remote-testgit.sh | 7 ++++++-\n t/t5801-remote-helpers.sh | 13 +++++++++++++\n transport-helper.c | 2 +-\n 3 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/git-remote-testgit.sh b/git-remote-testgit.sh\nindex e83315f..2109070 100755\n--- a/git-remote-testgit.sh\n+++ b/git-remote-testgit.sh\n@@ -104,7 +104,12 @@ do\n \t\t\t*\" $ref $a \"*)\n \t\t\t\tcontinue ;;\t# unchanged\n \t\t\tesac\n-\t\t\techo \"ok $ref\"\n+\t\t\tif test -z \"$GIT_REMOTE_TESTGIT_PUSH_ERROR\"\n+\t\t\tthen\n+\t\t\t\techo \"ok $ref\"\n+\t\t\telse\n+\t\t\t\techo \"error $ref $GIT_REMOTE_TESTGIT_PUSH_ERROR\"\n+\t\t\tfi\n \t\tdone\n \n \t\techo\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 352115c..fa72181 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -182,6 +182,19 @@ test_expect_success 'push update refs' '\n \t)\n '\n \n+test_expect_success 'push update refs failure' '\n+\t(cd local &&\n+\tgit checkout update &&\n+\techo \"update fail\" >>file &&\n+\tgit commit -a -m \"update fail\" &&\n+\tgit rev-parse --verify testgit/origin/heads/update >expect &&\n+\tGIT_REMOTE_TESTGIT_PUSH_ERROR=\"non-fast forward\" \\\n+\ttest_expect_code 1 git push origin update &&\n+\tgit rev-parse --verify testgit/origin/heads/update >actual &&\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'proper failure checks for fetching' '\n \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n \texport GIT_REMOTE_TESTGIT_FAILURE &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex f11d78a..2f5ac3f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -705,7 +705,7 @@ static int push_update_ref_status(struct strbuf *buf,\n \n \t(*ref)->status = status;\n \t(*ref)->remote_status = msg;\n-\treturn 0;\n+\treturn !(status == REF_STATUS_OK);\n }\n \n static void push_update_refs_status(struct helper_data *data,\n-- \n1.8.3.rc1.579.g184e698\n"},{"id":"216887","messageId":"7vr4he52ef.fsf@alter.siamese.dyndns.org","threadId":"33769","inReplyTo":"1368187710-4434-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T14:35:04Z","receivedAt":"2013-05-10T14:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Commit 664059f (transport-helper: update remote helper namespace)\n> updates the namespace when the push succeeds or not; we should do it\n> only when it succeeded.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n>\n> The regression is in 'next' so far.\n\nThanks.\n\nAs you may well know, anything in 'next' will stay there until 1.8.3\nand then 'next' will rewound and rebuilt after that. If you prefer,\nwe can do without \"oops, the previous one was wrong and this is a\nfix\" follow-up patch like this and instead replace the faulty change\nin the series when that happens.\n"},{"id":"216937","messageId":"7vsj1uzijo.fsf@alter.siamese.dyndns.org","threadId":"33769","inReplyTo":"1368187710-4434-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T20:28:11Z","receivedAt":"2013-05-10T20:28:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> +test_expect_success 'push update refs failure' '\n> +\t(cd local &&\n> +\tgit checkout update &&\n> +\techo \"update fail\" >>file &&\n> +\tgit commit -a -m \"update fail\" &&\n> +\tgit rev-parse --verify testgit/origin/heads/update >expect &&\n> +\tGIT_REMOTE_TESTGIT_PUSH_ERROR=\"non-fast forward\" \\\n> +\ttest_expect_code 1 git push origin update &&\n\nThis is not portable; as the remainder of this subshell does not\nmind having the environment, a simple fix may be something like:\n\n\tGIT_REMOTE_TESTGIT_PUSH_ERROR=\"...\" &&\n exoprt GIT_REMOTE_TESTGIT_PUSH_ERROR &&\n test_expect_code 1 git push ... &&\n\n> +\tgit rev-parse --verify testgit/origin/heads/update >actual &&\n> +\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_expect_success 'proper failure checks for fetching' '\n> \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> \texport GIT_REMOTE_TESTGIT_FAILURE &&\n> diff --git a/transport-helper.c b/transport-helper.c\n> index f11d78a..2f5ac3f 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -705,7 +705,7 @@ static int push_update_ref_status(struct strbuf *buf,\n> \n> \t(*ref)->status = status;\n> \t(*ref)->remote_status = msg;\n> -\treturn 0;\n> +\treturn !(status == REF_STATUS_OK);\n> }\n> \n> static void push_update_refs_status(struct helper_data *data,\n"},{"id":"216938","messageId":"CAMP44s2QHd7b2t654hnMw_xca0OJKjUjEgP2yU4PUe-w4z-zBg@mail.gmail.com","threadId":"33769","inReplyTo":"7vsj1uzijo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-10T20:36:16Z","receivedAt":"2013-05-10T20:36:16Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 10, 2013 at 3:28 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> +test_expect_success 'push update refs failure' '\n>> + (cd local &&\n>> + git checkout update &&\n>> + echo \"update fail\" >>file &&\n>> + git commit -a -m \"update fail\" &&\n>> + git rev-parse --verify testgit/origin/heads/update >expect &&\n>> + GIT_REMOTE_TESTGIT_PUSH_ERROR=\"non-fast forward\" \\\n>> + test_expect_code 1 git push origin update &&\n>\n> This is not portable\n\nWhy not? Other parts of this script run commands with environment\nvariables like this.\n\n-- \nFelipe Contreras\n"},{"id":"216939","messageId":"7vli7mzha6.fsf@alter.siamese.dyndns.org","threadId":"33769","inReplyTo":"CAMP44s2QHd7b2t654hnMw_xca0OJKjUjEgP2yU4PUe-w4z-zBg@mail.gmail.com","subject":"Re: [PATCH 2/2] transport-helper: fix remote helper namespace regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T20:55:29Z","receivedAt":"2013-05-10T20:55:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Fri, May 10, 2013 at 3:28 PM, Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>>\n>>> +test_expect_success 'push update refs failure' '\n>>> + (cd local &&\n>>> + git checkout update &&\n>>> + echo \"update fail\" >>file &&\n>>> + git commit -a -m \"update fail\" &&\n>>> + git rev-parse --verify testgit/origin/heads/update >expect &&\n>>> + GIT_REMOTE_TESTGIT_PUSH_ERROR=\"non-fast forward\" \\\n>>> + test_expect_code 1 git push origin update &&\n>>\n>> This is not portable\n>\n> Why not? Other parts of this script run commands with environment\n> variables like this.\n\nThere is a difference between a proper command and a shell function\nwith respect to the single-shot environment assignment.\n\n\tVAR=VAL command args\n\nis fine, but\n\n\tVAR=VAL shell_function args\n\nis not.\n\nPeople often make this mistake and we had to fix it number of times\nin t/ directory.\n"}]}