git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Mar 22, 2017, 21:38 UTC
Message-ID
<20170322213826.GC26108@aiede.mtv.corp.google.com>
In-Reply-To
<20170322195102.165314-3-bmwill@google.com>
Brandon Williams wrote:
Show 7 quoted lines
> --- 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.

Show 10 quoted lines
>  
>  	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?
[...]
Show 17 quoted lines
> --- 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.

[...]
Show 8 quoted lines
> --- 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?

[...]
Show 5 quoted lines
> @@ -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"),
Previous: Brandon WilliamsNext: Brandon Williams
Message 5 of 15 in “push options across http”
  1. 0/2 push options across httpBrandon Williams, Mar 22, 2017
  2. 2/2 remote-curl: allow push optionsBrandon Williams, Mar 22, 2017
  3. Junio C HamanoMar 22, 2017
  4. Brandon WilliamsMar 22, 2017
  5. Jonathan NiederMar 22, 2017
  6. Brandon WilliamsMar 22, 2017
  7. 1/2 send-pack: send push options correctly in stateless-rpc caseBrandon Williams, Mar 22, 2017
  8. Jonathan NiederMar 22, 2017
  9. Brandon WilliamsMar 22, 2017
  10. Junio C HamanoMar 22, 2017
  11. Stefan BellerMar 22, 2017
  12. 0/2 push options across httpBrandon Williams, Mar 22, 2017
  13. 1/2 send-pack: send push options correctly in stateless-rpc caseBrandon Williams, Mar 22, 2017
  14. 2/2 remote-curl: allow push optionsBrandon Williams, Mar 22, 2017
  15. Jonathan NiederMar 22, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.