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

From: Stefan Beller <sbeller@google.com>
Date: Oct 12, 2017, 16:51 UTC
Message-ID: <CAGZ79kZEQ9WeJdTu83PxC7KSfvQAE3aX4D0M-vRth8P5O2TrVA@mail.gmail.com>
In-Reply-To: <CAK7vU=0A_nf2bAgV6dQyiesLJ3HHs5guyfNSNTwzYzgS2+YeWg@mail.gmail.com>

On Thu, Oct 12, 2017 at 9:32 AM, Marius Paliga <marius.paliga@gmail.com> wrote:

Show 16 quoted lines

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

Show 11 quoted lines

> @@ -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