Re: [PATCH 1/2] config: return positive from git_config_parse_key()

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Glen Choo <chooglen@google.com>
>
> 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.

>
> 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.  It is perfectly fine and expected for
the status values of the entire process is more coarse grained than
individual library calls, the latter may convey not just "I failed"
but "I failed why" to their callers, while the former may not want
to say "I made a call to some library functions and got this error
code", let alone "I called library function X and got error code Y".

In other words, if your program does

	err = library_call_about_some_filesystem_operation();
	if (err)
		exit(err); /* or exit(-err) */
	err = library_call_about_some_database_operation();
	if (err)
		exit(err); /* or exit(-err) */
	err = library_call_about_some_parsing();
	if (err)
		exit(err); /* or exit(-err) */

there is something wrong.  The error codes from these different
library functions share the same "integer" namespace without being
segregated, and expecting the calling process to be able to tell
what error we discovered by relaying the literal translation of low
level library error code would not work.  The exit() codes would
need to be wider (i.e. not limited only to the possible errors from
a single library function) and would be coarser (i.e. a filesystem
operation may say "open failed due to permission error" or "open
failed because there was no such file"; at the end-user level, it
may be more appropriate to say "configuration file could not be
read", regardless of the reason why the filesystem operation
failed).

	err = library_call_about_some_filesystem_operation();
	if (err) {
		error("cannot open the filesystem entity");
		exit(ERR_FILESYSTEM);
	err = library_call_about_some_database_operation();
	if (err)
		exit(ERR_DATABASE);
	err = library_call_about_some_parsing();
	if (err)
		exit(ERR_PARSING);

So, this is not a good reason, either.

> 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".

Thanks.