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

Marius Paliga [off-list ref] 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 ;-)

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.

"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.

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);
     }

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.

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