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

From: Glen Choo <chooglen@google.com>
Date: Jul 21, 2023, 16:12 UTC
Message-ID: <kl6ltttxz1jj.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq4jlx28c6.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

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

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.

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

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.

Show 9 quoted lines

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