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

On Thu, Oct 19, 2017 at 7:19 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

quoted

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

Perhaps.

quoted

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?

It is not wrong per-se to phrase it like so, but I think that is
making it unnecessarily confusing by conflating two things.  (1)
configured values are overridden from the command line, just like
any other --option/config.variable pair

because they are single value options (usually).

and (2) unlike usual single
value variables where "last one wins" rule is simple enough to
explain,, multi-value variables need a way to "forget everything we
said so far and start from scratch" syntax, especially when multiple
input files are involved.

ok, my view of how that should be done is clashing once again
with the projects established standards. Sorry for the noise.

quoted

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.

The above is correct, and it might be worth giving it as an example
in the doc, because not just "give an empty entry to clear what has
been accumulated so far" but a multi-valued option in general is a
rather rare thing.

quoted

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?

quoted

quoted

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

I do not think so.  The point of these lines that appear before this
loop:

        git_config(git_push_config, &flags);
        argc = parse_options(argc, argv, prefix, options, push_usage, 0);
+       push_options = (push_options_cmdline.nr
+               ? &push_options_cmdline
+               : &push_options_config);

is that the command line overrides configured values, just like any
other configuration.  Adding _cmdline variant here is doubly wrong
when command line options are given in that it (1) duplicates what
was obtained from the command line, and (2) does not clear the
configured values.

Oh, ok. Sorry for the noise once again.

Thanks,
Stefan