* [PATCH] push options: fail properly in the stateless case @ 2017-02-08 1:09 Stefan Beller 2017-02-08 18:11 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Stefan Beller @ 2017-02-08 1:09 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, Stefan Beller When using non-builtin protocols relying on a transport helper (such as http), push options are not propagated to the helper. Fix this by propagating the push options to the transport helper and adding a test that push options using http fail properly. Signed-off-by: Stefan Beller <sbeller@google.com> --- Documentation/gitremote-helpers.txt | 3 +++ t/t5545-push-options.sh | 15 +++++++++++++++ transport-helper.c | 7 +++++++ 3 files changed, 25 insertions(+) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index 9e8681f9e1..6145d4d8df 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -462,6 +462,9 @@ set by Git if the remote helper has the 'option' capability. 'option pushcert {'true'|'false'}:: GPG sign pushes. +'option push-option <c-string>:: + Transmit this push option. + SEE ALSO -------- linkgit:git-remote[1] diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh index ea813b9383..9a57a7d8f2 100755 --- a/t/t5545-push-options.sh +++ b/t/t5545-push-options.sh @@ -3,6 +3,8 @@ test_description='pushing to a repository using push options' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-httpd.sh +start_httpd mk_repo_pair () { rm -rf workbench upstream && @@ -100,4 +102,17 @@ test_expect_success 'two push options work' ' test_cmp expect upstream/.git/hooks/post-receive.push_options ' +test_expect_success 'push option denied properly by http remote helper' '\ + mk_repo_pair && + git -C upstream config receive.advertisePushOptions false && + git -C upstream config http.receivepack true && + cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git && + git clone "$HTTPD_URL"/smart/upstream test_http_clone && + test_commit -C test_http_clone one && + test_must_fail git -C test_http_clone push --push-option=asdf origin master && + git -C test_http_clone push origin master +' + +stop_httpd + test_done diff --git a/transport-helper.c b/transport-helper.c index 91aed35ebb..1258d6aedd 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -826,6 +826,13 @@ static void set_common_push_options(struct transport *transport, if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, "if-asked") != 0) die("helper %s does not support --signed=if-asked", name); } + + if (flags & TRANSPORT_PUSH_OPTIONS) { + struct string_list_item *item; + for_each_string_list_item(item, transport->push_options) + if (set_helper_option(transport, "push-option", item->string) != 0) + die("helper %s does not support 'push-option'", name); + } } static int push_refs_with_push(struct transport *transport, -- 2.12.0.rc0.1.g018cb5e6f4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] push options: fail properly in the stateless case 2017-02-08 1:09 [PATCH] push options: fail properly in the stateless case Stefan Beller @ 2017-02-08 18:11 ` Junio C Hamano 2017-02-08 20:51 ` Stefan Beller 2017-02-08 22:04 ` [PATCHv2] push options: pass push options to the transport helper Stefan Beller 0 siblings, 2 replies; 6+ messages in thread From: Junio C Hamano @ 2017-02-08 18:11 UTC (permalink / raw) To: Stefan Beller; +Cc: git, jrnieder Stefan Beller <sbeller@google.com> writes: > When using non-builtin protocols relying on a transport helper > (such as http), push options are not propagated to the helper. > > Fix this by propagating the push options to the transport helper and The description up to this point is VERY readable and sensible. But that makes the title sound a bit strange. I read it as if it were saying "stateless case can never support push-options so fail if the caller attempts to use one", but that does not seem to be what is going on. > adding a test that push options using http fail properly. Sounds sensible. What end-user visible effect does this fix have? IOW, what feature do we use "push-option" for? Ahh, OK, so you need to describe that there are two issues in order to be understood by the readers: (1) the helper protocol does not propagate push-option (2) the http helper is not prepared to handle push-option You fix (1), and you take advantage of the fact (2) to ensure that (1) is fixed in the new test. With such an understanding, the title makes (sort of) sense and you wouldn't have to be asked "what end-user visible effect/benefit does this have?" > +'option push-option <c-string>:: > + Transmit this push option. > + There is no "c-string" in the current documentation used or defined. The closest thing I found is ... that field will be quoted in the manner of a C string ... in git-status page, but I do not think you send the value for an push-option after running quote_c_style(), so I am puzzled. I'd rather see 'option push-option <string>' as the bullet item, and in its description say how arbitrary values (if you allow them, that is) can be used, e.g. "Transmit <string> encoded in such and such way a the value of the push-option". ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] push options: fail properly in the stateless case 2017-02-08 18:11 ` Junio C Hamano @ 2017-02-08 20:51 ` Stefan Beller 2017-02-08 23:01 ` Junio C Hamano 2017-02-08 22:04 ` [PATCHv2] push options: pass push options to the transport helper Stefan Beller 1 sibling, 1 reply; 6+ messages in thread From: Stefan Beller @ 2017-02-08 20:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: git@vger.kernel.org, Jonathan Nieder On Wed, Feb 8, 2017 at 10:11 AM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> When using non-builtin protocols relying on a transport helper >> (such as http), push options are not propagated to the helper. >> >> Fix this by propagating the push options to the transport helper and > > The description up to this point is VERY readable and sensible. But > that makes the title sound a bit strange. Yes, the title was there first and then I massaged the commit message until it was readable. Originally I thought the issue is with stateless protocols, but that is wrong. The underlying issue is just the transport helper communication lacking. > I read it as if it were > saying "stateless case can never support push-options so fail if the > caller attempts to use one", but that does not seem to be what is > going on. Indeed the subject is wrong. > >> adding a test that push options using http fail properly. > > Sounds sensible. What end-user visible effect does this fix have? > IOW, what feature do we use "push-option" for? The Gerrit world started using push options for having a better git experience, not needing to navigate to the web UI, e.g.: # push for review and set me as a reviewer: git push --push-option reviewer=sbeller@google.com ssh://gerrit... # same with http, it looks like it worked, but the push option # never made it to the server git push --push-option reviewer=sbeller@google.com http://gerrit... # This patch will make the second command fail, reporting # the http helper not supporting push options. > > Ahh, OK, so you need to describe that there are two issues in order > to be understood by the readers: > > (1) the helper protocol does not propagate push-option > (2) the http helper is not prepared to handle push-option > > You fix (1), and you take advantage of the fact (2) to ensure that > (1) is fixed in the new test. > > With such an understanding, the title makes (sort of) sense and you > wouldn't have to be asked "what end-user visible effect/benefit does > this have?" Your analysis is correct. > >> +'option push-option <c-string>:: >> + Transmit this push option. >> + > > There is no "c-string" in the current documentation used or > defined. The closest thing I found is > > ... that field will be quoted in the manner of a C string ... > > in git-status page, but I do not think you send the value for an > push-option after running quote_c_style(), so I am puzzled. When implementing push options, we discussed that and according to Documentation/git-push: The given string must not contain a NUL or LF character. > > I'd rather see 'option push-option <string>' as the bullet item, and > in its description say how arbitrary values (if you allow them, that > is) can be used, e.g. "Transmit <string> encoded in such and such > way a the value of the push-option". okay. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] push options: fail properly in the stateless case 2017-02-08 20:51 ` Stefan Beller @ 2017-02-08 23:01 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2017-02-08 23:01 UTC (permalink / raw) To: Stefan Beller; +Cc: git@vger.kernel.org, Jonathan Nieder Stefan Beller <sbeller@google.com> writes: >>> +'option push-option <c-string>:: >>> + Transmit this push option. >>> + >> >> There is no "c-string" in the current documentation used or >> defined. The closest thing I found is >> >> ... that field will be quoted in the manner of a C string ... >> >> in git-status page, but I do not think you send the value for an >> push-option after running quote_c_style(), so I am puzzled. > > When implementing push options, we discussed that and according to > Documentation/git-push: > > The given string must not contain a NUL or LF character. OK, so "Transmit <string> as a push option" is sufficient, as the string is sent as-is. OK. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv2] push options: pass push options to the transport helper 2017-02-08 18:11 ` Junio C Hamano 2017-02-08 20:51 ` Stefan Beller @ 2017-02-08 22:04 ` Stefan Beller [not found] ` <CAFzf2XzLNOKbNbkCfzpVJtN-ROW8m8WVBsWM-z7Bxdnv55b45Q@mail.gmail.com> 1 sibling, 1 reply; 6+ messages in thread From: Stefan Beller @ 2017-02-08 22:04 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, Stefan Beller When using non-builtin protocols relying on a transport helper (such as http), push options are not propagated to the helper. The user could ask for push options and a push would seemingly succeed, but the push options would never be transported to the server, misleading the users expectation. Fix this by propagating the push options to the transport helper. This is only addressing the first issue of (1) the helper protocol does not propagate push-option (2) the http helper is not prepared to handle push-option Once we fix (2), the http transport helper can make use of push options as well, but that happens as a follow up. (1) is a bug fix, whereas (2) is a feature, which is why we only do (1) here. Signed-off-by: Stefan Beller <sbeller@google.com> --- Documentation/gitremote-helpers.txt | 4 ++++ t/t5545-push-options.sh | 15 +++++++++++++++ transport-helper.c | 7 +++++++ 3 files changed, 26 insertions(+) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index 9e8681f9e1..23474b1eab 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -462,6 +462,10 @@ set by Git if the remote helper has the 'option' capability. 'option pushcert {'true'|'false'}:: GPG sign pushes. +'option push-option <string>:: + Transmit <string> as a push option. As the a push option + must not contain LF or NUL characters, the string is not encoded. + SEE ALSO -------- linkgit:git-remote[1] diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh index ea813b9383..9a57a7d8f2 100755 --- a/t/t5545-push-options.sh +++ b/t/t5545-push-options.sh @@ -3,6 +3,8 @@ test_description='pushing to a repository using push options' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-httpd.sh +start_httpd mk_repo_pair () { rm -rf workbench upstream && @@ -100,4 +102,17 @@ test_expect_success 'two push options work' ' test_cmp expect upstream/.git/hooks/post-receive.push_options ' +test_expect_success 'push option denied properly by http remote helper' '\ + mk_repo_pair && + git -C upstream config receive.advertisePushOptions false && + git -C upstream config http.receivepack true && + cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git && + git clone "$HTTPD_URL"/smart/upstream test_http_clone && + test_commit -C test_http_clone one && + test_must_fail git -C test_http_clone push --push-option=asdf origin master && + git -C test_http_clone push origin master +' + +stop_httpd + test_done diff --git a/transport-helper.c b/transport-helper.c index 91aed35ebb..1258d6aedd 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -826,6 +826,13 @@ static void set_common_push_options(struct transport *transport, if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, "if-asked") != 0) die("helper %s does not support --signed=if-asked", name); } + + if (flags & TRANSPORT_PUSH_OPTIONS) { + struct string_list_item *item; + for_each_string_list_item(item, transport->push_options) + if (set_helper_option(transport, "push-option", item->string) != 0) + die("helper %s does not support 'push-option'", name); + } } static int push_refs_with_push(struct transport *transport, -- 2.12.0.rc0.1.g018cb5e6f4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <CAFzf2XzLNOKbNbkCfzpVJtN-ROW8m8WVBsWM-z7Bxdnv55b45Q@mail.gmail.com>]
* Re: [PATCHv2] push options: pass push options to the transport helper [not found] ` <CAFzf2XzLNOKbNbkCfzpVJtN-ROW8m8WVBsWM-z7Bxdnv55b45Q@mail.gmail.com> @ 2017-02-10 18:44 ` Stefan Beller 0 siblings, 0 replies; 6+ messages in thread From: Stefan Beller @ 2017-02-10 18:44 UTC (permalink / raw) To: Jonathan Nieder, git@vger.kernel.org On Thu, Feb 9, 2017 at 5:56 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Private reply because using HTML mail on phone: So I presume putting it back on the list is fine. > Does this need to be a remote helper capability? No, as all other capabilities of the git-protocol are mapped 1:1 to the transport helper protocol for support, e.g. each transport helper has to handle this on its own, c.f. remote-curl.c:set_option (line 39 ff and 1065), > What happens with remote > helpers that don't understand the option? The helper ought to print "unsupported" (remote-curl.c:1065 for the http helper) and then the transport helper will take care of it (transport-helper.c: 265 and e.g. 820) > > How do remote helpers communicate whether the server has said it accepts > push options? I guess the remote helper would communicate it the same way as communicating if the push succeeded? (i.e. reject non fast forward.) > > On Wed, Feb 8, 2017, 14:04 Stefan Beller <sbeller@google.com> wrote: >> >> When using non-builtin protocols relying on a transport helper >> (such as http), push options are not propagated to the helper. >> >> The user could ask for push options and a push would seemingly succeed, >> but the push options would never be transported to the server, >> misleading the users expectation. >> >> Fix this by propagating the push options to the transport helper. >> >> This is only addressing the first issue of >> (1) the helper protocol does not propagate push-option >> (2) the http helper is not prepared to handle push-option >> >> Once we fix (2), the http transport helper can make use of push options >> as well, but that happens as a follow up. (1) is a bug fix, whereas (2) >> is a feature, which is why we only do (1) here. >> >> Signed-off-by: Stefan Beller <sbeller@google.com> >> --- >> Documentation/gitremote-helpers.txt | 4 ++++ >> t/t5545-push-options.sh | 15 +++++++++++++++ >> transport-helper.c | 7 +++++++ >> 3 files changed, 26 insertions(+) >> >> diff --git a/Documentation/gitremote-helpers.txt >> b/Documentation/gitremote-helpers.txt >> index 9e8681f9e1..23474b1eab 100644 >> --- a/Documentation/gitremote-helpers.txt >> +++ b/Documentation/gitremote-helpers.txt >> @@ -462,6 +462,10 @@ set by Git if the remote helper has the 'option' >> capability. >> 'option pushcert {'true'|'false'}:: >> GPG sign pushes. >> >> +'option push-option <string>:: >> + Transmit <string> as a push option. As the a push option >> + must not contain LF or NUL characters, the string is not encoded. >> + >> SEE ALSO >> -------- >> linkgit:git-remote[1] >> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh >> index ea813b9383..9a57a7d8f2 100755 >> --- a/t/t5545-push-options.sh >> +++ b/t/t5545-push-options.sh >> @@ -3,6 +3,8 @@ >> test_description='pushing to a repository using push options' >> >> . ./test-lib.sh >> +. "$TEST_DIRECTORY"/lib-httpd.sh >> +start_httpd >> >> mk_repo_pair () { >> rm -rf workbench upstream && >> @@ -100,4 +102,17 @@ test_expect_success 'two push options work' ' >> test_cmp expect upstream/.git/hooks/post-receive.push_options >> ' >> >> +test_expect_success 'push option denied properly by http remote helper' >> '\ >> + mk_repo_pair && >> + git -C upstream config receive.advertisePushOptions false && >> + git -C upstream config http.receivepack true && >> + cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git && >> + git clone "$HTTPD_URL"/smart/upstream test_http_clone && >> + test_commit -C test_http_clone one && >> + test_must_fail git -C test_http_clone push --push-option=asdf >> origin master && >> + git -C test_http_clone push origin master >> +' >> + >> +stop_httpd >> + >> test_done >> diff --git a/transport-helper.c b/transport-helper.c >> index 91aed35ebb..1258d6aedd 100644 >> --- a/transport-helper.c >> +++ b/transport-helper.c >> @@ -826,6 +826,13 @@ static void set_common_push_options(struct transport >> *transport, >> if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, >> "if-asked") != 0) >> die("helper %s does not support >> --signed=if-asked", name); >> } >> + >> + if (flags & TRANSPORT_PUSH_OPTIONS) { >> + struct string_list_item *item; >> + for_each_string_list_item(item, transport->push_options) >> + if (set_helper_option(transport, "push-option", >> item->string) != 0) >> + die("helper %s does not support >> 'push-option'", name); >> + } >> } >> >> static int push_refs_with_push(struct transport *transport, >> -- >> 2.12.0.rc0.1.g018cb5e6f4 >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-10 18:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-08 1:09 [PATCH] push options: fail properly in the stateless case Stefan Beller
2017-02-08 18:11 ` Junio C Hamano
2017-02-08 20:51 ` Stefan Beller
2017-02-08 23:01 ` Junio C Hamano
2017-02-08 22:04 ` [PATCHv2] push options: pass push options to the transport helper Stefan Beller
[not found] ` <CAFzf2XzLNOKbNbkCfzpVJtN-ROW8m8WVBsWM-z7Bxdnv55b45Q@mail.gmail.com>
2017-02-10 18:44 ` Stefan Beller
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).