Re: Enhancement request: git-push: Allow (configurable) default push-option

[Stefan Beller <sbeller@google.com>]

On Thu, Oct 12, 2017 at 9:32 AM, Marius Paliga <marius.paliga@gmail.com> wrote:
> Subject: [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.

Now that I review this patch, this is nice information and can
remain in the commit message, but it would be more useful
in the Documentation as that is where the users look.
(Another thing regarding the documentation: Maybe we want
to update Documentation/config.txt as well, that contains all
configuration)

> @@ -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, 0);

Junio,
do we have variables that behave similarly to this?

(I just wondered if the `v == NULL` could be lumped in
to here, resetting the string list)

>
> +test_expect_success 'default push option' '
...
> +'
> +
> +test_expect_success 'two default push options' '
...
> +'
> +
> +test_expect_success 'default and manual push options' '
...
> +'

Thanks for adding thorough tests!
Do we also need tests for the reset of the list?

Thanks,
Stefan