Re: [PATCH] Added support for new configuration parameter push.pushOption
Junio C Hamano writes:
> 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);
Re: [PATCH] Added support for new configuration parameter push.pushOption
Marius Paliga 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 ". 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.
> 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 ` 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.
> 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);
> }
> @@ -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.
> 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 de
[PATCH] Added support for new configuration parameter push.pushOption
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
---
Documentation/git-push.txt | 3 ++
builtin/push.c | 12
t/t5545-push-options.sh| 77 ++
3 files changed, 92 insertions(+)
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=::
--exec=::
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 con
