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@xxxxxxxxx>
>
> With whatever subset of the following changes seems sensible to you
> squashed in,
> Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
>
> 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