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

[Junio C Hamano <gitster@pobox.com>]

Marius Paliga <marius.paliga@gmail.com> writes:

> @@ -505,6 +509,12 @@ static int git_push_config(const char *k, const
> char *v, void *cb)
>          recurse_submodules = val;
>      }
>
> +    default_push_options = git_config_get_value_multi("push.optiondefault");
> +    if (default_push_options)
> +        for_each_string_list_item(item, default_push_options)
> +            if (!string_list_has_string(&push_options, item->string))
> +                string_list_append(&push_options, item->string);
> +
>      return git_default_config(k, v, NULL);
>  }

Sorry for not catching this earlier, but git_config_get_value* call
inside git_push_config() is just wrong.

There are two styles of configuration parsing.  The original (and
still perfectly valid) way is to call git_config() with a callback
function like git_push_config().  Under this style, the config files
are read from lower-priority to higher-priority ones, and the
callback function is called once for each entry found, with <key, value>
pair and the callback specific opaque data.  One way to add the
parsing of a new variable like push.optiondefault is to add

	if (!strcmp(k, "push.optiondefault") {
		... handle one "[push] optiondefault" entry here ...
		return 0;
	}

to the function.

An alternate way is to use git_config_get_* functions _outside_
callback of git_config().  This is a newer invention.  Your call to
git_config_get_value_multi() will scan all configuration files and
returns _all_  entries for the given variable at once.

When there is already a callback style parser, in general, it is
cleaner to simply piggy-back on it, instead of reading variables
independently using git_config_get_* functions.  When there isn't a
callback style parser, using either style is OK.  It also is OK to
switch to git_config_get_* altogether, rewriting the callback style
parser, but I do not think it is warranted in this case, which adds
just one variable.

In any case, with the above code, you'll end up calling the
git_config_get_* function and grabbing all the values for
push.optiondefault for each and every configuration variable
definition (count "git config -l | wc -l" to estimate how many times
it will be called).  Which is probably not what you wanted to do.

Also, watch out for how a configuration variable defined like below
is reported to either of the above two styles:

	[push]	optiondefault

 - To a git_config() callback function like git_push_config(), such
   an entry is called with k=="push.optiondefault", v==NULL.

 - git_config_get_value_multi() would return a string-list element
   with the string set to NULL to signal that one value is NULL
   (i.e. it is different from "[push] optiondefault = ").

I suspect that with your code, we'd hit

	if (strchr(item->string, '\n'))

and end up dereferencing NULL right there.

> @@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
> char *prefix)
>      int push_cert = -1;
>      int rc;
>      const char *repo = NULL;    /* default repository */
> -    struct string_list push_options = STRING_LIST_INIT_DUP;
>      const struct string_list_item *item;
>
>      struct option options[] = {

Also, I suspect that this code does not allow the command line
option to override the default set in the configuration file.
OPT_STRING_LIST() appends to the &push_options string list without
first clearing it, and you are pre-populating the list while reading
the configuration, so the values taken from the command line will
only add to them.

The right way to do this would probably be:

 - Do not muck with push_options in cmd_push().

 - Prepare another string list, push_options_from_config, that is
   file-scope global.

 - In git_push_config(), do not call get_multi; instead react to a
   call with k=="push.optionsdefault" and

   - reject if "v" is NULL, with "return config_error_nonbool(k);"

   - otherwise, append "v" to the "from-config" string list--do not
     attempt to dedup or sort.

   - if "v" is an empty string, clear the "from-config" list.

 - After parse_options() returns to cmd_push(), see if push_options
   is empty.  If it is, you did not get any command line option, so
   override it with what you collected in the "from-config" string
   list.  Otherwise, do not even look at "from-config" string list.

By the way, I really hate "push.optiondefault" as the variable
name.  The "default" part is obvious and there is no need to say it,
as the configuration variables are there to give the default to what
we would normally give from the command line.  Rather, you should
say for which option (there are many options "git push" takes) this
variable gives the default.  Perhaps "push.pushOption" is a much
better name; I am sure people can come up with even better ones,
though ;-)