On Thu 27-01-11 14:45:16, Junio C Hamano wrote: > Libor Pechacek <lpechacek@xxxxxxx> writes: > > Fixed the typo and return values from get_value and git_config_set_multivar. > > We have changed git_config_parse_key to return negative values on error, but > > forgot to negate the numbers when returning them as exit codes. > > Earlier get_value() returned: > > -1: when it saw an uncompilable regexp (either in key or value); > 0: when a value was available (under --all) or unique (without --all); > 1: when the requested variable is missing; and > 2: when the requested variable is multivalued under --all. Fixed one part with the last change and broke the other one. Thanks for catching it. The same return value for "invalid key" and "invalid regex" is OK for me. BTW is it OK to exit(-1);? The return value of get_value() gets propagated to the process exit status. At the same time shell uses values >128 to indicate that the process was terminated by a signal. [...] > When moving an existing code segment around like this, I would not mind to > see style fixes thrown in to the patch, as long as the moved code is small > enough. Perhaps like this: I've added the style fix into the patch. [...] > > +/* Auxiliary function to sanity-check and split the key into the section > > /* > * Style. We write our multi-line comments > * like this. > */ Fixed. > > +int git_config_parse_key(const char *key, char **store_key, int *baselen_) [...] > > Does it make sense for this function to be prepared to get called with > NULL in store_key like this (and in the remainder of your patch)? No, I wrote it unnecessarily generic. Removed the excess code. Thanks for pointing it out. [...] > > + test_must_fail git -c name=value config core.name > > ' > > Don't you want to make sure that your sanity check triggers in new tests? Added a few tests after getting familiar with the test suite. Libor -- Libor Pechacek SUSE L3 Team, Prague -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html