threads / patch / 46983

patchAdded support for new configuration parameter push.pushOption

Subject: [PATCH] Added support for new configuration parameter push.pushOption

## tl;dr

14 messages between Oct 17, 2017 and Oct 24, 2017. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

Marius Paliga· Oct 17, 2017, 06:32 UTC · lore
builtin/push.c: add push.pushOption config

Currently push options need to be given explicitly, via the command line as "git push --push-option".

The UX of Git would be enhanced if push options could be configured instead of given each time on the command line.

Add the config option push.pushOption, which is a multi string option, containing push options that are sent by default.

When push options are set in the system wide config (/etc/gitconfig), they can be unset later in the more specific repository config by setting the string to the empty string.

Add tests and documentation as well.
Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
---
 Documentation/git-push.txt |  3 ++
 builtin/push.c             | 12 ++++++++
 t/t5545-push-options.sh    | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)
Show changes to 3 files +90 −0

Documentation/git-push.txt, builtin/push.c, t/t5545-push-options.sh

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..da9b17624 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
     Transmit the given string to the server, which passes them to
     the pre-receive as well as the post-receive hook. The given string
     must not contain a NUL or LF character.
+    Default push options can also be specified with configuration
+    variable `push.pushOption`. String(s) specified here will always
+    be passed to the server without need to specify it using `--push-option`

 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..10e520c8f 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;

+static struct string_list push_options_from_config = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
     refspec_nr++;
@@ -503,6 +505,14 @@ static int git_push_config(const char *k, const
char *v, void *cb)
         int val = git_config_bool(k, v) ?
             RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
         recurse_submodules = val;
+    } else if (!strcmp(k, "push.pushoption")) {
+        if (v == NULL)
+            return config_error_nonbool(k);
+        else
+            if (v[0] == '\0')
+                string_list_clear(&push_options_from_config, 0);
+            else
+                string_list_append(&push_options_from_config, v);
     }

     return git_default_config(k, v, NULL);
@@ -562,6 +572,8 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
     packet_trace_identity("push");
     git_config(git_push_config, &flags);
     argc = parse_options(argc, argv, prefix, options, push_usage, 0);
+    if (!push_options.nr)
+        push_options = push_options_from_config;
     set_push_cert_flags(&flags, push_cert);

     if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL |
TRANSPORT_PUSH_MIRROR))))
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 90a4b0d2f..463783789 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '
     test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '

+test_expect_success 'default push option' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default push up master
+    ) &&
+    test_refs master master &&
+    echo "default" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'two default push options' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default1 -c push.pushOption=default2
push up master
+    ) &&
+    test_refs master master &&
+    printf "default1\ndefault2\n" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'push option from command line overrides
from-config push option' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default push --push-option=manual up master
+    ) &&
+    test_refs master master &&
+    echo "manual" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'empty value of push.pushOption in config clears
the list' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default1 -c push.pushOption= -c
push.pushOption=default2 push up master
+    ) &&
+    test_refs master master &&
+    echo "default2" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'invalid push option in config' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        test_must_fail git -c push.pushOption push up master
+    ) &&
+    test_refs master HEAD@{1}
Junio C Hamano· Oct 18, 2017, 00:54 UTC · re: Marius Paliga · lore

Re: [PATCH] Added support for new configuration parameter push.pushOption

Marius Paliga <marius.paliga@gmail.com> writes:
> builtin/push.c: add push.pushOption config

This is a good title for this change; it would be perfect if it were on the Subject: header ;-)

Show 8 quoted lines
> Currently push options need to be given explicitly, via
> the command line as "git push --push-option".
>
> The UX of Git would be enhanced if push options could be
> configured instead of given each time on the command line.
>
> Add the config option push.pushOption, which is a multi
> string option, containing push options that are sent by default.

s/Currently p/P/ would be sufficient; we describe the status quo in present tense, and give an order to the codebase to "become like so" (or command a patch monkey to "make the codebase like so") by using imperative mood.

I think something shorter like this would be sufficient:
    Push options need to be given explicitly, via the command line as "git
    push --push-option <option>".  Add the config option push.pushOption,
    which is a multi-valued option, containing push options that are sent
    by default.
    When push options are set in the lower-priority configulation file
    (e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in
    the more specific repository config by the empty string.
Show 11 quoted lines
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 3e76e99f3..da9b17624 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -161,6 +161,9 @@ already exists on the remote side.
>      Transmit the given string to the server, which passes them to
>      the pre-receive as well as the post-receive hook. The given string
>      must not contain a NUL or LF character.
> +    Default push options can also be specified with configuration
> +    variable `push.pushOption`. String(s) specified here will always
> +    be passed to the server without need to specify it using `--push-option`
"will always be passed" is not a "Default".  
If we really need to say it, say something like
	When no `--push-option <option>` is given from the command
	line, the values of this configuration variable are used
	instead.

But that is what "Default" means, so dropping the second sentence altogether would be more concise and better.

Show 23 quoted lines
> diff --git a/builtin/push.c b/builtin/push.c
> index 2ac810422..10e520c8f 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -32,6 +32,8 @@ static const char **refspec;
>  static int refspec_nr;
>  static int refspec_alloc;
>
> +static struct string_list push_options_from_config = STRING_LIST_INIT_DUP;
> +
>  static void add_refspec(const char *ref)
>  {
>      refspec_nr++;
> @@ -503,6 +505,14 @@ static int git_push_config(const char *k, const
> char *v, void *cb)
>          int val = git_config_bool(k, v) ?
>              RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
>          recurse_submodules = val;
> +    } else if (!strcmp(k, "push.pushoption")) {
> +        if (v == NULL)
> +            return config_error_nonbool(k);
> +        else
> +            if (v[0] == '\0')
Make this "else if (!*v)" on a single line and dedent the remainder.
> +                string_list_clear(&push_options_from_config, 0);
> +            else
> +                string_list_append(&push_options_from_config, v);
>      }
Show 7 quoted lines
> @@ -562,6 +572,8 @@ int cmd_push(int argc, const char **argv, const
> char *prefix)
>      packet_trace_identity("push");
>      git_config(git_push_config, &flags);
>      argc = parse_options(argc, argv, prefix, options, push_usage, 0);
> +    if (!push_options.nr)
> +        push_options = push_options_from_config;

We encourage our developers to think twice before doing a structure assignment.

I think this is a bad idea, primarily because at the point where string_list_clear() is later called on &push_options, it is unclear how to release the resources held by push_options_from_config(). It also is bad to depend on the implementation detail that overwriting the structure do not leak push_options.item when push_options.nr is zero, but it is conceivable that STRING_LIST_INIT_* could later be modified to pre-allocate a small array, at which point this will become a memory leak.

Show 11 quoted lines
> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
> index 90a4b0d2f..463783789 100755
> --- a/t/t5545-push-options.sh
> +++ b/t/t5545-push-options.sh
> @@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '
> ...
> +        git -c push.pushOption=default1 -c push.pushOption=default2
> push up master
> ...
> +test_expect_success 'push option from command line overrides
> from-config push option' '

It appears that your MUA is expanding tabs to spaces and wrapping long lines in your patch? Please double check and make sure that will not happen.

I couldn't use your patch (because it was broken by your MUA) as a base to show an incremental improvement, but here is how I would do the "code" part.

 builtin/push.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
Show changes to builtin/push.c +18 −4
diff --git a/builtin/push.c b/builtin/push.c
index 03846e8379..89ef029c67 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
 
+static struct string_list push_options_config = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
 	refspec_nr++;
@@ -503,6 +505,13 @@ static int git_push_config(const char *k, const char *v, void *cb)
 		int val = git_config_bool(k, v) ?
 			RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
 		recurse_submodules = val;
+	} else if (!strcmp(k, "push.pushoption")) {
+		if (!v)
+			return config_error_nonbool(k);
+		else if (!*v)
+			string_list_clear(&push_options_config, 0);
+		else
+			string_list_append(&push_options_config, v);
 	}
 
 	return git_default_config(k, v, NULL);
@@ -515,7 +524,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int push_cert = -1;
 	int rc;
 	const char *repo = NULL;	/* default repository */
-	struct string_list push_options = STRING_LIST_INIT_DUP;
+	struct string_list push_options_cmdline = STRING_LIST_INIT_DUP;
+	struct string_list *push_options;
 	const struct string_list_item *item;
 
 	struct option options[] = {
@@ -562,6 +572,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	packet_trace_identity("push");
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
+	push_options = (push_options_cmdline.nr
+			? &push_options_cmdline
+			: &push_options_config);
 	set_push_cert_flags(&flags, push_cert);
 
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
@@ -584,12 +597,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		set_refspecs(argv + 1, argc - 1, repo);
 	}
 
-	for_each_string_list_item(item, &push_options)
+	for_each_string_list_item(item, push_options)
 		if (strchr(item->string, '\n'))
 			die(_("push options must not have new line characters"));
 
-	rc = do_push(repo, flags, &push_options);
-	string_list_clear(&push_options, 0);
+	rc = do_push(repo, flags, push_options);
+	string_list_clear(&push_options_cmdline, 0);
+	string_list_clear(&push_options_config, 0);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else
Junio C Hamano· Oct 18, 2017, 01:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] Added support for new configuration parameter push.pushOption

Junio C Hamano <gitster@pobox.com> writes:
Show 30 quoted lines
> base to show an incremental improvement, but here is how I would do
> the "code" part.
>
>  builtin/push.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 03846e8379..89ef029c67 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -32,6 +32,8 @@ static const char **refspec;
>  static int refspec_nr;
>  static int refspec_alloc;
>  
> +static struct string_list push_options_config = STRING_LIST_INIT_DUP;
> +
>  static void add_refspec(const char *ref)
>  {
>  	refspec_nr++;
> @@ -503,6 +505,13 @@ static int git_push_config(const char *k, const char *v, void *cb)
>  		int val = git_config_bool(k, v) ?
>  			RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
>  		recurse_submodules = val;
> +	} else if (!strcmp(k, "push.pushoption")) {
> +		if (!v)
> +			return config_error_nonbool(k);
> +		else if (!*v)
> +			string_list_clear(&push_options_config, 0);
> +		else
> +			string_list_append(&push_options_config, v);
Here needs to be
		return 0;

as there is no point in falling through to call git_default_config().

>  	}
>  
>  	return git_default_config(k, v, NULL);
Marius Paliga· Oct 19, 2017, 17:47 UTC · re: Junio C Hamano · lore

[PATCH] builtin/push.c: add push.pushOption config

Push options need to be given explicitly, via the command line as "git push --push-option <option>". Add the config option push.pushOption, which is a multi-valued option, containing push options that are sent by default.

When push options are set in the lower-priority configulation file (e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in the more specific repository config by the empty string.

Add tests and update documentation as well.
Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
---
 Documentation/git-push.txt |  3 ++
 builtin/push.c             | 26 +++++++++++++---
 t/t5545-push-options.sh    | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 5 deletions(-)
Show changes to 3 files +101 −5

Documentation/git-push.txt, builtin/push.c, t/t5545-push-options.sh

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..aa78057dc 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
 	Transmit the given string to the server, which passes them to
 	the pre-receive as well as the post-receive hook. The given string
 	must not contain a NUL or LF character.
+	When no `--push-option <option>` is given from the command
+	line, the values of configuration variable `push.pushOption`
+	are used instead.
 
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..1c28427d8 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
 
+static struct string_list push_options_config = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
 	refspec_nr++;
@@ -503,6 +505,15 @@ static int git_push_config(const char *k, const char *v, void *cb)
 		int val = git_config_bool(k, v) ?
 			RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
 		recurse_submodules = val;
+	} else if (!strcmp(k, "push.pushoption")) {
+		if (!v)
+			return config_error_nonbool(k);
+		else
+			if (!*v)
+				string_list_clear(&push_options_config, 0);
+			else
+				string_list_append(&push_options_config, v);
+		return 0;
 	}
 
 	return git_default_config(k, v, NULL);
@@ -515,7 +526,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int push_cert = -1;
 	int rc;
 	const char *repo = NULL;	/* default repository */
-	struct string_list push_options = STRING_LIST_INIT_DUP;
+	struct string_list push_options_cmdline = STRING_LIST_INIT_DUP;
+	struct string_list *push_options;
 	const struct string_list_item *item;
 
 	struct option options[] = {
@@ -551,7 +563,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		  0, "signed", &push_cert, "yes|no|if-asked", N_("GPG sign the push"),
 		  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, N_("server-specific"), N_("option to transmit")),
+		OPT_STRING_LIST('o', "push-option", &push_options_cmdline, N_("server-specific"), N_("option to transmit")),
 		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"),
@@ -562,6 +574,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	packet_trace_identity("push");
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
+	push_options = (push_options_cmdline.nr
+		? &push_options_cmdline
+		: &push_options_config);
 	set_push_cert_flags(&flags, push_cert);
 
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
@@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		set_refspecs(argv + 1, argc - 1, repo);
 	}
 
-	for_each_string_list_item(item, &push_options)
+	for_each_string_list_item(item, push_options)
 		if (strchr(item->string, '\n'))
 			die(_("push options must not have new line characters"));
 
-	rc = do_push(repo, flags, &push_options);
-	string_list_clear(&push_options, 0);
+	rc = do_push(repo, flags, push_options);
+	string_list_clear(&push_options_cmdline, 0);
+	string_list_clear(&push_options_config, 0);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 90a4b0d2f..463783789 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '
 	test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '
 
+test_expect_success 'default push option' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		git -c push.pushOption=default push up master
+	) &&
+	test_refs master master &&
+	echo "default" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'two default push options' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		git -c push.pushOption=default1 -c push.pushOption=default2 push up master
+	) &&
+	test_refs master master &&
+	printf "default1\ndefault2\n" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'push option from command line overrides from-config push option' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		git -c push.pushOption=default push --push-option=manual up master
+	) &&
+	test_refs master master &&
+	echo "manual" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'empty value of push.pushOption in config clears the list' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		git -c push.pushOption=default1 -c push.pushOption= -c push.pushOption=default2 push up master
+	) &&
+	test_refs master master &&
+	echo "default2" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'invalid push option in config' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		test_must_fail git -c push.pushOption push up master
+	) &&
+	test_refs master HEAD@{1}
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.14.1
Stefan Beller· Oct 19, 2017, 19:46 UTC · re: Marius Paliga · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

On Thu, Oct 19, 2017 at 10:47 AM, Marius Paliga <marius.paliga@gmail.com> wrote:
Show 10 quoted lines
> Push options need to be given explicitly, via the command line as "git
> push --push-option <option>".  Add the config option push.pushOption,
> which is a multi-valued option, containing push options that are sent
> by default.
>
> When push options are set in the lower-priority configulation file
> (e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in
> the more specific repository config by the empty string.
>
> Add tests and update documentation as well.
Thanks for keeping working on this feature!
Show 7 quoted lines
> @@ -161,6 +161,9 @@ already exists on the remote side.
>         Transmit the given string to the server, which passes them to
>         the pre-receive as well as the post-receive hook. The given string
>         must not contain a NUL or LF character.
> +       When no `--push-option <option>` is given from the command
> +       line, the values of configuration variable `push.pushOption`
> +       are used instead.

We'd also want to document how push.pushOption works in Documentation/config.txt (that contains all the configs)

So in the config, we have to explicitly give an empty option to clear the previous options, but on the command line we do not need that, but instead we'd have to repeat any push options that we desire that were configured?

Example:
  /etc/gitconfig
  push.pushoption = a
  push.pushoption = b
  ~/.gitconfig
  push.pushoption = c
  repo/.git/config
  push.pushoption =
  push.pushoption = b

will result in only b as a and c are cleared.

If I were to run
  git -c push.pushOption=d push ... (in repo)
it would be b and d, but
  git push --push-option=d
would be d only?
As a user I might have expected it to be the same,
expecting
  git push --push-option='' --push-option=d
to suppress b.
Show 6 quoted lines
> @@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>                 set_refspecs(argv + 1, argc - 1, repo);
>         }
>
> -       for_each_string_list_item(item, &push_options)
> +       for_each_string_list_item(item, push_options)
We have to do the same for _cmdline here, too?
The tests look good!

Thanks, Stefan

Junio C Hamano· Oct 20, 2017, 02:19 UTC · re: Stefan Beller · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
>> @@ -161,6 +161,9 @@ already exists on the remote side.
>>         Transmit the given string to the server, which passes them to
>>         the pre-receive as well as the post-receive hook. The given string
>>         must not contain a NUL or LF character.
>> +       When no `--push-option <option>` is given from the command
>> +       line, the values of configuration variable `push.pushOption`
>> +       are used instead.
>
> We'd also want to document how push.pushOption works in
> Documentation/config.txt (that contains all the configs)
Perhaps.
> So in the config, we have to explicitly give an empty option to
> clear the previous options, but on the command line we do not need
> that, but instead we'd have to repeat any push options that we desire
> that were configured?

It is not wrong per-se to phrase it like so, but I think that is making it unnecessarily confusing by conflating two things. (1) configured values are overridden from the command line, just like any other --option/config.variable pair and (2) unlike usual single value variables where "last one wins" rule is simple enough to explain,, multi-value variables need a way to "forget everything we said so far and start from scratch" syntax, especially when multiple input files are involved.

Show 15 quoted lines
> Example:
>
>   /etc/gitconfig
>   push.pushoption = a
>   push.pushoption = b
>
>   ~/.gitconfig
>   push.pushoption = c
>
>   repo/.git/config
>   push.pushoption =
>   push.pushoption = b
>
> will result in only b as a and c are
> cleared.

The above is correct, and it might be worth giving it as an example in the doc, because not just "give an empty entry to clear what has been accumulated so far" but a multi-valued option in general is a rather rare thing.

Show 5 quoted lines
> If I were to run
>   git -c push.pushOption=d push ... (in repo)
> it would be b and d, but
>   git push --push-option=d
> would be d only?
Show 8 quoted lines
>> @@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>>                 set_refspecs(argv + 1, argc - 1, repo);
>>         }
>>
>> -       for_each_string_list_item(item, &push_options)
>> +       for_each_string_list_item(item, push_options)
>
> We have to do the same for _cmdline here, too?

I do not think so. The point of these lines that appear before this loop:

 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
+	push_options = (push_options_cmdline.nr
+		? &push_options_cmdline
+		: &push_options_config);

is that the command line overrides configured values, just like any other configuration. Adding _cmdline variant here is doubly wrong when command line options are given in that it (1) duplicates what was obtained from the command line, and (2) does not clear the configured values.

Junio C Hamano· Oct 20, 2017, 06:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

Junio C Hamano <gitster@pobox.com> writes:
>> We'd also want to document how push.pushOption works in
>> Documentation/config.txt (that contains all the configs)
>
> Perhaps.

Here is my attempt. I have a feeling that the way http.extraheaders is described may be much easier to read and we may want to mimick its style. I dunno.

 Documentation/config.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)
Show changes to Documentation/config.txt +10 −0
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6adb..631ed1172e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2621,6 +2621,16 @@ push.gpgSign::
 	override a value from a lower-priority config file. An explicit
 	command-line flag always overrides this config option.
 
+push.pushOption::
+	When no `--push-option=<option>` argument is given from the
+	command line, `git push` behaves as if each <value> of
+	this variable is given as `--push-option=<value>`.
++
+This is a multi-valued variable, and an empty value can be used in a
+higher priority cofiguration file (e.g. `.git/config` in a
+repository) to clear the values inherited from a lower priority
+configuration files (e.g. `$HOME/.gitconfig`).
+
 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'
Junio C Hamano· Oct 20, 2017, 06:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> We'd also want to document how push.pushOption works in
>>> Documentation/config.txt (that contains all the configs)
>>
>> Perhaps.
>
> Here is my attempt.

Another thing I noticed while we are around the area is that unlike all other options in git-push.txt page, this one forgets to say it always takes mandatory string. Here is a possible fix.

 Documentation/git-push.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to Documentation/git-push.txt +3 −2
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index aa78057dc5..a8504e0ae3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -156,12 +156,12 @@ already exists on the remote side.
 	Either all refs are updated, or on error, no refs are updated.
 	If the server does not support atomic pushes the push will fail.
 
--o::
---push-option::
+-o <option>::
+--push-option=<option>::
 	Transmit the given string to the server, which passes them to
 	the pre-receive as well as the post-receive hook. The given string
 	must not contain a NUL or LF character.
-	When no `--push-option <option>` is given from the command
+	When no `--push-option=<option>` is given from the command
 	line, the values of configuration variable `push.pushOption`
 	are used instead.
Marius Paliga· Oct 20, 2017, 08:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

Show 11 quoted lines
> --o::
> ---push-option::
> +-o <option>::
> +--push-option=<option>::
>         Transmit the given string to the server, which passes them to
>         the pre-receive as well as the post-receive hook. The given string
>         must not contain a NUL or LF character.
> -       When no `--push-option <option>` is given from the command
> +       When no `--push-option=<option>` is given from the command
>         line, the values of configuration variable `push.pushOption`
>         are used instead.

Should we also mention that this option can take multiple values from the command line?

-o <option>::
--push-option=<option>::
        Transmit the given string to the server, which passes them to
        the pre-receive as well as the post-receive hook. The given string
        must not contain a NUL or LF character.
+       Multiple `--push-option=<option>` are allowed on the command line.
       When no `--push-option=<option>` is given from the command
        line, the values of configuration variable `push.pushOption`
        are used instead.
2017-10-20 8:18 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 38 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> We'd also want to document how push.pushOption works in
>>>> Documentation/config.txt (that contains all the configs)
>>>
>>> Perhaps.
>>
>> Here is my attempt.
>
> Another thing I noticed while we are around the area is that unlike
> all other options in git-push.txt page, this one forgets to say it
> always takes mandatory string.  Here is a possible fix.
>
>  Documentation/git-push.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index aa78057dc5..a8504e0ae3 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -156,12 +156,12 @@ already exists on the remote side.
>         Either all refs are updated, or on error, no refs are updated.
>         If the server does not support atomic pushes the push will fail.
>
> --o::
> ---push-option::
> +-o <option>::
> +--push-option=<option>::
>         Transmit the given string to the server, which passes them to
>         the pre-receive as well as the post-receive hook. The given string
>         must not contain a NUL or LF character.
> -       When no `--push-option <option>` is given from the command
> +       When no `--push-option=<option>` is given from the command
>         line, the values of configuration variable `push.pushOption`
>         are used instead.
>
Junio C Hamano· Oct 21, 2017, 01:33 UTC · re: Marius Paliga · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

Marius Paliga <marius.paliga@gmail.com> writes:
Show 12 quoted lines
> Should we also mention that this option can take multiple values from
> the command line?
>
> -o <option>::
> --push-option=<option>::
>         Transmit the given string to the server, which passes them to
>         the pre-receive as well as the post-receive hook. The given string
>         must not contain a NUL or LF character.
> +       Multiple `--push-option=<option>` are allowed on the command line.
>        When no `--push-option=<option>` is given from the command
>         line, the values of configuration variable `push.pushOption`
>         are used instead.

My first reaction was "Do we do that for other options that can be given multiple times? If not, perhaps this is not needed."

Then my second reaction was "Do we have that many options that can be given multiple times in the first place? If not, perhaps a change like this to highlight these oddball options may not be a bad idea."

And my third reaction was "Well, even if we have many such options, and even if most of them are not explicitly marked as usable multiple times in the current documentation, that's not a reason to keep it that way---the readers ought to be able to find out which ones can be used multiple times and how these multiple instances interact with each other, because the usual rule for single-instance options is 'the last one wins' (e.g. 'git status -uall -uno' should be the same as 'git status -uno') but to these multi-value options that rule does not hold".

So, yes, I think it is a good idea.

But it opens a tangent #leftoverbits. Among the most commonly used commands listed in "git --help", there indeed are some commands that take multiple options of the same kind (even if we do not count an obvious exception "--verbose", which everybody understands as the number of times it is given indicates the verbosity level). Somebody needs to go over their documentation and add "this can be given multiple times from the command line, and here is what happens to them".

In your suggestion for "push -o <value1> -o <value2>", "here is what happens" is missing. Perhaps

	When multiple `--push-option=<option>` are given, they are
	all sent to the other side in the order listed on the
	command line.
or something like that.
Thanks.
Stefan Beller· Oct 20, 2017, 19:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

On Thu, Oct 19, 2017 at 7:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>>> @@ -161,6 +161,9 @@ already exists on the remote side.
>>>         Transmit the given string to the server, which passes them to
>>>         the pre-receive as well as the post-receive hook. The given string
>>>         must not contain a NUL or LF character.
>>> +       When no `--push-option <option>` is given from the command
>>> +       line, the values of configuration variable `push.pushOption`
>>> +       are used instead.
>>
>> We'd also want to document how push.pushOption works in
>> Documentation/config.txt (that contains all the configs)
>
> Perhaps.
>
>> So in the config, we have to explicitly give an empty option to
>> clear the previous options, but on the command line we do not need
>> that, but instead we'd have to repeat any push options that we desire
>> that were configured?
>
> It is not wrong per-se to phrase it like so, but I think that is
> making it unnecessarily confusing by conflating two things.  (1)
> configured values are overridden from the command line, just like
> any other --option/config.variable pair
because they are single value options (usually).
Show 5 quoted lines
> and (2) unlike usual single
> value variables where "last one wins" rule is simple enough to
> explain,, multi-value variables need a way to "forget everything we
> said so far and start from scratch" syntax, especially when multiple
> input files are involved.

ok, my view of how that should be done is clashing once again with the projects established standards. Sorry for the noise.

Show 50 quoted lines
>> Example:
>>
>>   /etc/gitconfig
>>   push.pushoption = a
>>   push.pushoption = b
>>
>>   ~/.gitconfig
>>   push.pushoption = c
>>
>>   repo/.git/config
>>   push.pushoption =
>>   push.pushoption = b
>>
>> will result in only b as a and c are
>> cleared.
>
> The above is correct, and it might be worth giving it as an example
> in the doc, because not just "give an empty entry to clear what has
> been accumulated so far" but a multi-valued option in general is a
> rather rare thing.
>
>> If I were to run
>>   git -c push.pushOption=d push ... (in repo)
>> it would be b and d, but
>>   git push --push-option=d
>> would be d only?
>
>>> @@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>>>                 set_refspecs(argv + 1, argc - 1, repo);
>>>         }
>>>
>>> -       for_each_string_list_item(item, &push_options)
>>> +       for_each_string_list_item(item, push_options)
>>
>> We have to do the same for _cmdline here, too?
>
> I do not think so.  The point of these lines that appear before this
> loop:
>
>         git_config(git_push_config, &flags);
>         argc = parse_options(argc, argv, prefix, options, push_usage, 0);
> +       push_options = (push_options_cmdline.nr
> +               ? &push_options_cmdline
> +               : &push_options_config);
>
> is that the command line overrides configured values, just like any
> other configuration.  Adding _cmdline variant here is doubly wrong
> when command line options are given in that it (1) duplicates what
> was obtained from the command line, and (2) does not clear the
> configured values.
Oh, ok. Sorry for the noise once again.

Thanks, Stefan

Junio C Hamano· Oct 21, 2017, 01:52 UTC · re: Stefan Beller · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

Stefan Beller <sbeller@google.com> writes:
Show 20 quoted lines
>>> So in the config, we have to explicitly give an empty option to
>>> clear the previous options, but on the command line we do not need
>>> that, but instead we'd have to repeat any push options that we desire
>>> that were configured?
>>
>> It is not wrong per-se to phrase it like so, but I think that is
>> making it unnecessarily confusing by conflating two things.  (1)
>> configured values are overridden from the command line, just like
>> any other --option/config.variable pair
>
> because they are single value options (usually).
>
>> and (2) unlike usual single
>> value variables where "last one wins" rule is simple enough to
>> explain,, multi-value variables need a way to "forget everything we
>> said so far and start from scratch" syntax, especially when multiple
>> input files are involved.
>
> ok, my view of how that should be done is clashing once again
> with the projects established standards. Sorry for the noise.

I do not think it is a noise. I was primarily focusing on "have to explicitly" part, making it sound as if it was a flaw. I do think it is a good idea to explain a variable and/or an option is multi-valued and how multiple instances of them interact with each other. I think the status quo is:

	Both configuration and command line, these multi-valued
	things accumulate.  The "command line can be used to
	override things from the configuration" rule applies as any
	other config/option pair.
	In addition, in the configuration, there is a mechanism to
	clear the values read so far with the "an empty value
	clears" rule, because you may not have control over, or it
	may be cumbersome to modify, lower-priority configuration
	files.  There is no such thing for command line, as the
	entirety of the command line for each invocation is under
	your control.
If somebody has
	[alias] mypush = push -ofoo

then the command line argument for one invocation of "git mypush" may *not* be under your control and it might be also convenient if there were a mechanism to clear what has been accumulated from the command line. It may be worth considering, but if we were going in that direction, I suspect that it is probably a good idea to first step back a bit and introduce a mechanism to easily define pairs of option/config in a more sysmtematic way [*1*]. Once we have such a mechanism, the "clear the previous" syntax for the command line would be implemented uniformly in there.

[Footnote]
*1* E.g. right now, the fact that "push --push-option" and
    "push.pushOption" are related, or that "status -u<mode>" and
    "status.showUntrackedFiles" are related, is only known to the
    code and the documentation; I am not sure if it is even a good
    idea to add config to each and every option that exists, but for
    the ones that do exist, it would be nicer if there were a more
    uniform and systematic way for parse-options.c and config.c APIs
    to help our code, instead of writing git_config() callback and
    options[] array to give to parse_options(), making sure they
    refer to the same internal variable, and the latter overrides
    the former.
Marius Paliga· Oct 23, 2017, 11:44 UTC · re: Junio C Hamano · lore

[PATCH] builtin/push.c: add push.pushOption config

Push options need to be given explicitly, via the command line as "git push --push-option <option>". Add the config option push.pushOption, which is a multi-valued option, containing push options that are sent by default.

When push options are set in the lower-priority configulation file (e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in the more specific repository config by the empty string.

Add tests and update documentation as well.
Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
---
 Documentation/config.txt   | 29 +++++++++++++++++
 Documentation/git-push.txt | 10 ++++--
 builtin/push.c             | 26 +++++++++++++---
 t/t5545-push-options.sh    | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 135 insertions(+), 7 deletions(-)
Show changes to 4 files +135 −6

Documentation/config.txt, Documentation/git-push.txt, builtin/push.c, t/t5545-push-options.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6ad..0bd46a6ab 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2621,6 +2621,35 @@ push.gpgSign::
 	override a value from a lower-priority config file. An explicit
 	command-line flag always overrides this config option.
 
+push.pushOption::
+	When no `--push-option=<option>` argument is given from the
+	command line, `git push` behaves as if each <value> of
+	this variable is given as `--push-option=<value>`.
++
+This is a multi-valued variable, and an empty value can be used in a
+higher priority cofiguration file (e.g. `.git/config` in a
+repository) to clear the values inherited from a lower priority
+configuration files (e.g. `$HOME/.gitconfig`).
++
+--
+
+Example:
+
+/etc/gitconfig
+  push.pushoption = a
+  push.pushoption = b
+
+~/.gitconfig
+  push.pushoption = c
+
+repo/.git/config
+  push.pushoption =
+  push.pushoption = b
+
+This will result in only b (a and c are cleared).
+
+--
+
 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 3e76e99f3..5b08302fc 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -156,11 +156,17 @@ already exists on the remote side.
 	Either all refs are updated, or on error, no refs are updated.
 	If the server does not support atomic pushes the push will fail.
 
--o::
---push-option::
+-o <option>::
+--push-option=<option>::
 	Transmit the given string to the server, which passes them to
 	the pre-receive as well as the post-receive hook. The given string
 	must not contain a NUL or LF character.
+	When multiple `--push-option=<option>` are given, they are
+	all sent to the other side in the order listed on the
+	command line.
+	When no `--push-option=<option>` is given from the command
+	line, the values of configuration variable `push.pushOption`
+	are used instead.
 
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..1c28427d8 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
 
+static struct string_list push_options_config = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
 	refspec_nr++;
@@ -503,6 +505,15 @@ static int git_push_config(const char *k, const char *v, void *cb)
 		int val = git_config_bool(k, v) ?
 			RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
 		recurse_submodules = val;
+	} else if (!strcmp(k, "push.pushoption")) {
+		if (!v)
+			return config_error_nonbool(k);
+		else
+			if (!*v)
+				string_list_clear(&push_options_config, 0);
+			else
+				string_list_append(&push_options_config, v);
+		return 0;
 	}
 
 	return git_default_config(k, v, NULL);
@@ -515,7 +526,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int push_cert = -1;
 	int rc;
 	const char *repo = NULL;	/* default repository */
-	struct string_list push_options = STRING_LIST_INIT_DUP;
+	struct string_list push_options_cmdline = STRING_LIST_INIT_DUP;
+	struct string_list *push_options;
 	const struct string_list_item *item;
 
 	struct option options[] = {
@@ -551,7 +563,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		  0, "signed", &push_cert, "yes|no|if-asked", N_("GPG sign the push"),
 		  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, N_("server-specific"), N_("option to transmit")),
+		OPT_STRING_LIST('o', "push-option", &push_options_cmdline, N_("server-specific"), N_("option to transmit")),
 		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"),
@@ -562,6 +574,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	packet_trace_identity("push");
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
+	push_options = (push_options_cmdline.nr
+		? &push_options_cmdline
+		: &push_options_config);
 	set_push_cert_flags(&flags, push_cert);
 
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
@@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		set_refspecs(argv + 1, argc - 1, repo);
 	}
 
-	for_each_string_list_item(item, &push_options)
+	for_each_string_list_item(item, push_options)
 		if (strchr(item->string, '\n'))
 			die(_("push options must not have new line characters"));
 
-	rc = do_push(repo, flags, &push_options);
-	string_list_clear(&push_options, 0);
+	rc = do_push(repo, flags, push_options);
+	string_list_clear(&push_options_cmdline, 0);
+	string_list_clear(&push_options_config, 0);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 90a4b0d2f..463783789 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '
 	test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '
 
+test_expect_success 'default push option' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		git -c push.pushOption=default push up master
+	) &&
+	test_refs master master &&
+	echo "default" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'two default push options' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		git -c push.pushOption=default1 -c push.pushOption=default2 push up master
+	) &&
+	test_refs master master &&
+	printf "default1\ndefault2\n" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'push option from command line overrides from-config push option' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		git -c push.pushOption=default push --push-option=manual up master
+	) &&
+	test_refs master master &&
+	echo "manual" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'empty value of push.pushOption in config clears the list' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		git -c push.pushOption=default1 -c push.pushOption= -c push.pushOption=default2 push up master
+	) &&
+	test_refs master master &&
+	echo "default2" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'invalid push option in config' '
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git push --mirror up &&
+		test_commit two &&
+		test_must_fail git -c push.pushOption push up master
+	) &&
+	test_refs master HEAD@{1}
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.14.1
Junio C Hamano· Oct 24, 2017, 00:59 UTC · re: Marius Paliga · lore

Re: [PATCH] builtin/push.c: add push.pushOption config

Marius Paliga <marius.paliga@gmail.com> writes:
Show 13 quoted lines
> Push options need to be given explicitly, via the command line as "git
> push --push-option <option>".  Add the config option push.pushOption,
> which is a multi-valued option, containing push options that are sent
> by default.
>
> When push options are set in the lower-priority configulation file
> (e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in
> the more specific repository config by the empty string.
>
> Add tests and update documentation as well.
>
> Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
> ---
Looks good.
> +This is a multi-valued variable, and an empty value can be used in a
> +higher priority cofiguration file (e.g. `.git/config` in a
s/cofig/config/; I think you inherited from me, sorry.

I'll tweak it while queuing; there is not need to resend only to fix this.

Thanks.

← back to recent threads