Junio C Hamano <gitster@xxxxxxxxx> writes: >> From: Glen Choo <chooglen@xxxxxxxxxx> >> >> git_config_parse_key() returns #define-d error codes, but negated. This >> negation is merely a convenience to other parts of config.c that don't >> bother inspecting the return value before passing it along. But: >> >> a) There's no good reason why those callers couldn't negate the value >> themselves. > > That is not a good reason to break from a widely adopted convention > in UNIXy library functions to signal success with 0 and failure with > negative values. The callers if they want to have a positive values > can flip the polarity themselves, by the way. Oh, interesting. I was trying to follow the conventions of the surrounding config.c code and many other parts of the codebase, which returns positive values. Why do we choose to return postive values throughout the codebase, by the way? Is it because they were really intended for exit(3), and not to be used as a library. >> >> b) In other callers, this value eventually gets fed to exit(3), and >> those callers need to sanitize the negative value (and they sometimes >> do so lossily, by overriding the return value with >> CONFIG_INVALID_KEY). > > There is no reason to think that each and every minute difference > the direct callers of a library function may want to notice by > different error return values needs to be propagated to the calling > process via its exit value. Yes, I fully agree. I didn't intend to be a statement on how things should be, but rather how they already are. The oddities in this case are: - No callers actually care about the sign of git_config_parse_key() since it can only return values of one sign. Only configset_find_element() benefits from the sign being negative (since it returns negative on config key errors), but instead of putting the burden on the function it depends on, it could just return the negative value itself. And this "burden" is real, in that other callers have to worry about the negative value. - For better or worse, we've already wired git_config_parse_key() directly to the exit values, e.g. if one peeks into builtin/config.c:cmd_config(), we'll see "return git_config_set_multivar_in_file_gently()", which in turn may return the value from git_config_parse_key(). (And as a result, we also try hard to separate the error values returnable by git_config_parse_key() vs the others returnable by git_config_set_multivar_in_file_gently().) I would strongly prefer if builtin/config.c took more responsibility for noticing config.c errors and sanitizing them accordingly, but it seemed like too much churn for this series. If you think it is the right time for it, though (which I think it might be), I could try to make that change. >> c) We want to move that into a separate library, and returning only >> negative values no longer makes as much sense. > > Quite the contrary, if it were a purely internal convention, we may > not care too much, as we have only ourselves to confuse by picking > an unusual convention, but if we are making more parts of our code > available in a library-ish interface, I would expect they follow > some convention, and that convention would be "0 for success, > negative for failure". Right. I do not care what the convention is, only that we pick one. I chose the one that I saw in the surrounding code (positive), but I'm happy to go with UNIXy (negative) if others think it is worth the churn.
