Re: [PATCH] builtin/push.c: add push.pushOption config

From: Stefan Beller <sbeller@google.com>
Date: Oct 19, 2017, 19:46 UTC
Message-ID: <CAGZ79kaSU+w0=zb61=5pEzhtd4U5Hzae4C2bUgpchNHAL_mzMA@mail.gmail.com>
In-Reply-To: <20171019174715.6577-1-marius.paliga@gmail.com>

On Thu, Oct 19, 2017 at 10:47 AM, Marius Paliga <marius.paliga@gmail.com> wrote:

Show 10 quoted lines

> 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.
>
> Add tests and update documentation as well.

Thanks for keeping working on this feature!

Show 7 quoted lines

> @@ -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.
> +       When no `--push-option <option>` is given from the command
> +       line, the values of configuration variable `push.pushOption`
> +       are used instead.

We'd also want to document how push.pushOption works in Documentation/config.txt (that contains all the configs)

So in the config, we have to explicitly give an empty option to clear the previous options, but on the command line we do not need that, but instead we'd have to repeat any push options that we desire that were configured?

Example:

  /etc/gitconfig
  push.pushoption = a
  push.pushoption = b

  ~/.gitconfig
  push.pushoption = c

  repo/.git/config
  push.pushoption =
  push.pushoption = b

will result in only b as a and c are cleared.

If I were to run
  git -c push.pushOption=d push ... (in repo)
it would be b and d, but
  git push --push-option=d
would be d only?

As a user I might have expected it to be the same,
expecting
  git push --push-option='' --push-option=d
to suppress b.

Show 6 quoted lines

> @@ -584,12 +599,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)

We have to do the same for _cmdline here, too?

The tests look good!

Thanks, Stefan