{"thread":{"id":"45470","subject":"[PATCH 2/2] remote-curl: allow push options","startedAt":"2017-03-22T19:51:36Z","lastAt":"2017-03-22T22:30:00Z","messageCount":15,"participants":["Brandon Williams","Jonathan Nieder","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"314954","messageId":"20170322195102.165314-3-bmwill@google.com","threadId":"45470","inReplyTo":"20170322195102.165314-1-bmwill@google.com","subject":"[PATCH 2/2] remote-curl: allow push options","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T19:51:02Z","receivedAt":"2017-03-22T19:51:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach remote-curl to understand push options and to be able to convey\nthem across HTTP.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n builtin/send-pack.c     |  5 +++++\n remote-curl.c           |  8 ++++++++\n t/t5545-push-options.sh | 30 +++++++++++++++++++++++++++++-\n 3 files changed, 42 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 1ff5a6753..6796f3368 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint progress = -1;\n \tint from_stdin = 0;\n \tstruct push_cas_option cas = {0};\n+\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n \n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbose),\n@@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc, N_(\"use stateless RPC protocol\")),\n \t\tOPT_BOOL(0, \"stdin\", &from_stdin, N_(\"read refs from stdin\")),\n \t\tOPT_BOOL(0, \"helper-status\", &helper_status, N_(\"print status from remote helper\")),\n+\t\tOPT_STRING_LIST('o', \"push-option\", &push_options,\n+\t\t\t\tN_(\"server-specific\"),\n+\t\t\t\tN_(\"option to transmit\")),\n \t\t{ OPTION_CALLBACK,\n \t\t  0, CAS_OPT_NAME, &cas, N_(\"refname>:<expect\"),\n \t\t  N_(\"require old value of ref to be at this value\"),\n@@ -199,6 +203,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \targs.use_thin_pack = use_thin_pack;\n \targs.atomic = atomic;\n \targs.stateless_rpc = stateless_rpc;\n+\targs.push_options = push_options.nr ? &push_options : NULL;\n \n \tif (from_stdin) {\n \t\tstruct argv_array all_refspecs = ARGV_ARRAY_INIT;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 34a97e732..e953d06f6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -22,6 +22,7 @@ struct options {\n \tunsigned long depth;\n \tchar *deepen_since;\n \tstruct string_list deepen_not;\n+\tstruct string_list push_options;\n \tunsigned progress : 1,\n \t\tcheck_self_contained_and_connected : 1,\n \t\tcloning : 1,\n@@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)\n \t\telse\n \t\t\treturn -1;\n \t\treturn 0;\n+\t} else if (!strcmp(name, \"push-option\")) {\n+\t\tstring_list_append(&options.push_options, value);\n+\t\treturn 0;\n \n #if LIBCURL_VERSION_NUM >= 0x070a08\n \t} else if (!strcmp(name, \"family\")) {\n@@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \t\targv_array_push(&args, \"--quiet\");\n \telse if (options.verbosity > 1)\n \t\targv_array_push(&args, \"--verbose\");\n+\tfor (i = 0; i < options.push_options.nr; i++)\n+\t\targv_array_pushf(&args, \"--push-option=%s\",\n+\t\t\t\t options.push_options.items[i].string);\n \targv_array_push(&args, options.progress ? \"--progress\" : \"--no-progress\");\n \tfor_each_string_list_item(cas_option, &cas_options)\n \t\targv_array_push(&args, cas_option->string);\n@@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)\n \toptions.progress = !!isatty(2);\n \toptions.thin = 1;\n \tstring_list_init(&options.deepen_not, 1);\n+\tstring_list_init(&options.push_options, 1);\n \n \tremote = remote_get(argv[1]);\n \ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex 9a57a7d8f..ac62083e9 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -102,7 +102,9 @@ test_expect_success 'two push options work' '\n \ttest_cmp expect upstream/.git/hooks/post-receive.push_options\n '\n \n-test_expect_success 'push option denied properly by http remote helper' '\\\n+test_expect_success 'push option denied properly by http server' '\n+\ttest_when_finished \"rm -rf test_http_clone\" &&\n+\ttest_when_finished \"rm -rf \\\"$HTTPD_DOCUMENT_ROOT_PATH\\\"/upstream.git\" &&\n \tmk_repo_pair &&\n \tgit -C upstream config receive.advertisePushOptions false &&\n \tgit -C upstream config http.receivepack true &&\n@@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http remote helper' '\\\n \tgit -C test_http_clone push origin master\n '\n \n+test_expect_success 'push options work properly across http' '\n+\ttest_when_finished \"rm -rf test_http_clone\" &&\n+\ttest_when_finished \"rm -rf \\\"$HTTPD_DOCUMENT_ROOT_PATH\\\"/upstream.git\" &&\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\tgit -C upstream config http.receivepack true &&\n+\tcp -R upstream/.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git &&\n+\tgit clone \"$HTTPD_URL\"/smart/upstream test_http_clone &&\n+\n+\ttest_commit -C test_http_clone one &&\n+\tgit -C test_http_clone push origin master &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git rev-parse --verify master >expect &&\n+\tgit -C test_http_clone rev-parse --verify master >actual &&\n+\ttest_cmp expect actual &&\n+\n+\ttest_commit -C test_http_clone two &&\n+\tgit -C test_http_clone push --push-option=asdf --push-option=\"more structured text\" origin master &&\n+\tprintf \"asdf\\nmore structured text\\n\" >expect &&\n+\ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/pre-receive.push_options &&\n+\ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/post-receive.push_options &&\n+\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git rev-parse --verify master >expect &&\n+\tgit -C test_http_clone rev-parse --verify master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n stop_httpd\n \n test_done\n-- \n2.12.1.500.gab5fba24ee-goog\n\n"},{"id":"314955","messageId":"20170322195102.165314-2-bmwill@google.com","threadId":"45470","inReplyTo":"20170322195102.165314-1-bmwill@google.com","subject":"[PATCH 1/2] send-pack: send push options correctly in stateless-rpc case","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T19:51:01Z","receivedAt":"2017-03-22T19:51:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"\"git send-pack --stateless-rpc\" puts each request in a sequence of pkt-lines\nfollowed by a flush-pkt. The push option code forgot about this and sends push\noptions and their terminating delimiter as ordinary pkt-lines that get their\nlength header stripped off by remote-curl before being sent to the server.\n\nThe result is multiple malformed requests, which the server rejects.\n\nFortunately send-pack --stateless-rpc already is aware of this \"pkt-line within\npkt-line\" framing for the update commands that precede push options. Handle\npush options the same way.\n\nHelped-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n send-pack.c | 20 ++++++++------------\n 1 file changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex d2d2a49a0..66e652f7e 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -532,6 +532,14 @@ int send_pack(struct send_pack_args *args,\n \t\t}\n \t}\n \n+\tif (use_push_options) {\n+\t\tstruct string_list_item *item;\n+\n+\t\tpacket_buf_flush(&req_buf);\n+\t\tfor_each_string_list_item(item, args->push_options)\n+\t\t\tpacket_buf_write(&req_buf, \"%s\", item->string);\n+\t}\n+\n \tif (args->stateless_rpc) {\n \t\tif (!args->dry_run && (cmds_sent || is_repository_shallow())) {\n \t\t\tpacket_buf_flush(&req_buf);\n@@ -544,18 +552,6 @@ int send_pack(struct send_pack_args *args,\n \tstrbuf_release(&req_buf);\n \tstrbuf_release(&cap_buf);\n \n-\tif (use_push_options) {\n-\t\tstruct string_list_item *item;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\n-\t\tfor_each_string_list_item(item, args->push_options)\n-\t\t\tpacket_buf_write(&sb, \"%s\", item->string);\n-\n-\t\twrite_or_die(out, sb.buf, sb.len);\n-\t\tpacket_flush(out);\n-\t\tstrbuf_release(&sb);\n-\t}\n-\n \tif (use_sideband && cmds_sent) {\n \t\tmemset(&demux, 0, sizeof(demux));\n \t\tdemux.proc = sideband_demux;\n-- \n2.12.1.500.gab5fba24ee-goog\n\n"},{"id":"314956","messageId":"20170322195102.165314-1-bmwill@google.com","threadId":"45470","inReplyTo":null,"subject":"[PATCH 0/2] push options across http","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T19:51:00Z","receivedAt":"2017-03-22T19:51:44Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"This series enables push options to be sent across http using remote-curl\n\nThanks to Jonathan Nieder for helping troubleshoot.\n\nBrandon Williams (2):\n  send-pack: send push options correctly in stateless-rpc case\n  remote-curl: allow push options\n\n builtin/send-pack.c     |  5 +++++\n remote-curl.c           |  8 ++++++++\n send-pack.c             | 20 ++++++++------------\n t/t5545-push-options.sh | 30 +++++++++++++++++++++++++++++-\n 4 files changed, 50 insertions(+), 13 deletions(-)\n\n-- \n2.12.1.500.gab5fba24ee-goog\n\n"},{"id":"314969","messageId":"20170322211546.GB26108@aiede.mtv.corp.google.com","threadId":"45470","inReplyTo":"20170322195102.165314-2-bmwill@google.com","subject":"Re: [PATCH 1/2] send-pack: send push options correctly in stateless-rpc case","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-22T21:15:46Z","receivedAt":"2017-03-22T21:16:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Williams wrote:\n\n> \"git send-pack --stateless-rpc\" puts each request in a sequence of pkt-lines\n> followed by a flush-pkt. The push option code forgot about this and sends push\n> options and their terminating delimiter as ordinary pkt-lines that get their\n> length header stripped off by remote-curl before being sent to the server.\n>\n> The result is multiple malformed requests, which the server rejects.\n>\n> Fortunately send-pack --stateless-rpc already is aware of this \"pkt-line within\n> pkt-line\" framing for the update commands that precede push options. Handle\n> push options the same way.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>  send-pack.c | 20 ++++++++------------\n>  1 file changed, 8 insertions(+), 12 deletions(-)\n\nThis is only a hypothetical issue until the next patch though, right?\n\nFor what it's worth,\n\nTested-by: Jonathan Nieder <jrnieder@gmail.com>\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"314971","messageId":"xmqqshm5huzz.fsf@gitster.mtv.corp.google.com","threadId":"45470","inReplyTo":"20170322195102.165314-1-bmwill@google.com","subject":"Re: [PATCH 0/2] push options across http","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T21:17:04Z","receivedAt":"2017-03-22T21:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> This series enables push options to be sent across http using remote-curl\n>\n> Thanks to Jonathan Nieder for helping troubleshoot.\n>\n> Brandon Williams (2):\n>   send-pack: send push options correctly in stateless-rpc case\n>   remote-curl: allow push options\n\nThanks.  Why does the topic name sound familiar to me?  Did we have\na recent attempt to do the same that didn't work well or something?\n\n>\n>  builtin/send-pack.c     |  5 +++++\n>  remote-curl.c           |  8 ++++++++\n>  send-pack.c             | 20 ++++++++------------\n>  t/t5545-push-options.sh | 30 +++++++++++++++++++++++++++++-\n>  4 files changed, 50 insertions(+), 13 deletions(-)\n"},{"id":"314972","messageId":"CAGZ79kYi3Pzr5U7ef16bOADpYYve9=Hg_bBpn8w32CgMLLna0A@mail.gmail.com","threadId":"45470","inReplyTo":"xmqqshm5huzz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] push options across http","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-22T21:20:36Z","receivedAt":"2017-03-22T21:20:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 22, 2017 at 2:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Thanks.  Why does the topic name sound familiar to me?  Did we have\n> a recent attempt to do the same that didn't work well or something?\n\n'sb/push-options-via-transport' sounds similar. Before that we silently\nignored push options in http, with that series we got an error indicating\nhttp doesn't support push options. this series actually adds support for it.\n\nThanks,\nStefan\n"},{"id":"314973","messageId":"xmqqo9wthukz.fsf@gitster.mtv.corp.google.com","threadId":"45470","inReplyTo":"20170322195102.165314-3-bmwill@google.com","subject":"Re: [PATCH 2/2] remote-curl: allow push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T21:26:04Z","receivedAt":"2017-03-22T21:26:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Teach remote-curl to understand push options and to be able to convey\n> them across HTTP.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n\nAn earlier 438fc684 (\"push options: pass push options to the\ntransport helper\", 2017-02-08) said:\n\n    ...\n    Fix this by propagating the push options to the transport helper.\n\n    This is only addressing the first issue of\n       (1) the helper protocol does not propagate push-option\n       (2) the http helper is not prepared to handle push-option\n\n    Once we fix (2), the http transport helper can make use of push options\n    as well, but that happens as a follow up. (1) is a bug fix, whereas (2)\n    is a feature, which is why we only do (1) here.\n\nand this is the step (2) to complete what it started?\n\n>  builtin/send-pack.c     |  5 +++++\n>  remote-curl.c           |  8 ++++++++\n>  t/t5545-push-options.sh | 30 +++++++++++++++++++++++++++++-\n>  3 files changed, 42 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 1ff5a6753..6796f3368 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n>  \tint progress = -1;\n>  \tint from_stdin = 0;\n>  \tstruct push_cas_option cas = {0};\n> +\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n>  \n>  \tstruct option options[] = {\n>  \t\tOPT__VERBOSITY(&verbose),\n> @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc, N_(\"use stateless RPC protocol\")),\n>  \t\tOPT_BOOL(0, \"stdin\", &from_stdin, N_(\"read refs from stdin\")),\n>  \t\tOPT_BOOL(0, \"helper-status\", &helper_status, N_(\"print status from remote helper\")),\n> +\t\tOPT_STRING_LIST('o', \"push-option\", &push_options,\n> +\t\t\t\tN_(\"server-specific\"),\n> +\t\t\t\tN_(\"option to transmit\")),\n>  \t\t{ OPTION_CALLBACK,\n>  \t\t  0, CAS_OPT_NAME, &cas, N_(\"refname>:<expect\"),\n>  \t\t  N_(\"require old value of ref to be at this value\"),\n> @@ -199,6 +203,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n>  \targs.use_thin_pack = use_thin_pack;\n>  \targs.atomic = atomic;\n>  \targs.stateless_rpc = stateless_rpc;\n> +\targs.push_options = push_options.nr ? &push_options : NULL;\n>  \n>  \tif (from_stdin) {\n>  \t\tstruct argv_array all_refspecs = ARGV_ARRAY_INIT;\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 34a97e732..e953d06f6 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -22,6 +22,7 @@ struct options {\n>  \tunsigned long depth;\n>  \tchar *deepen_since;\n>  \tstruct string_list deepen_not;\n> +\tstruct string_list push_options;\n>  \tunsigned progress : 1,\n>  \t\tcheck_self_contained_and_connected : 1,\n>  \t\tcloning : 1,\n> @@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)\n>  \t\telse\n>  \t\t\treturn -1;\n>  \t\treturn 0;\n> +\t} else if (!strcmp(name, \"push-option\")) {\n> +\t\tstring_list_append(&options.push_options, value);\n> +\t\treturn 0;\n>  \n>  #if LIBCURL_VERSION_NUM >= 0x070a08\n>  \t} else if (!strcmp(name, \"family\")) {\n> @@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n>  \t\targv_array_push(&args, \"--quiet\");\n>  \telse if (options.verbosity > 1)\n>  \t\targv_array_push(&args, \"--verbose\");\n> +\tfor (i = 0; i < options.push_options.nr; i++)\n> +\t\targv_array_pushf(&args, \"--push-option=%s\",\n> +\t\t\t\t options.push_options.items[i].string);\n>  \targv_array_push(&args, options.progress ? \"--progress\" : \"--no-progress\");\n>  \tfor_each_string_list_item(cas_option, &cas_options)\n>  \t\targv_array_push(&args, cas_option->string);\n> @@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)\n>  \toptions.progress = !!isatty(2);\n>  \toptions.thin = 1;\n>  \tstring_list_init(&options.deepen_not, 1);\n> +\tstring_list_init(&options.push_options, 1);\n>  \n>  \tremote = remote_get(argv[1]);\n>  \n> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\n> index 9a57a7d8f..ac62083e9 100755\n> --- a/t/t5545-push-options.sh\n> +++ b/t/t5545-push-options.sh\n> @@ -102,7 +102,9 @@ test_expect_success 'two push options work' '\n>  \ttest_cmp expect upstream/.git/hooks/post-receive.push_options\n>  '\n>  \n> -test_expect_success 'push option denied properly by http remote helper' '\\\n> +test_expect_success 'push option denied properly by http server' '\n> +\ttest_when_finished \"rm -rf test_http_clone\" &&\n> +\ttest_when_finished \"rm -rf \\\"$HTTPD_DOCUMENT_ROOT_PATH\\\"/upstream.git\" &&\n>  \tmk_repo_pair &&\n>  \tgit -C upstream config receive.advertisePushOptions false &&\n>  \tgit -C upstream config http.receivepack true &&\n> @@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http remote helper' '\\\n>  \tgit -C test_http_clone push origin master\n>  '\n>  \n> +test_expect_success 'push options work properly across http' '\n> +\ttest_when_finished \"rm -rf test_http_clone\" &&\n> +\ttest_when_finished \"rm -rf \\\"$HTTPD_DOCUMENT_ROOT_PATH\\\"/upstream.git\" &&\n> +\tmk_repo_pair &&\n> +\tgit -C upstream config receive.advertisePushOptions true &&\n> +\tgit -C upstream config http.receivepack true &&\n> +\tcp -R upstream/.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git &&\n> +\tgit clone \"$HTTPD_URL\"/smart/upstream test_http_clone &&\n> +\n> +\ttest_commit -C test_http_clone one &&\n> +\tgit -C test_http_clone push origin master &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git rev-parse --verify master >expect &&\n> +\tgit -C test_http_clone rev-parse --verify master >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\ttest_commit -C test_http_clone two &&\n> +\tgit -C test_http_clone push --push-option=asdf --push-option=\"more structured text\" origin master &&\n> +\tprintf \"asdf\\nmore structured text\\n\" >expect &&\n> +\ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/pre-receive.push_options &&\n> +\ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/post-receive.push_options &&\n> +\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git rev-parse --verify master >expect &&\n> +\tgit -C test_http_clone rev-parse --verify master >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  stop_httpd\n>  \n>  test_done\n"},{"id":"314978","messageId":"20170322213634.GB11254@google.com","threadId":"45470","inReplyTo":"xmqqo9wthukz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] remote-curl: allow push options","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T21:36:34Z","receivedAt":"2017-03-22T21:36:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/22, Junio C Hamano wrote:\n> Brandon Williams <bmwill@google.com> writes:\n> \n> > Teach remote-curl to understand push options and to be able to convey\n> > them across HTTP.\n> >\n> > Signed-off-by: Brandon Williams <bmwill@google.com>\n> > ---\n> \n> An earlier 438fc684 (\"push options: pass push options to the\n> transport helper\", 2017-02-08) said:\n> \n>     ...\n>     Fix this by propagating the push options to the transport helper.\n> \n>     This is only addressing the first issue of\n>        (1) the helper protocol does not propagate push-option\n>        (2) the http helper is not prepared to handle push-option\n> \n>     Once we fix (2), the http transport helper can make use of push options\n>     as well, but that happens as a follow up. (1) is a bug fix, whereas (2)\n>     is a feature, which is why we only do (1) here.\n> \n> and this is the step (2) to complete what it started?\n\nYep!  As far as I understand from looking at that commit, push options\nwere being passed to remote-curl and remote-curl was silently ignoring\nthem.  So that patch made it complain when a remote-helper didn't\nsupport push options.\n\nThis patch teaches remote-curl to actually support push options, fixing\n(2).\n\n-- \nBrandon Williams\n"},{"id":"314983","messageId":"20170322213826.GC26108@aiede.mtv.corp.google.com","threadId":"45470","inReplyTo":"20170322195102.165314-3-bmwill@google.com","subject":"Re: [PATCH 2/2] remote-curl: allow push options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-22T21:38:26Z","receivedAt":"2017-03-22T21:46:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Williams wrote:\n\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n>  \tint progress = -1;\n>  \tint from_stdin = 0;\n>  \tstruct push_cas_option cas = {0};\n> +\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n\nIt's safe for this to be NODUP, since the strings added to it live in\nargv.\n\n>  \n>  \tstruct option options[] = {\n>  \t\tOPT__VERBOSITY(&verbose),\n> @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc, N_(\"use stateless RPC protocol\")),\n>  \t\tOPT_BOOL(0, \"stdin\", &from_stdin, N_(\"read refs from stdin\")),\n>  \t\tOPT_BOOL(0, \"helper-status\", &helper_status, N_(\"print status from remote helper\")),\n> +\t\tOPT_STRING_LIST('o', \"push-option\", &push_options,\n> +\t\t\t\tN_(\"server-specific\"),\n> +\t\t\t\tN_(\"option to transmit\")),\n\nDoes this need the short-and-sweet option name 'o'?  For this command,\nI think I'd prefer giving it no short name.\n\nShould this option be documented in the manpage, too?\n\n[...]\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -22,6 +22,7 @@ struct options {\n>  \tunsigned long depth;\n>  \tchar *deepen_since;\n>  \tstruct string_list deepen_not;\n> +\tstruct string_list push_options;\n>  \tunsigned progress : 1,\n>  \t\tcheck_self_contained_and_connected : 1,\n>  \t\tcloning : 1,\n> @@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)\n>  \t\telse\n>  \t\t\treturn -1;\n>  \t\treturn 0;\n> +\t} else if (!strcmp(name, \"push-option\")) {\n> +\t\tstring_list_append(&options.push_options, value);\n> +\t\treturn 0;\n\npush_options has strdup_strings enabled so this takes ownership of a\ncopy of value.  Good.\n\n[...]\n> --- a/t/t5545-push-options.sh\n> +++ b/t/t5545-push-options.sh\n> @@ -102,7 +102,9 @@ test_expect_success 'two push options work' '\n>  \ttest_cmp expect upstream/.git/hooks/post-receive.push_options\n>  '\n>  \n> -test_expect_success 'push option denied properly by http remote helper' '\\\n> +test_expect_success 'push option denied properly by http server' '\n\nShould this test use test_i18ngrep to check that the error message\ndiagnoses the problem correctly instead of hitting an unrelated error\ncondition?\n\n[...]\n> @@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http remote helper' '\\\n>  \tgit -C test_http_clone push origin master\n>  '\n>  \n> +test_expect_success 'push options work properly across http' '\n\nNice.\n\nTested-by: Jonathan Nieder <jrnieder@gmail.com>\n\nWith whatever subset of the following changes seems sensible to you\nsquashed in,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\ndiff --git i/Documentation/git-send-pack.txt w/Documentation/git-send-pack.txt\nindex a831dd0288..966abb0df8 100644\n--- i/Documentation/git-send-pack.txt\n+++ w/Documentation/git-send-pack.txt\n@@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a flush packet.\n \twill also fail if the actual call to `gpg --sign` fails.  See\n \tlinkgit:git-receive-pack[1] for the details on the receiving end.\n \n+--push-option=<string>::\n+\tPass the specified string as a push option for consumption by\n+\thooks on the server side.  If the server doesn't support push\n+\toptions, error out.  See linkgit:git-push[1] and\n+\tlinkgit:githooks[5] for details.\n+\n <host>::\n \tA remote host to house the repository.  When this\n \tpart is specified, 'git-receive-pack' is invoked via\ndiff --git i/builtin/send-pack.c w/builtin/send-pack.c\nindex 6796f33687..832fd7ed0a 100644\n--- i/builtin/send-pack.c\n+++ w/builtin/send-pack.c\n@@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tunsigned force_update = 0;\n \tunsigned quiet = 0;\n \tint push_cert = 0;\n+\tstruct string_list push_options = STRING_LIST_INIT_NODUP;\n \tunsigned use_thin_pack = 0;\n \tunsigned atomic = 0;\n \tunsigned stateless_rpc = 0;\n@@ -152,7 +153,6 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint progress = -1;\n \tint from_stdin = 0;\n \tstruct push_cas_option cas = {0};\n-\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n \n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbose),\n@@ -166,15 +166,15 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK,\n \t\t  0, \"signed\", &push_cert, \"yes|no|if-asked\", N_(\"GPG sign the push\"),\n \t\t  PARSE_OPT_OPTARG, option_parse_push_signed },\n+\t\tOPT_STRING_LIST(0, \"push-option\", &push_options,\n+\t\t\t\tN_(\"server-specific\"),\n+\t\t\t\tN_(\"option to transmit\")),\n \t\tOPT_BOOL(0, \"progress\", &progress, N_(\"force progress reporting\")),\n \t\tOPT_BOOL(0, \"thin\", &use_thin_pack, N_(\"use thin pack\")),\n \t\tOPT_BOOL(0, \"atomic\", &atomic, N_(\"request atomic transaction on remote side\")),\n \t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc, N_(\"use stateless RPC protocol\")),\n \t\tOPT_BOOL(0, \"stdin\", &from_stdin, N_(\"read refs from stdin\")),\n \t\tOPT_BOOL(0, \"helper-status\", &helper_status, N_(\"print status from remote helper\")),\n-\t\tOPT_STRING_LIST('o', \"push-option\", &push_options,\n-\t\t\t\tN_(\"server-specific\"),\n-\t\t\t\tN_(\"option to transmit\")),\n \t\t{ OPTION_CALLBACK,\n \t\t  0, CAS_OPT_NAME, &cas, N_(\"refname>:<expect\"),\n \t\t  N_(\"require old value of ref to be at this value\"),\n"},{"id":"314995","messageId":"20170322221312.GD11254@google.com","threadId":"45470","inReplyTo":"20170322213826.GC26108@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/2] remote-curl: allow push options","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T22:13:12Z","receivedAt":"2017-03-22T22:13:19Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/22, Jonathan Nieder wrote:\n\nI agree with most of these changes,  I'll make them locally and send out\na reroll.\n\n> Brandon Williams wrote:\n> \n> > --- a/builtin/send-pack.c\n> > +++ b/builtin/send-pack.c\n> > @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n> >  \tint progress = -1;\n> >  \tint from_stdin = 0;\n> >  \tstruct push_cas_option cas = {0};\n> > +\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n> \n> It's safe for this to be NODUP, since the strings added to it live in\n> argv.\n> \n> >  \n> >  \tstruct option options[] = {\n> >  \t\tOPT__VERBOSITY(&verbose),\n> > @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n> >  \t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc, N_(\"use stateless RPC protocol\")),\n> >  \t\tOPT_BOOL(0, \"stdin\", &from_stdin, N_(\"read refs from stdin\")),\n> >  \t\tOPT_BOOL(0, \"helper-status\", &helper_status, N_(\"print status from remote helper\")),\n> > +\t\tOPT_STRING_LIST('o', \"push-option\", &push_options,\n> > +\t\t\t\tN_(\"server-specific\"),\n> > +\t\t\t\tN_(\"option to transmit\")),\n> \n> Does this need the short-and-sweet option name 'o'?  For this command,\n> I think I'd prefer giving it no short name.\n> \n> Should this option be documented in the manpage, too?\n> \n> [...]\n> > --- a/remote-curl.c\n> > +++ b/remote-curl.c\n> > @@ -22,6 +22,7 @@ struct options {\n> >  \tunsigned long depth;\n> >  \tchar *deepen_since;\n> >  \tstruct string_list deepen_not;\n> > +\tstruct string_list push_options;\n> >  \tunsigned progress : 1,\n> >  \t\tcheck_self_contained_and_connected : 1,\n> >  \t\tcloning : 1,\n> > @@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)\n> >  \t\telse\n> >  \t\t\treturn -1;\n> >  \t\treturn 0;\n> > +\t} else if (!strcmp(name, \"push-option\")) {\n> > +\t\tstring_list_append(&options.push_options, value);\n> > +\t\treturn 0;\n> \n> push_options has strdup_strings enabled so this takes ownership of a\n> copy of value.  Good.\n> \n> [...]\n> > --- a/t/t5545-push-options.sh\n> > +++ b/t/t5545-push-options.sh\n> > @@ -102,7 +102,9 @@ test_expect_success 'two push options work' '\n> >  \ttest_cmp expect upstream/.git/hooks/post-receive.push_options\n> >  '\n> >  \n> > -test_expect_success 'push option denied properly by http remote helper' '\\\n> > +test_expect_success 'push option denied properly by http server' '\n> \n> Should this test use test_i18ngrep to check that the error message\n> diagnoses the problem correctly instead of hitting an unrelated error\n> condition?\n> \n> [...]\n> > @@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http remote helper' '\\\n> >  \tgit -C test_http_clone push origin master\n> >  '\n> >  \n> > +test_expect_success 'push options work properly across http' '\n> \n> Nice.\n> \n> Tested-by: Jonathan Nieder <jrnieder@gmail.com>\n> \n> With whatever subset of the following changes seems sensible to you\n> squashed in,\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> \n> Thanks.\n> \n> diff --git i/Documentation/git-send-pack.txt w/Documentation/git-send-pack.txt\n> index a831dd0288..966abb0df8 100644\n> --- i/Documentation/git-send-pack.txt\n> +++ w/Documentation/git-send-pack.txt\n> @@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a flush packet.\n>  \twill also fail if the actual call to `gpg --sign` fails.  See\n>  \tlinkgit:git-receive-pack[1] for the details on the receiving end.\n>  \n> +--push-option=<string>::\n> +\tPass the specified string as a push option for consumption by\n> +\thooks on the server side.  If the server doesn't support push\n> +\toptions, error out.  See linkgit:git-push[1] and\n> +\tlinkgit:githooks[5] for details.\n> +\n>  <host>::\n>  \tA remote host to house the repository.  When this\n>  \tpart is specified, 'git-receive-pack' is invoked via\n> diff --git i/builtin/send-pack.c w/builtin/send-pack.c\n> index 6796f33687..832fd7ed0a 100644\n> --- i/builtin/send-pack.c\n> +++ w/builtin/send-pack.c\n> @@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n>  \tunsigned force_update = 0;\n>  \tunsigned quiet = 0;\n>  \tint push_cert = 0;\n> +\tstruct string_list push_options = STRING_LIST_INIT_NODUP;\n>  \tunsigned use_thin_pack = 0;\n>  \tunsigned atomic = 0;\n>  \tunsigned stateless_rpc = 0;\n> @@ -152,7 +153,6 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n>  \tint progress = -1;\n>  \tint from_stdin = 0;\n>  \tstruct push_cas_option cas = {0};\n> -\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n>  \n>  \tstruct option options[] = {\n>  \t\tOPT__VERBOSITY(&verbose),\n> @@ -166,15 +166,15 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n>  \t\t{ OPTION_CALLBACK,\n>  \t\t  0, \"signed\", &push_cert, \"yes|no|if-asked\", N_(\"GPG sign the push\"),\n>  \t\t  PARSE_OPT_OPTARG, option_parse_push_signed },\n> +\t\tOPT_STRING_LIST(0, \"push-option\", &push_options,\n> +\t\t\t\tN_(\"server-specific\"),\n> +\t\t\t\tN_(\"option to transmit\")),\n>  \t\tOPT_BOOL(0, \"progress\", &progress, N_(\"force progress reporting\")),\n>  \t\tOPT_BOOL(0, \"thin\", &use_thin_pack, N_(\"use thin pack\")),\n>  \t\tOPT_BOOL(0, \"atomic\", &atomic, N_(\"request atomic transaction on remote side\")),\n>  \t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc, N_(\"use stateless RPC protocol\")),\n>  \t\tOPT_BOOL(0, \"stdin\", &from_stdin, N_(\"read refs from stdin\")),\n>  \t\tOPT_BOOL(0, \"helper-status\", &helper_status, N_(\"print status from remote helper\")),\n> -\t\tOPT_STRING_LIST('o', \"push-option\", &push_options,\n> -\t\t\t\tN_(\"server-specific\"),\n> -\t\t\t\tN_(\"option to transmit\")),\n>  \t\t{ OPTION_CALLBACK,\n>  \t\t  0, CAS_OPT_NAME, &cas, N_(\"refname>:<expect\"),\n>  \t\t  N_(\"require old value of ref to be at this value\"),\n\n-- \nBrandon Williams\n"},{"id":"315001","messageId":"20170322221950.GE11254@google.com","threadId":"45470","inReplyTo":"20170322211546.GB26108@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/2] send-pack: send push options correctly in stateless-rpc case","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T22:19:50Z","receivedAt":"2017-03-22T22:19:57Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/22, Jonathan Nieder wrote:\n> Brandon Williams wrote:\n> \n> > \"git send-pack --stateless-rpc\" puts each request in a sequence of pkt-lines\n> > followed by a flush-pkt. The push option code forgot about this and sends push\n> > options and their terminating delimiter as ordinary pkt-lines that get their\n> > length header stripped off by remote-curl before being sent to the server.\n> >\n> > The result is multiple malformed requests, which the server rejects.\n> >\n> > Fortunately send-pack --stateless-rpc already is aware of this \"pkt-line within\n> > pkt-line\" framing for the update commands that precede push options. Handle\n> > push options the same way.\n> >\n> > Signed-off-by: Brandon Williams <bmwill@google.com>\n> > ---\n> >  send-pack.c | 20 ++++++++------------\n> >  1 file changed, 8 insertions(+), 12 deletions(-)\n> \n> This is only a hypothetical issue until the next patch though, right?\n\nCorrect, Its in preparation for the next patch.\n\n> \n> For what it's worth,\n> \n> Tested-by: Jonathan Nieder <jrnieder@gmail.com>\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> \n> Thanks.\n\n-- \nBrandon Williams\n"},{"id":"315002","messageId":"20170322222200.103204-1-bmwill@google.com","threadId":"45470","inReplyTo":"20170322195102.165314-1-bmwill@google.com","subject":"[PATCH v2 0/2] push options across http","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T22:21:58Z","receivedAt":"2017-03-22T22:22:10Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"v2 addresses Jonathan's comments from v1.\n  * Fix a test\n  * Add some documentation\n  * remove short option from --push-option in git send-pack\n\nBrandon Williams (2):\n  send-pack: send push options correctly in stateless-rpc case\n  remote-curl: allow push options\n\n Documentation/git-send-pack.txt |  6 ++++++\n builtin/send-pack.c             |  5 +++++\n remote-curl.c                   |  8 ++++++++\n send-pack.c                     | 20 ++++++++------------\n t/t5545-push-options.sh         | 33 +++++++++++++++++++++++++++++++--\n 5 files changed, 58 insertions(+), 14 deletions(-)\n\n-- \n2.12.1.500.gab5fba24ee-goog\n\n"},{"id":"315003","messageId":"20170322222200.103204-2-bmwill@google.com","threadId":"45470","inReplyTo":"20170322222200.103204-1-bmwill@google.com","subject":"[PATCH v2 1/2] send-pack: send push options correctly in stateless-rpc case","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T22:21:59Z","receivedAt":"2017-03-22T22:22:14Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"\"git send-pack --stateless-rpc\" puts each request in a sequence of pkt-lines\nfollowed by a flush-pkt. The push option code forgot about this and sends push\noptions and their terminating delimiter as ordinary pkt-lines that get their\nlength header stripped off by remote-curl before being sent to the server.\n\nThe result is multiple malformed requests, which the server rejects.\n\nFortunately send-pack --stateless-rpc already is aware of this \"pkt-line within\npkt-line\" framing for the update commands that precede push options. Handle\npush options the same way.\n\nHelped-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n send-pack.c | 20 ++++++++------------\n 1 file changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex d2d2a49a0..66e652f7e 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -532,6 +532,14 @@ int send_pack(struct send_pack_args *args,\n \t\t}\n \t}\n \n+\tif (use_push_options) {\n+\t\tstruct string_list_item *item;\n+\n+\t\tpacket_buf_flush(&req_buf);\n+\t\tfor_each_string_list_item(item, args->push_options)\n+\t\t\tpacket_buf_write(&req_buf, \"%s\", item->string);\n+\t}\n+\n \tif (args->stateless_rpc) {\n \t\tif (!args->dry_run && (cmds_sent || is_repository_shallow())) {\n \t\t\tpacket_buf_flush(&req_buf);\n@@ -544,18 +552,6 @@ int send_pack(struct send_pack_args *args,\n \tstrbuf_release(&req_buf);\n \tstrbuf_release(&cap_buf);\n \n-\tif (use_push_options) {\n-\t\tstruct string_list_item *item;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\n-\t\tfor_each_string_list_item(item, args->push_options)\n-\t\t\tpacket_buf_write(&sb, \"%s\", item->string);\n-\n-\t\twrite_or_die(out, sb.buf, sb.len);\n-\t\tpacket_flush(out);\n-\t\tstrbuf_release(&sb);\n-\t}\n-\n \tif (use_sideband && cmds_sent) {\n \t\tmemset(&demux, 0, sizeof(demux));\n \t\tdemux.proc = sideband_demux;\n-- \n2.12.1.500.gab5fba24ee-goog\n\n"},{"id":"315004","messageId":"20170322222200.103204-3-bmwill@google.com","threadId":"45470","inReplyTo":"20170322222200.103204-1-bmwill@google.com","subject":"[PATCH v2 2/2] remote-curl: allow push options","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T22:22:00Z","receivedAt":"2017-03-22T22:22:17Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach remote-curl to understand push options and to be able to convey\nthem across HTTP.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n Documentation/git-send-pack.txt |  6 ++++++\n builtin/send-pack.c             |  5 +++++\n remote-curl.c                   |  8 ++++++++\n t/t5545-push-options.sh         | 33 +++++++++++++++++++++++++++++++--\n 4 files changed, 50 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex a831dd028..966abb0df 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a flush packet.\n \twill also fail if the actual call to `gpg --sign` fails.  See\n \tlinkgit:git-receive-pack[1] for the details on the receiving end.\n \n+--push-option=<string>::\n+\tPass the specified string as a push option for consumption by\n+\thooks on the server side.  If the server doesn't support push\n+\toptions, error out.  See linkgit:git-push[1] and\n+\tlinkgit:githooks[5] for details.\n+\n <host>::\n \tA remote host to house the repository.  When this\n \tpart is specified, 'git-receive-pack' is invoked via\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 1ff5a6753..832fd7ed0 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tunsigned force_update = 0;\n \tunsigned quiet = 0;\n \tint push_cert = 0;\n+\tstruct string_list push_options = STRING_LIST_INIT_NODUP;\n \tunsigned use_thin_pack = 0;\n \tunsigned atomic = 0;\n \tunsigned stateless_rpc = 0;\n@@ -165,6 +166,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK,\n \t\t  0, \"signed\", &push_cert, \"yes|no|if-asked\", N_(\"GPG sign the push\"),\n \t\t  PARSE_OPT_OPTARG, option_parse_push_signed },\n+\t\tOPT_STRING_LIST(0, \"push-option\", &push_options,\n+\t\t\t\tN_(\"server-specific\"),\n+\t\t\t\tN_(\"option to transmit\")),\n \t\tOPT_BOOL(0, \"progress\", &progress, N_(\"force progress reporting\")),\n \t\tOPT_BOOL(0, \"thin\", &use_thin_pack, N_(\"use thin pack\")),\n \t\tOPT_BOOL(0, \"atomic\", &atomic, N_(\"request atomic transaction on remote side\")),\n@@ -199,6 +203,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \targs.use_thin_pack = use_thin_pack;\n \targs.atomic = atomic;\n \targs.stateless_rpc = stateless_rpc;\n+\targs.push_options = push_options.nr ? &push_options : NULL;\n \n \tif (from_stdin) {\n \t\tstruct argv_array all_refspecs = ARGV_ARRAY_INIT;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 34a97e732..e953d06f6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -22,6 +22,7 @@ struct options {\n \tunsigned long depth;\n \tchar *deepen_since;\n \tstruct string_list deepen_not;\n+\tstruct string_list push_options;\n \tunsigned progress : 1,\n \t\tcheck_self_contained_and_connected : 1,\n \t\tcloning : 1,\n@@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)\n \t\telse\n \t\t\treturn -1;\n \t\treturn 0;\n+\t} else if (!strcmp(name, \"push-option\")) {\n+\t\tstring_list_append(&options.push_options, value);\n+\t\treturn 0;\n \n #if LIBCURL_VERSION_NUM >= 0x070a08\n \t} else if (!strcmp(name, \"family\")) {\n@@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \t\targv_array_push(&args, \"--quiet\");\n \telse if (options.verbosity > 1)\n \t\targv_array_push(&args, \"--verbose\");\n+\tfor (i = 0; i < options.push_options.nr; i++)\n+\t\targv_array_pushf(&args, \"--push-option=%s\",\n+\t\t\t\t options.push_options.items[i].string);\n \targv_array_push(&args, options.progress ? \"--progress\" : \"--no-progress\");\n \tfor_each_string_list_item(cas_option, &cas_options)\n \t\targv_array_push(&args, cas_option->string);\n@@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)\n \toptions.progress = !!isatty(2);\n \toptions.thin = 1;\n \tstring_list_init(&options.deepen_not, 1);\n+\tstring_list_init(&options.push_options, 1);\n \n \tremote = remote_get(argv[1]);\n \ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex 9a57a7d8f..97065e62b 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -102,17 +102,46 @@ test_expect_success 'two push options work' '\n \ttest_cmp expect upstream/.git/hooks/post-receive.push_options\n '\n \n-test_expect_success 'push option denied properly by http remote helper' '\\\n+test_expect_success 'push option denied properly by http server' '\n+\ttest_when_finished \"rm -rf test_http_clone\" &&\n+\ttest_when_finished \"rm -rf \\\"$HTTPD_DOCUMENT_ROOT_PATH\\\"/upstream.git\" &&\n \tmk_repo_pair &&\n \tgit -C upstream config receive.advertisePushOptions false &&\n \tgit -C upstream config http.receivepack true &&\n \tcp -R upstream/.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git &&\n \tgit clone \"$HTTPD_URL\"/smart/upstream test_http_clone &&\n \ttest_commit -C test_http_clone one &&\n-\ttest_must_fail git -C test_http_clone push --push-option=asdf origin master &&\n+\ttest_must_fail git -C test_http_clone push --push-option=asdf origin master 2>actual &&\n+\ttest_i18ngrep \"the receiving end does not support push options\" actual &&\n \tgit -C test_http_clone push origin master\n '\n \n+test_expect_success 'push options work properly across http' '\n+\ttest_when_finished \"rm -rf test_http_clone\" &&\n+\ttest_when_finished \"rm -rf \\\"$HTTPD_DOCUMENT_ROOT_PATH\\\"/upstream.git\" &&\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\tgit -C upstream config http.receivepack true &&\n+\tcp -R upstream/.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git &&\n+\tgit clone \"$HTTPD_URL\"/smart/upstream test_http_clone &&\n+\n+\ttest_commit -C test_http_clone one &&\n+\tgit -C test_http_clone push origin master &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git rev-parse --verify master >expect &&\n+\tgit -C test_http_clone rev-parse --verify master >actual &&\n+\ttest_cmp expect actual &&\n+\n+\ttest_commit -C test_http_clone two &&\n+\tgit -C test_http_clone push --push-option=asdf --push-option=\"more structured text\" origin master &&\n+\tprintf \"asdf\\nmore structured text\\n\" >expect &&\n+\ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/pre-receive.push_options &&\n+\ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/post-receive.push_options &&\n+\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git rev-parse --verify master >expect &&\n+\tgit -C test_http_clone rev-parse --verify master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n stop_httpd\n \n test_done\n-- \n2.12.1.500.gab5fba24ee-goog\n\n"},{"id":"315009","messageId":"20170322222918.GF26108@aiede.mtv.corp.google.com","threadId":"45470","inReplyTo":"20170322222200.103204-1-bmwill@google.com","subject":"Re: [PATCH v2 0/2] push options across http","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-22T22:29:18Z","receivedAt":"2017-03-22T22:30:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Williams wrote:\n\n> v2 addresses Jonathan's comments from v1.\n>   * Fix a test\n>   * Add some documentation\n>   * remove short option from --push-option in git send-pack\n> \n> Brandon Williams (2):\n>   send-pack: send push options correctly in stateless-rpc case\n>   remote-curl: allow push options\n> \n>  Documentation/git-send-pack.txt |  6 ++++++\n>  builtin/send-pack.c             |  5 +++++\n>  remote-curl.c                   |  8 ++++++++\n>  send-pack.c                     | 20 ++++++++------------\n>  t/t5545-push-options.sh         | 33 +++++++++++++++++++++++++++++++--\n>  5 files changed, 58 insertions(+), 14 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\nTested-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"}]}