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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 21, 2023, 04:32 UTC
Message-ID: <xmqq4jlx28c6.fsf@gitster.g>
In-Reply-To: <99244816307b822bd8ffcbff8690ef449c797a23.1689891436.git.gitgitgadget@gmail.com>

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

Show 8 quoted lines

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

Show 5 quoted lines

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