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

[Brandon Williams <bmwill@google.com>]

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