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

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

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 18, 2017, 00:54 UTC
Message-ID
<xmqqvajdl1hn.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<CAK7vU=1LGTLrEkRYS=autkDHmJAeq9=qa5jfx=DE+acPLsCqTg@mail.gmail.com>
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(-)
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
Previous: Marius PaligaNext: Junio C Hamano
Message 2 of 14 in “Added support for new configuration parameter push.pushOption”
  1. Added support for new configuration parameter push.pushOptionMarius Paliga, Oct 17, 2017
  2. Junio C HamanoOct 18, 2017
  3. Junio C HamanoOct 18, 2017
  4. builtin/push.c: add push.pushOption configMarius Paliga, Oct 19, 2017
  5. Stefan BellerOct 19, 2017
  6. Junio C HamanoOct 20, 2017
  7. Junio C HamanoOct 20, 2017
  8. Junio C HamanoOct 20, 2017
  9. Marius PaligaOct 20, 2017
  10. Junio C HamanoOct 21, 2017
  11. Stefan BellerOct 20, 2017
  12. Junio C HamanoOct 21, 2017
  13. builtin/push.c: add push.pushOption configMarius Paliga, Oct 23, 2017
  14. Junio C HamanoOct 24, 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.