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

[Stefan Beller <sbeller@xxxxxxxxxx>]

On Thu, Oct 19, 2017 at 10:47 AM, Marius Paliga <marius.paliga@xxxxxxxxx> wrote:
> 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!


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


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