On Thu, Oct 19, 2017 at 7:19 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> writes: > >>> @@ -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. > >> 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. >> 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. > >> 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? > >>> @@ -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
