[PATCH 0/2] push options across http

[PATCH 0/2] push options across http

[Brandon Williams]

This series enables push options to be sent across http using remote-curl

Thanks to Jonathan Nieder for helping troubleshoot.

Brandon Williams (2):
  send-pack: send push options correctly in stateless-rpc case
  remote-curl: allow push options

 builtin/send-pack.c     |  5 +++++
 remote-curl.c           |  8 ++++++++
 send-pack.c             | 20 ++++++++------------
 t/t5545-push-options.sh | 30 +++++++++++++++++++++++++++++-
 4 files changed, 50 insertions(+), 13 deletions(-)

-- 
2.12.1.500.gab5fba24ee-goog

[PATCH 1/2] send-pack: send push options correctly in stateless-rpc case

[Brandon Williams]

"git send-pack --stateless-rpc" puts each request in a sequence of pkt-lines
followed by a flush-pkt. The push option code forgot about this and sends push
options and their terminating delimiter as ordinary pkt-lines that get their
length header stripped off by remote-curl before being sent to the server.

The result is multiple malformed requests, which the server rejects.

Fortunately send-pack --stateless-rpc already is aware of this "pkt-line within
pkt-line" framing for the update commands that precede push options. Handle
push options the same way.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 send-pack.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index d2d2a49a0..66e652f7e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -532,6 +532,14 @@ int send_pack(struct send_pack_args *args,
 		}
 	}
 
+	if (use_push_options) {
+		struct string_list_item *item;
+
+		packet_buf_flush(&req_buf);
+		for_each_string_list_item(item, args->push_options)
+			packet_buf_write(&req_buf, "%s", item->string);
+	}
+
 	if (args->stateless_rpc) {
 		if (!args->dry_run && (cmds_sent || is_repository_shallow())) {
 			packet_buf_flush(&req_buf);
@@ -544,18 +552,6 @@ int send_pack(struct send_pack_args *args,
 	strbuf_release(&req_buf);
 	strbuf_release(&cap_buf);
 
-	if (use_push_options) {
-		struct string_list_item *item;
-		struct strbuf sb = STRBUF_INIT;
-
-		for_each_string_list_item(item, args->push_options)
-			packet_buf_write(&sb, "%s", item->string);
-
-		write_or_die(out, sb.buf, sb.len);
-		packet_flush(out);
-		strbuf_release(&sb);
-	}
-
 	if (use_sideband && cmds_sent) {
 		memset(&demux, 0, sizeof(demux));
 		demux.proc = sideband_demux;
-- 
2.12.1.500.gab5fba24ee-goog

[PATCH 2/2] remote-curl: allow push options

[Brandon Williams]

Teach remote-curl to understand push options and to be able to convey
them across HTTP.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/send-pack.c     |  5 +++++
 remote-curl.c           |  8 ++++++++
 t/t5545-push-options.sh | 30 +++++++++++++++++++++++++++++-
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 1ff5a6753..6796f3368 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int progress = -1;
 	int from_stdin = 0;
 	struct push_cas_option cas = {0};
+	struct string_list push_options = STRING_LIST_INIT_DUP;
 
 	struct option options[] = {
 		OPT__VERBOSITY(&verbose),
@@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "stateless-rpc", &stateless_rpc, N_("use stateless RPC protocol")),
 		OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")),
 		OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")),
+		OPT_STRING_LIST('o', "push-option", &push_options,
+				N_("server-specific"),
+				N_("option to transmit")),
 		{ OPTION_CALLBACK,
 		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
 		  N_("require old value of ref to be at this value"),
@@ -199,6 +203,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	args.use_thin_pack = use_thin_pack;
 	args.atomic = atomic;
 	args.stateless_rpc = stateless_rpc;
+	args.push_options = push_options.nr ? &push_options : NULL;
 
 	if (from_stdin) {
 		struct argv_array all_refspecs = ARGV_ARRAY_INIT;
diff --git a/remote-curl.c b/remote-curl.c
index 34a97e732..e953d06f6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -22,6 +22,7 @@ struct options {
 	unsigned long depth;
 	char *deepen_since;
 	struct string_list deepen_not;
+	struct string_list push_options;
 	unsigned progress : 1,
 		check_self_contained_and_connected : 1,
 		cloning : 1,
@@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)
 		else
 			return -1;
 		return 0;
+	} else if (!strcmp(name, "push-option")) {
+		string_list_append(&options.push_options, value);
+		return 0;
 
 #if LIBCURL_VERSION_NUM >= 0x070a08
 	} else if (!strcmp(name, "family")) {
@@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 		argv_array_push(&args, "--quiet");
 	else if (options.verbosity > 1)
 		argv_array_push(&args, "--verbose");
+	for (i = 0; i < options.push_options.nr; i++)
+		argv_array_pushf(&args, "--push-option=%s",
+				 options.push_options.items[i].string);
 	argv_array_push(&args, options.progress ? "--progress" : "--no-progress");
 	for_each_string_list_item(cas_option, &cas_options)
 		argv_array_push(&args, cas_option->string);
@@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)
 	options.progress = !!isatty(2);
 	options.thin = 1;
 	string_list_init(&options.deepen_not, 1);
+	string_list_init(&options.push_options, 1);
 
 	remote = remote_get(argv[1]);
 
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 9a57a7d8f..ac62083e9 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -102,7 +102,9 @@ 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' '\
+test_expect_success 'push option denied properly by http server' '
+	test_when_finished "rm -rf test_http_clone" &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
 	mk_repo_pair &&
 	git -C upstream config receive.advertisePushOptions false &&
 	git -C upstream config http.receivepack true &&
@@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http remote helper' '\
 	git -C test_http_clone push origin master
 '
 
+test_expect_success 'push options work properly across http' '
+	test_when_finished "rm -rf test_http_clone" &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	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 &&
+	git -C test_http_clone push origin master &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
+	git -C test_http_clone rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	test_commit -C test_http_clone two &&
+	git -C test_http_clone push --push-option=asdf --push-option="more structured text" origin master &&
+	printf "asdf\nmore structured text\n" >expect &&
+	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options &&
+	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/post-receive.push_options &&
+
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
+	git -C test_http_clone rev-parse --verify master >actual &&
+	test_cmp expect actual
+'
+
 stop_httpd
 
 test_done
-- 
2.12.1.500.gab5fba24ee-goog

Re: [PATCH 1/2] send-pack: send push options correctly in stateless-rpc case

[Jonathan Nieder]

Brandon Williams wrote:

> "git send-pack --stateless-rpc" puts each request in a sequence of pkt-lines
> followed by a flush-pkt. The push option code forgot about this and sends push
> options and their terminating delimiter as ordinary pkt-lines that get their
> length header stripped off by remote-curl before being sent to the server.
>
> The result is multiple malformed requests, which the server rejects.
>
> Fortunately send-pack --stateless-rpc already is aware of this "pkt-line within
> pkt-line" framing for the update commands that precede push options. Handle
> push options the same way.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  send-pack.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)

This is only a hypothetical issue until the next patch though, right?

For what it's worth,

Tested-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH 0/2] push options across http

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> This series enables push options to be sent across http using remote-curl
>
> Thanks to Jonathan Nieder for helping troubleshoot.
>
> Brandon Williams (2):
>   send-pack: send push options correctly in stateless-rpc case
>   remote-curl: allow push options

Thanks.  Why does the topic name sound familiar to me?  Did we have
a recent attempt to do the same that didn't work well or something?

>
>  builtin/send-pack.c     |  5 +++++
>  remote-curl.c           |  8 ++++++++
>  send-pack.c             | 20 ++++++++------------
>  t/t5545-push-options.sh | 30 +++++++++++++++++++++++++++++-
>  4 files changed, 50 insertions(+), 13 deletions(-)

Re: [PATCH 0/2] push options across http

[Stefan Beller]

On Wed, Mar 22, 2017 at 2:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.  Why does the topic name sound familiar to me?  Did we have
> a recent attempt to do the same that didn't work well or something?

'sb/push-options-via-transport' sounds similar. Before that we silently
ignored push options in http, with that series we got an error indicating
http doesn't support push options. this series actually adds support for it.

Thanks,
Stefan

Re: [PATCH 2/2] remote-curl: allow push options

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Teach remote-curl to understand push options and to be able to convey
> them across HTTP.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---

An earlier 438fc684 ("push options: pass push options to the
transport helper", 2017-02-08) said:

    ...
    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.

and this is the step (2) to complete what it started?

>  builtin/send-pack.c     |  5 +++++
>  remote-curl.c           |  8 ++++++++
>  t/t5545-push-options.sh | 30 +++++++++++++++++++++++++++++-
>  3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 1ff5a6753..6796f3368 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  	int progress = -1;
>  	int from_stdin = 0;
>  	struct push_cas_option cas = {0};
> +	struct string_list push_options = STRING_LIST_INIT_DUP;
>  
>  	struct option options[] = {
>  		OPT__VERBOSITY(&verbose),
> @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "stateless-rpc", &stateless_rpc, N_("use stateless RPC protocol")),
>  		OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")),
>  		OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")),
> +		OPT_STRING_LIST('o', "push-option", &push_options,
> +				N_("server-specific"),
> +				N_("option to transmit")),
>  		{ OPTION_CALLBACK,
>  		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>  		  N_("require old value of ref to be at this value"),
> @@ -199,6 +203,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  	args.use_thin_pack = use_thin_pack;
>  	args.atomic = atomic;
>  	args.stateless_rpc = stateless_rpc;
> +	args.push_options = push_options.nr ? &push_options : NULL;
>  
>  	if (from_stdin) {
>  		struct argv_array all_refspecs = ARGV_ARRAY_INIT;
> diff --git a/remote-curl.c b/remote-curl.c
> index 34a97e732..e953d06f6 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -22,6 +22,7 @@ struct options {
>  	unsigned long depth;
>  	char *deepen_since;
>  	struct string_list deepen_not;
> +	struct string_list push_options;
>  	unsigned progress : 1,
>  		check_self_contained_and_connected : 1,
>  		cloning : 1,
> @@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)
>  		else
>  			return -1;
>  		return 0;
> +	} else if (!strcmp(name, "push-option")) {
> +		string_list_append(&options.push_options, value);
> +		return 0;
>  
>  #if LIBCURL_VERSION_NUM >= 0x070a08
>  	} else if (!strcmp(name, "family")) {
> @@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
>  		argv_array_push(&args, "--quiet");
>  	else if (options.verbosity > 1)
>  		argv_array_push(&args, "--verbose");
> +	for (i = 0; i < options.push_options.nr; i++)
> +		argv_array_pushf(&args, "--push-option=%s",
> +				 options.push_options.items[i].string);
>  	argv_array_push(&args, options.progress ? "--progress" : "--no-progress");
>  	for_each_string_list_item(cas_option, &cas_options)
>  		argv_array_push(&args, cas_option->string);
> @@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)
>  	options.progress = !!isatty(2);
>  	options.thin = 1;
>  	string_list_init(&options.deepen_not, 1);
> +	string_list_init(&options.push_options, 1);
>  
>  	remote = remote_get(argv[1]);
>  
> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
> index 9a57a7d8f..ac62083e9 100755
> --- a/t/t5545-push-options.sh
> +++ b/t/t5545-push-options.sh
> @@ -102,7 +102,9 @@ 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' '\
> +test_expect_success 'push option denied properly by http server' '
> +	test_when_finished "rm -rf test_http_clone" &&
> +	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
>  	mk_repo_pair &&
>  	git -C upstream config receive.advertisePushOptions false &&
>  	git -C upstream config http.receivepack true &&
> @@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http remote helper' '\
>  	git -C test_http_clone push origin master
>  '
>  
> +test_expect_success 'push options work properly across http' '
> +	test_when_finished "rm -rf test_http_clone" &&
> +	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
> +	mk_repo_pair &&
> +	git -C upstream config receive.advertisePushOptions true &&
> +	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 &&
> +	git -C test_http_clone push origin master &&
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
> +	git -C test_http_clone rev-parse --verify master >actual &&
> +	test_cmp expect actual &&
> +
> +	test_commit -C test_http_clone two &&
> +	git -C test_http_clone push --push-option=asdf --push-option="more structured text" origin master &&
> +	printf "asdf\nmore structured text\n" >expect &&
> +	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options &&
> +	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/post-receive.push_options &&
> +
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
> +	git -C test_http_clone rev-parse --verify master >actual &&
> +	test_cmp expect actual
> +'
> +
>  stop_httpd
>  
>  test_done

Re: [PATCH 2/2] remote-curl: allow push options

[Brandon Williams]

On 03/22, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > Teach remote-curl to understand push options and to be able to convey
> > them across HTTP.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> 
> An earlier 438fc684 ("push options: pass push options to the
> transport helper", 2017-02-08) said:
> 
>     ...
>     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.
> 
> and this is the step (2) to complete what it started?

Yep!  As far as I understand from looking at that commit, push options
were being passed to remote-curl and remote-curl was silently ignoring
them.  So that patch made it complain when a remote-helper didn't
support push options.

This patch teaches remote-curl to actually support push options, fixing
(2).

-- 
Brandon Williams

Re: [PATCH 2/2] remote-curl: allow push options

[Jonathan Nieder]

Brandon Williams wrote:

> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  	int progress = -1;
>  	int from_stdin = 0;
>  	struct push_cas_option cas = {0};
> +	struct string_list push_options = STRING_LIST_INIT_DUP;

It's safe for this to be NODUP, since the strings added to it live in
argv.

>  
>  	struct option options[] = {
>  		OPT__VERBOSITY(&verbose),
> @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "stateless-rpc", &stateless_rpc, N_("use stateless RPC protocol")),
>  		OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")),
>  		OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")),
> +		OPT_STRING_LIST('o', "push-option", &push_options,
> +				N_("server-specific"),
> +				N_("option to transmit")),

Does this need the short-and-sweet option name 'o'?  For this command,
I think I'd prefer giving it no short name.

Should this option be documented in the manpage, too?

[...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -22,6 +22,7 @@ struct options {
>  	unsigned long depth;
>  	char *deepen_since;
>  	struct string_list deepen_not;
> +	struct string_list push_options;
>  	unsigned progress : 1,
>  		check_self_contained_and_connected : 1,
>  		cloning : 1,
> @@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)
>  		else
>  			return -1;
>  		return 0;
> +	} else if (!strcmp(name, "push-option")) {
> +		string_list_append(&options.push_options, value);
> +		return 0;

push_options has strdup_strings enabled so this takes ownership of a
copy of value.  Good.

[...]
> --- a/t/t5545-push-options.sh
> +++ b/t/t5545-push-options.sh
> @@ -102,7 +102,9 @@ 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' '\
> +test_expect_success 'push option denied properly by http server' '

Should this test use test_i18ngrep to check that the error message
diagnoses the problem correctly instead of hitting an unrelated error
condition?

[...]
> @@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http remote helper' '\
>  	git -C test_http_clone push origin master
>  '
>  
> +test_expect_success 'push options work properly across http' '

Nice.

Tested-by: Jonathan Nieder <jrnieder@gmail.com>

With whatever subset of the following changes seems sensible to you
squashed in,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

diff --git i/Documentation/git-send-pack.txt w/Documentation/git-send-pack.txt
index a831dd0288..966abb0df8 100644
--- i/Documentation/git-send-pack.txt
+++ w/Documentation/git-send-pack.txt
@@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a flush packet.
 	will also fail if the actual call to `gpg --sign` fails.  See
 	linkgit:git-receive-pack[1] for the details on the receiving end.
 
+--push-option=<string>::
+	Pass the specified string as a push option for consumption by
+	hooks on the server side.  If the server doesn't support push
+	options, error out.  See linkgit:git-push[1] and
+	linkgit:githooks[5] for details.
+
 <host>::
 	A remote host to house the repository.  When this
 	part is specified, 'git-receive-pack' is invoked via
diff --git i/builtin/send-pack.c w/builtin/send-pack.c
index 6796f33687..832fd7ed0a 100644
--- i/builtin/send-pack.c
+++ w/builtin/send-pack.c
@@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	unsigned force_update = 0;
 	unsigned quiet = 0;
 	int push_cert = 0;
+	struct string_list push_options = STRING_LIST_INIT_NODUP;
 	unsigned use_thin_pack = 0;
 	unsigned atomic = 0;
 	unsigned stateless_rpc = 0;
@@ -152,7 +153,6 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int progress = -1;
 	int from_stdin = 0;
 	struct push_cas_option cas = {0};
-	struct string_list push_options = STRING_LIST_INIT_DUP;
 
 	struct option options[] = {
 		OPT__VERBOSITY(&verbose),
@@ -166,15 +166,15 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK,
 		  0, "signed", &push_cert, "yes|no|if-asked", N_("GPG sign the push"),
 		  PARSE_OPT_OPTARG, option_parse_push_signed },
+		OPT_STRING_LIST(0, "push-option", &push_options,
+				N_("server-specific"),
+				N_("option to transmit")),
 		OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
 		OPT_BOOL(0, "thin", &use_thin_pack, N_("use thin pack")),
 		OPT_BOOL(0, "atomic", &atomic, N_("request atomic transaction on remote side")),
 		OPT_BOOL(0, "stateless-rpc", &stateless_rpc, N_("use stateless RPC protocol")),
 		OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")),
 		OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")),
-		OPT_STRING_LIST('o', "push-option", &push_options,
-				N_("server-specific"),
-				N_("option to transmit")),
 		{ OPTION_CALLBACK,
 		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
 		  N_("require old value of ref to be at this value"),

Re: [PATCH 2/2] remote-curl: allow push options

[Brandon Williams]

On 03/22, Jonathan Nieder wrote:

I agree with most of these changes,  I'll make them locally and send out
a reroll.

> Brandon Williams wrote:
> 
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
> >  	int progress = -1;
> >  	int from_stdin = 0;
> >  	struct push_cas_option cas = {0};
> > +	struct string_list push_options = STRING_LIST_INIT_DUP;
> 
> It's safe for this to be NODUP, since the strings added to it live in
> argv.
> 
> >  
> >  	struct option options[] = {
> >  		OPT__VERBOSITY(&verbose),
> > @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
> >  		OPT_BOOL(0, "stateless-rpc", &stateless_rpc, N_("use stateless RPC protocol")),
> >  		OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")),
> >  		OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")),
> > +		OPT_STRING_LIST('o', "push-option", &push_options,
> > +				N_("server-specific"),
> > +				N_("option to transmit")),
> 
> Does this need the short-and-sweet option name 'o'?  For this command,
> I think I'd prefer giving it no short name.
> 
> Should this option be documented in the manpage, too?
> 
> [...]
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -22,6 +22,7 @@ struct options {
> >  	unsigned long depth;
> >  	char *deepen_since;
> >  	struct string_list deepen_not;
> > +	struct string_list push_options;
> >  	unsigned progress : 1,
> >  		check_self_contained_and_connected : 1,
> >  		cloning : 1,
> > @@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)
> >  		else
> >  			return -1;
> >  		return 0;
> > +	} else if (!strcmp(name, "push-option")) {
> > +		string_list_append(&options.push_options, value);
> > +		return 0;
> 
> push_options has strdup_strings enabled so this takes ownership of a
> copy of value.  Good.
> 
> [...]
> > --- a/t/t5545-push-options.sh
> > +++ b/t/t5545-push-options.sh
> > @@ -102,7 +102,9 @@ 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' '\
> > +test_expect_success 'push option denied properly by http server' '
> 
> Should this test use test_i18ngrep to check that the error message
> diagnoses the problem correctly instead of hitting an unrelated error
> condition?
> 
> [...]
> > @@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http remote helper' '\
> >  	git -C test_http_clone push origin master
> >  '
> >  
> > +test_expect_success 'push options work properly across http' '
> 
> Nice.
> 
> Tested-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> With whatever subset of the following changes seems sensible to you
> squashed in,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Thanks.
> 
> diff --git i/Documentation/git-send-pack.txt w/Documentation/git-send-pack.txt
> index a831dd0288..966abb0df8 100644
> --- i/Documentation/git-send-pack.txt
> +++ w/Documentation/git-send-pack.txt
> @@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a flush packet.
>  	will also fail if the actual call to `gpg --sign` fails.  See
>  	linkgit:git-receive-pack[1] for the details on the receiving end.
>  
> +--push-option=<string>::
> +	Pass the specified string as a push option for consumption by
> +	hooks on the server side.  If the server doesn't support push
> +	options, error out.  See linkgit:git-push[1] and
> +	linkgit:githooks[5] for details.
> +
>  <host>::
>  	A remote host to house the repository.  When this
>  	part is specified, 'git-receive-pack' is invoked via
> diff --git i/builtin/send-pack.c w/builtin/send-pack.c
> index 6796f33687..832fd7ed0a 100644
> --- i/builtin/send-pack.c
> +++ w/builtin/send-pack.c
> @@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  	unsigned force_update = 0;
>  	unsigned quiet = 0;
>  	int push_cert = 0;
> +	struct string_list push_options = STRING_LIST_INIT_NODUP;
>  	unsigned use_thin_pack = 0;
>  	unsigned atomic = 0;
>  	unsigned stateless_rpc = 0;
> @@ -152,7 +153,6 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  	int progress = -1;
>  	int from_stdin = 0;
>  	struct push_cas_option cas = {0};
> -	struct string_list push_options = STRING_LIST_INIT_DUP;
>  
>  	struct option options[] = {
>  		OPT__VERBOSITY(&verbose),
> @@ -166,15 +166,15 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  		{ OPTION_CALLBACK,
>  		  0, "signed", &push_cert, "yes|no|if-asked", N_("GPG sign the push"),
>  		  PARSE_OPT_OPTARG, option_parse_push_signed },
> +		OPT_STRING_LIST(0, "push-option", &push_options,
> +				N_("server-specific"),
> +				N_("option to transmit")),
>  		OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
>  		OPT_BOOL(0, "thin", &use_thin_pack, N_("use thin pack")),
>  		OPT_BOOL(0, "atomic", &atomic, N_("request atomic transaction on remote side")),
>  		OPT_BOOL(0, "stateless-rpc", &stateless_rpc, N_("use stateless RPC protocol")),
>  		OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")),
>  		OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")),
> -		OPT_STRING_LIST('o', "push-option", &push_options,
> -				N_("server-specific"),
> -				N_("option to transmit")),
>  		{ OPTION_CALLBACK,
>  		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>  		  N_("require old value of ref to be at this value"),

-- 
Brandon Williams

Re: [PATCH 1/2] send-pack: send push options correctly in stateless-rpc case

[Brandon Williams]

On 03/22, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> > "git send-pack --stateless-rpc" puts each request in a sequence of pkt-lines
> > followed by a flush-pkt. The push option code forgot about this and sends push
> > options and their terminating delimiter as ordinary pkt-lines that get their
> > length header stripped off by remote-curl before being sent to the server.
> >
> > The result is multiple malformed requests, which the server rejects.
> >
> > Fortunately send-pack --stateless-rpc already is aware of this "pkt-line within
> > pkt-line" framing for the update commands that precede push options. Handle
> > push options the same way.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  send-pack.c | 20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> This is only a hypothetical issue until the next patch though, right?

Correct, Its in preparation for the next patch.

> 
> For what it's worth,
> 
> Tested-by: Jonathan Nieder <jrnieder@gmail.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Thanks.

-- 
Brandon Williams

[PATCH v2 0/2] push options across http

[Brandon Williams]

v2 addresses Jonathan's comments from v1.
  * Fix a test
  * Add some documentation
  * remove short option from --push-option in git send-pack

Brandon Williams (2):
  send-pack: send push options correctly in stateless-rpc case
  remote-curl: allow push options

 Documentation/git-send-pack.txt |  6 ++++++
 builtin/send-pack.c             |  5 +++++
 remote-curl.c                   |  8 ++++++++
 send-pack.c                     | 20 ++++++++------------
 t/t5545-push-options.sh         | 33 +++++++++++++++++++++++++++++++--
 5 files changed, 58 insertions(+), 14 deletions(-)

-- 
2.12.1.500.gab5fba24ee-goog

[PATCH v2 1/2] send-pack: send push options correctly in stateless-rpc case

[Brandon Williams]

"git send-pack --stateless-rpc" puts each request in a sequence of pkt-lines
followed by a flush-pkt. The push option code forgot about this and sends push
options and their terminating delimiter as ordinary pkt-lines that get their
length header stripped off by remote-curl before being sent to the server.

The result is multiple malformed requests, which the server rejects.

Fortunately send-pack --stateless-rpc already is aware of this "pkt-line within
pkt-line" framing for the update commands that precede push options. Handle
push options the same way.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 send-pack.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index d2d2a49a0..66e652f7e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -532,6 +532,14 @@ int send_pack(struct send_pack_args *args,
 		}
 	}
 
+	if (use_push_options) {
+		struct string_list_item *item;
+
+		packet_buf_flush(&req_buf);
+		for_each_string_list_item(item, args->push_options)
+			packet_buf_write(&req_buf, "%s", item->string);
+	}
+
 	if (args->stateless_rpc) {
 		if (!args->dry_run && (cmds_sent || is_repository_shallow())) {
 			packet_buf_flush(&req_buf);
@@ -544,18 +552,6 @@ int send_pack(struct send_pack_args *args,
 	strbuf_release(&req_buf);
 	strbuf_release(&cap_buf);
 
-	if (use_push_options) {
-		struct string_list_item *item;
-		struct strbuf sb = STRBUF_INIT;
-
-		for_each_string_list_item(item, args->push_options)
-			packet_buf_write(&sb, "%s", item->string);
-
-		write_or_die(out, sb.buf, sb.len);
-		packet_flush(out);
-		strbuf_release(&sb);
-	}
-
 	if (use_sideband && cmds_sent) {
 		memset(&demux, 0, sizeof(demux));
 		demux.proc = sideband_demux;
-- 
2.12.1.500.gab5fba24ee-goog

[PATCH v2 2/2] remote-curl: allow push options

[Brandon Williams]

Teach remote-curl to understand push options and to be able to convey
them across HTTP.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-send-pack.txt |  6 ++++++
 builtin/send-pack.c             |  5 +++++
 remote-curl.c                   |  8 ++++++++
 t/t5545-push-options.sh         | 33 +++++++++++++++++++++++++++++++--
 4 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index a831dd028..966abb0df 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a flush packet.
 	will also fail if the actual call to `gpg --sign` fails.  See
 	linkgit:git-receive-pack[1] for the details on the receiving end.
 
+--push-option=<string>::
+	Pass the specified string as a push option for consumption by
+	hooks on the server side.  If the server doesn't support push
+	options, error out.  See linkgit:git-push[1] and
+	linkgit:githooks[5] for details.
+
 <host>::
 	A remote host to house the repository.  When this
 	part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 1ff5a6753..832fd7ed0 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	unsigned force_update = 0;
 	unsigned quiet = 0;
 	int push_cert = 0;
+	struct string_list push_options = STRING_LIST_INIT_NODUP;
 	unsigned use_thin_pack = 0;
 	unsigned atomic = 0;
 	unsigned stateless_rpc = 0;
@@ -165,6 +166,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK,
 		  0, "signed", &push_cert, "yes|no|if-asked", N_("GPG sign the push"),
 		  PARSE_OPT_OPTARG, option_parse_push_signed },
+		OPT_STRING_LIST(0, "push-option", &push_options,
+				N_("server-specific"),
+				N_("option to transmit")),
 		OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
 		OPT_BOOL(0, "thin", &use_thin_pack, N_("use thin pack")),
 		OPT_BOOL(0, "atomic", &atomic, N_("request atomic transaction on remote side")),
@@ -199,6 +203,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	args.use_thin_pack = use_thin_pack;
 	args.atomic = atomic;
 	args.stateless_rpc = stateless_rpc;
+	args.push_options = push_options.nr ? &push_options : NULL;
 
 	if (from_stdin) {
 		struct argv_array all_refspecs = ARGV_ARRAY_INIT;
diff --git a/remote-curl.c b/remote-curl.c
index 34a97e732..e953d06f6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -22,6 +22,7 @@ struct options {
 	unsigned long depth;
 	char *deepen_since;
 	struct string_list deepen_not;
+	struct string_list push_options;
 	unsigned progress : 1,
 		check_self_contained_and_connected : 1,
 		cloning : 1,
@@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)
 		else
 			return -1;
 		return 0;
+	} else if (!strcmp(name, "push-option")) {
+		string_list_append(&options.push_options, value);
+		return 0;
 
 #if LIBCURL_VERSION_NUM >= 0x070a08
 	} else if (!strcmp(name, "family")) {
@@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 		argv_array_push(&args, "--quiet");
 	else if (options.verbosity > 1)
 		argv_array_push(&args, "--verbose");
+	for (i = 0; i < options.push_options.nr; i++)
+		argv_array_pushf(&args, "--push-option=%s",
+				 options.push_options.items[i].string);
 	argv_array_push(&args, options.progress ? "--progress" : "--no-progress");
 	for_each_string_list_item(cas_option, &cas_options)
 		argv_array_push(&args, cas_option->string);
@@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)
 	options.progress = !!isatty(2);
 	options.thin = 1;
 	string_list_init(&options.deepen_not, 1);
+	string_list_init(&options.push_options, 1);
 
 	remote = remote_get(argv[1]);
 
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 9a57a7d8f..97065e62b 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -102,17 +102,46 @@ 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' '\
+test_expect_success 'push option denied properly by http server' '
+	test_when_finished "rm -rf test_http_clone" &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
 	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 &&
+	test_must_fail git -C test_http_clone push --push-option=asdf origin master 2>actual &&
+	test_i18ngrep "the receiving end does not support push options" actual &&
 	git -C test_http_clone push origin master
 '
 
+test_expect_success 'push options work properly across http' '
+	test_when_finished "rm -rf test_http_clone" &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	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 &&
+	git -C test_http_clone push origin master &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
+	git -C test_http_clone rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	test_commit -C test_http_clone two &&
+	git -C test_http_clone push --push-option=asdf --push-option="more structured text" origin master &&
+	printf "asdf\nmore structured text\n" >expect &&
+	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options &&
+	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/post-receive.push_options &&
+
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
+	git -C test_http_clone rev-parse --verify master >actual &&
+	test_cmp expect actual
+'
+
 stop_httpd
 
 test_done
-- 
2.12.1.500.gab5fba24ee-goog

Re: [PATCH v2 0/2] push options across http

[Jonathan Nieder]

Brandon Williams wrote:

> v2 addresses Jonathan's comments from v1.
>   * Fix a test
>   * Add some documentation
>   * remove short option from --push-option in git send-pack
> 
> Brandon Williams (2):
>   send-pack: send push options correctly in stateless-rpc case
>   remote-curl: allow push options
> 
>  Documentation/git-send-pack.txt |  6 ++++++
>  builtin/send-pack.c             |  5 +++++
>  remote-curl.c                   |  8 ++++++++
>  send-pack.c                     | 20 ++++++++------------
>  t/t5545-push-options.sh         | 33 +++++++++++++++++++++++++++++++--
>  5 files changed, 58 insertions(+), 14 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Tested-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.