[PATCH v2] push: introduce --push-option-if-able

[PATCH v2] push: introduce --push-option-if-able

[Drew DeVault]

This introduces a --push-option-if-able, and along with it updates
send-pack, transport, push, etc to track the list of push options
specified via this flag. These options will be used if the remote
supports push options, but will not cause the push operation to
terminate if the remote does not support push options.

This is desirable in the following scenario: you frequently use two git
hosts, A and B, of which only B supports push options. If you wish to
set a push option globally (via git config push.pushOptions), any
attempts to push to host A will fail, requiring you to explicitly
override it at the command line. This renders the push.pushOption
config value basically useless for a lot of users.

Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
Previous version of this patch made --push-option non-fatal in the face
of a server which does not support push options. Following feedback that
this might be risky when the push options are relied upon to prevent
some undesirable default behavior from occuring, I've implemented the
suggested --push-option-if-able as an alternative. Thanks to Jeff King
for the review.

It was also suggested to add remote.*.pushOption{,IfAble}, but seeing as
remote.*.pushOption is not presently supported I think this is best
saved for a later patch (it's definitely a good idea, though).

 Documentation/config/push.txt      |  6 +++++
 Documentation/git-push.txt         | 14 +++++++++++-
 Documentation/git-receive-pack.txt | 10 +++++++++
 Documentation/githooks.txt         |  3 ++-
 builtin/push.c                     | 35 +++++++++++++++++++++++++-----
 send-pack.c                        |  9 ++++++--
 send-pack.h                        |  2 +-
 submodule.c                        | 11 +++++++++-
 submodule.h                        |  1 +
 transport-helper.c                 |  3 +++
 transport.c                        |  2 ++
 transport.h                        |  5 +++++
 12 files changed, 90 insertions(+), 11 deletions(-)

diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index 54871f8213..6ad0c6808a 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -99,6 +99,12 @@ This will result in only b (a and c are cleared).
 
 --
 
+push.pushOptionIfAble::
+	When no `--push-option-if-able=<option>` argument is given
+	from the command line, `git push` behaves as if each <value>
+	of this variable is given as `--push-option-if-able=<value>`.
+	In other respects, it is equivalent to push.pushOption.
+
 push.recurseSubmodules::
 	Make sure all submodule commits used by the revisions to be pushed
 	are available on a remote-tracking branch. If the value is 'check'
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3b8053447e..5f8504b05e 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
 	   [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-v | --verbose]
-	   [-u | --set-upstream] [-o <string> | --push-option=<string>]
+	   [-u | --set-upstream][-o <string> | --push-option=<string>]
+	   [--push-option-if-able=<string>]
 	   [--[no-]signed|--signed=(true|false|if-asked)]
 	   [--force-with-lease[=<refname>[:<expect>]]]
 	   [--no-verify] [<repository> [<refspec>...]]
@@ -224,6 +225,17 @@ already exists on the remote side.
 	line, the values of configuration variable `push.pushOption`
 	are used instead.
 
+--push-option-if-able=<option>::
+	Identical to --push-option, but does not terminate the push if the
+	remote does not support push options. This is useful, for example,
+	if you wish to globally enable a push option for use on a specific
+	git host, but also occasionally push to hosts which do not have
+	push options enabled. If you were to use --push-option instead,
+	pushing to the latter would cause the push to be aborted.
+	When no `--push-option-if-able=<option>` is given from the command
+	line, the values of configuration variable `push.pushOptionIfAble`
+	are used instead.
+
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
 	Path to the 'git-receive-pack' program on the remote
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 25702ed730..992ef5784f 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -109,6 +109,16 @@ the following environment variables:
 This hook is called before any refname is updated and before any
 fast-forward checks are performed.
 
+The number of push options given on the command line of
+`git push --push-option=...` can be read from the environment
+variable `GIT_PUSH_OPTION_COUNT`, and the options themselves are
+found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...
+If it is negotiated to not use the push options phase, the
+environment variables will not be set. If the client selects
+to use push options, but doesn't transmit any, the count variable
+will be set to zero, `GIT_PUSH_OPTION_COUNT=0`. In order for to receive push
+options, `receive.advertisePushOptions` must be enabled on the server.
+
 If the pre-receive hook exits with a non-zero exit status no updates
 will be performed, and the update, post-receive and post-update
 hooks will not be invoked either.  This can be useful to quickly
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 3dccab5375..48103116fd 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -283,7 +283,8 @@ found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...
 If it is negotiated to not use the push options phase, the
 environment variables will not be set. If the client selects
 to use push options, but doesn't transmit any, the count variable
-will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
+will be set to zero, `GIT_PUSH_OPTION_COUNT=0`. In order for to receive push
+options, `receive.advertisePushOptions` must be enabled on the server.
 
 See the section on "Quarantine Environment" in
 linkgit:git-receive-pack[1] for some caveats.
diff --git a/builtin/push.c b/builtin/push.c
index 6dbf0f0bb7..ddffb3d2f0 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -60,6 +60,7 @@ static struct push_cas_option cas;
 static struct refspec rs = REFSPEC_INIT_PUSH;
 
 static struct string_list push_options_config = STRING_LIST_INIT_DUP;
+static struct string_list push_options_if_able_config = STRING_LIST_INIT_DUP;
 
 static const char *map_refspec(const char *ref,
 			       struct remote *remote, struct ref *local_refs)
@@ -389,6 +390,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
 
 static int do_push(const char *repo, int flags,
 		   const struct string_list *push_options,
+		   const struct string_list *push_options_if_able,
 		   struct remote *remote)
 {
 	int i, errs;
@@ -396,7 +398,7 @@ static int do_push(const char *repo, int flags,
 	int url_nr;
 	struct refspec *push_refspec = &rs;
 
-	if (push_options->nr)
+	if (push_options->nr || push_options_if_able->nr)
 		flags |= TRANSPORT_PUSH_OPTIONS;
 
 	if (!push_refspec->nr && !(flags & TRANSPORT_PUSH_ALL)) {
@@ -411,16 +413,20 @@ static int do_push(const char *repo, int flags,
 		for (i = 0; i < url_nr; i++) {
 			struct transport *transport =
 				transport_get(remote, url[i]);
-			if (flags & TRANSPORT_PUSH_OPTIONS)
+			if (flags & TRANSPORT_PUSH_OPTIONS) {
 				transport->push_options = push_options;
+				transport->push_options_if_able = push_options_if_able;
+			}
 			if (push_with_options(transport, push_refspec, flags))
 				errs++;
 		}
 	} else {
 		struct transport *transport =
 			transport_get(remote, NULL);
-		if (flags & TRANSPORT_PUSH_OPTIONS)
+		if (flags & TRANSPORT_PUSH_OPTIONS) {
 			transport->push_options = push_options;
+			transport->push_options_if_able = push_options_if_able;
+		}
 		if (push_with_options(transport, push_refspec, flags))
 			errs++;
 	}
@@ -510,6 +516,15 @@ static int git_push_config(const char *k, const char *v, void *cb)
 			else
 				string_list_append(&push_options_config, v);
 		return 0;
+	} else if (!strcmp(k, "push.pushoptionifable")) {
+		if (!v)
+			return config_error_nonbool(k);
+		else
+			if (!*v)
+				string_list_clear(&push_options_if_able_config, 0);
+			else
+				string_list_append(&push_options_if_able_config, v);
+		return 0;
 	} else if (!strcmp(k, "color.push")) {
 		push_use_color = git_config_colorbool(k, v);
 		return 0;
@@ -533,7 +548,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int rc;
 	const char *repo = NULL;	/* default repository */
 	struct string_list push_options_cmdline = STRING_LIST_INIT_DUP;
-	struct string_list *push_options;
+	struct string_list push_options_if_able_cmdline = STRING_LIST_INIT_DUP;
+	struct string_list *push_options, *push_options_if_able;
 	const struct string_list_item *item;
 	struct remote *remote;
 
@@ -571,6 +587,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		  PARSE_OPT_OPTARG, option_parse_push_signed },
 		OPT_BIT(0, "atomic", &flags, N_("request atomic transaction on remote side"), TRANSPORT_PUSH_ATOMIC),
 		OPT_STRING_LIST('o', "push-option", &push_options_cmdline, N_("server-specific"), N_("option to transmit")),
+		OPT_STRING_LIST(0, "push-option-if-able", &push_options_if_able_cmdline, N_("server-specific"), N_("option to transmit, if supported by remote")),
 		OPT_SET_INT('4', "ipv4", &family, N_("use IPv4 addresses only"),
 				TRANSPORT_FAMILY_IPV4),
 		OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
@@ -584,6 +601,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	push_options = (push_options_cmdline.nr
 		? &push_options_cmdline
 		: &push_options_config);
+	push_options_if_able = (push_options_if_able_cmdline.nr
+		? &push_options_if_able_cmdline
+		: &push_options_if_able_config);
 	set_push_cert_flags(&flags, push_cert);
 
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
@@ -641,10 +661,15 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	for_each_string_list_item(item, push_options)
 		if (strchr(item->string, '\n'))
 			die(_("push options must not have new line characters"));
+	for_each_string_list_item(item, push_options_if_able)
+		if (strchr(item->string, '\n'))
+			die(_("push options must not have new line characters"));
 
-	rc = do_push(repo, flags, push_options, remote);
+	rc = do_push(repo, flags, push_options, push_options_if_able, remote);
 	string_list_clear(&push_options_cmdline, 0);
 	string_list_clear(&push_options_config, 0);
+	string_list_clear(&push_options_if_able_config, 0);
+	string_list_clear(&push_options_if_able_cmdline, 0);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else
diff --git a/send-pack.c b/send-pack.c
index 0407841ae8..60705405f4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -438,10 +438,13 @@ int send_pack(struct send_pack_args *args,
 
 	use_atomic = atomic_supported && args->atomic;
 
-	if (args->push_options && !push_options_supported)
+	if (args->push_options && args->push_options->nr && !push_options_supported)
 		die(_("the receiving end does not support push options"));
+	if (args->push_options_if_able && args->push_options_if_able->nr && !push_options_supported)
+		warning(_("the receiving end does not support push options"));
 
-	use_push_options = push_options_supported && args->push_options;
+	use_push_options = push_options_supported
+		&& (args->push_options || args->push_options_if_able);
 
 	if (status_report)
 		strbuf_addstr(&cap_buf, " report-status");
@@ -536,6 +539,8 @@ int send_pack(struct send_pack_args *args,
 		packet_buf_flush(&req_buf);
 		for_each_string_list_item(item, args->push_options)
 			packet_buf_write(&req_buf, "%s", item->string);
+		for_each_string_list_item(item, args->push_options_if_able)
+			packet_buf_write(&req_buf, "%s", item->string);
 	}
 
 	if (args->stateless_rpc) {
diff --git a/send-pack.h b/send-pack.h
index e148fcd960..644968ba06 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -27,7 +27,7 @@ struct send_pack_args {
 		push_cert:2,
 		stateless_rpc:1,
 		atomic:1;
-	const struct string_list *push_options;
+	const struct string_list *push_options, *push_options_if_able;
 };
 
 struct option;
diff --git a/submodule.c b/submodule.c
index 31f391d7d2..2386a2ad56 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1050,6 +1050,7 @@ static int push_submodule(const char *path,
 			  const struct remote *remote,
 			  const struct refspec *rs,
 			  const struct string_list *push_options,
+			  const struct string_list *push_options_if_able,
 			  int dry_run)
 {
 	if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
@@ -1064,6 +1065,12 @@ static int push_submodule(const char *path,
 				argv_array_pushf(&cp.args, "--push-option=%s",
 						 item->string);
 		}
+		if (push_options_if_able && push_options_if_able->nr) {
+			const struct string_list_item *item;
+			for_each_string_list_item(item, push_options_if_able)
+				argv_array_pushf(&cp.args, "--push-option-if-able=%s",
+						 item->string);
+		}
 
 		if (remote->origin != REMOTE_UNCONFIGURED) {
 			int i;
@@ -1123,6 +1130,7 @@ int push_unpushed_submodules(struct repository *r,
 			     const struct remote *remote,
 			     const struct refspec *rs,
 			     const struct string_list *push_options,
+			     const struct string_list *push_options_if_able,
 			     int dry_run)
 {
 	int i, ret = 1;
@@ -1157,7 +1165,8 @@ int push_unpushed_submodules(struct repository *r,
 		const char *path = needs_pushing.items[i].string;
 		fprintf(stderr, _("Pushing submodule '%s'\n"), path);
 		if (!push_submodule(path, remote, rs,
-				    push_options, dry_run)) {
+				    push_options, push_options_if_able,
+				    dry_run)) {
 			fprintf(stderr, _("Unable to push submodule '%s'\n"), path);
 			ret = 0;
 		}
diff --git a/submodule.h b/submodule.h
index c81ec1a9b6..38047c7ef1 100644
--- a/submodule.h
+++ b/submodule.h
@@ -116,6 +116,7 @@ int push_unpushed_submodules(struct repository *r,
 			     const struct remote *remote,
 			     const struct refspec *rs,
 			     const struct string_list *push_options,
+			     const struct string_list *push_options_if_able,
 			     int dry_run);
 /*
  * Given a submodule path (as in the index), return the repository
diff --git a/transport-helper.c b/transport-helper.c
index 20a7185ec4..e0cf40ffa2 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -865,6 +865,9 @@ static void set_common_push_options(struct transport *transport,
 		for_each_string_list_item(item, transport->push_options)
 			if (set_helper_option(transport, "push-option", item->string) != 0)
 				die(_("helper %s does not support 'push-option'"), name);
+		for_each_string_list_item(item, transport->push_options_if_able)
+			if (set_helper_option(transport, "push-option-if-able", item->string) != 0)
+				die(_("helper %s does not support 'push-option-if-able'"), name);
 	}
 }
 
diff --git a/transport.c b/transport.c
index 1fdc7dac1a..47861a4e65 100644
--- a/transport.c
+++ b/transport.c
@@ -691,6 +691,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
 	args.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);
 	args.push_options = transport->push_options;
+	args.push_options_if_able = transport->push_options_if_able;
 	args.url = transport->url;
 
 	if (flags & TRANSPORT_PUSH_CERT_ALWAYS)
@@ -1195,6 +1196,7 @@ int transport_push(struct repository *r,
 						      transport->remote,
 						      rs,
 						      transport->push_options,
+						      transport->push_options_if_able,
 						      pretend)) {
 				oid_array_clear(&commits);
 				trace2_region_leave("transport_push", "push_submodules", r);
diff --git a/transport.h b/transport.h
index e0131daab9..1ceeaf43fe 100644
--- a/transport.h
+++ b/transport.h
@@ -91,6 +91,11 @@ struct transport {
 	 * on the remote side, if both sides support the push options capability.
 	 */
 	const struct string_list *push_options;
+	/*
+	 * Like push_options, but if the remote does not support push options,
+	 * the push continues regardless.
+	 */
+	const struct string_list *push_options_if_able;
 
 	/*
 	 * These strings will be passed to the remote side on each command
-- 
2.25.0

Re: [PATCH v2] push: introduce --push-option-if-able

[Jeff King]

On Tue, Feb 18, 2020 at 03:09:14PM -0500, Drew DeVault wrote:

> This introduces a --push-option-if-able, and along with it updates
> send-pack, transport, push, etc to track the list of push options
> specified via this flag. These options will be used if the remote
> supports push options, but will not cause the push operation to
> terminate if the remote does not support push options.
> 
> This is desirable in the following scenario: you frequently use two git
> hosts, A and B, of which only B supports push options. If you wish to
> set a push option globally (via git config push.pushOptions), any
> attempts to push to host A will fail, requiring you to explicitly
> override it at the command line. This renders the push.pushOption
> config value basically useless for a lot of users.

Unsurprisingly, this approach makes sense to me. :)

The implementation looks like the right direction, but I noticed a few
things I think are worth addressing:

>  Documentation/config/push.txt      |  6 +++++
>  Documentation/git-push.txt         | 14 +++++++++++-
>  Documentation/git-receive-pack.txt | 10 +++++++++
>  Documentation/githooks.txt         |  3 ++-
>  builtin/push.c                     | 35 +++++++++++++++++++++++++-----
>  send-pack.c                        |  9 ++++++--
>  send-pack.h                        |  2 +-
>  submodule.c                        | 11 +++++++++-
>  submodule.h                        |  1 +
>  transport-helper.c                 |  3 +++
>  transport.c                        |  2 ++
>  transport.h                        |  5 +++++

We'd probably want some test coverage of the new command-line and config
options. Looks like t/t5545-push-options.sh would be a good place to add
it, and you should be able to emulate some of the existing tests there.

> @@ -224,6 +225,17 @@ already exists on the remote side.
>  	line, the values of configuration variable `push.pushOption`
>  	are used instead.
>  
> +--push-option-if-able=<option>::
> +	Identical to --push-option, but does not terminate the push if the
> +	remote does not support push options. This is useful, for example,
> +	if you wish to globally enable a push option for use on a specific
> +	git host, but also occasionally push to hosts which do not have
> +	push options enabled. If you were to use --push-option instead,
> +	pushing to the latter would cause the push to be aborted.
> +	When no `--push-option-if-able=<option>` is given from the command
> +	line, the values of configuration variable `push.pushOptionIfAble`
> +	are used instead.

The discussion of the rationale looks good here.

How do the two command-line lists (and their config counterparts)
interact? E.g., if I do:

  git push --push-option-if-able=foo

we know that overrides push.pushOptionIfAble config. Does it / should it
override push.pushOption? Likewise, a blank config option resets the
list. Would:

  [push]
  pushOptionIfAble = foo
  pushOption =

send "foo" or not?

IMHO we should consider it from the user's point of view as a single
list of push options, some of which are annotated with "if able" and
some not (and the answer would be "yes" to both of my questions).

And then the implementation could reflect that by using a single list.
I think that simplifies things by not having to pass around both lists.
You should be able to use string_list's util field to store the flag
(either allocate and point to a struct for each option, or just cast 0/1
to a void pointer).

> diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
> index 25702ed730..992ef5784f 100644
> --- a/Documentation/git-receive-pack.txt
> +++ b/Documentation/git-receive-pack.txt
> @@ -109,6 +109,16 @@ the following environment variables:
>  This hook is called before any refname is updated and before any
>  fast-forward checks are performed.
>  
> +The number of push options given on the command line of
> +`git push --push-option=...` can be read from the environment
> +variable `GIT_PUSH_OPTION_COUNT`, and the options themselves are
> +found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...
> +If it is negotiated to not use the push options phase, the
> +environment variables will not be set. If the client selects
> +to use push options, but doesn't transmit any, the count variable
> +will be set to zero, `GIT_PUSH_OPTION_COUNT=0`. In order for to receive push
> +options, `receive.advertisePushOptions` must be enabled on the server.

I think this documentation change made more sense as a separate patch
(since the server side is not affected either way by the "if able"
feature).

It still has the "for to" typo (or am I misreading it?).

> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 3dccab5375..48103116fd 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -283,7 +283,8 @@ found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...
>  If it is negotiated to not use the push options phase, the
>  environment variables will not be set. If the client selects
>  to use push options, but doesn't transmit any, the count variable
> -will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
> +will be set to zero, `GIT_PUSH_OPTION_COUNT=0`. In order for to receive push
> +options, `receive.advertisePushOptions` must be enabled on the server.

Likewise here (including the typo ;) ).

-Peff