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

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

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.

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