Re: [PATCH 1/2] docs: add a basic description of the config API

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2012, 06:40 UTC
Message-ID: <7vmx8tzv3l.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CACsJy8AU3ZA1=Q3vahhP6Nr=FZNKd7oRJ04mtKVs+uvNqJeVaw@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 17 quoted lines

>>> The "1 means I understood this" convention is used by userdiff_config. I
>>> don't like that it is unlike every other config callback,...
>>> Looking at the code again, though, ...
>>> Hmm. Yeah. The userdiff calling convention dates back to late 2008....
>>> So I think we could go back and simplify the userdiff_config code now.
>>
>> I remembered where I saw the new "offender"; it was nd/columns
>> topic (Cc'ing Nguyễn).
>
> nd/columns does use "1" convention in git_column_config(), but the
> direct config callback function does not return 1 to config machinery.
> All call sites follow this pattern:
>
> int ret = git_column_config(key, var, "command", &colopts);
> if (ret <= 0) return ret;
>
> I think it's ok.

I too think this should be acceptable, but that is not the point.

Your excuse that "the toplevel callback in my callchain never returns 1, so overall the nd/columns series is ok" just muddies the water. It means if later somebody wanted to use inner callback functions you use from the git_column_config() callchain chain as a toplevel callback for whatever reason, that will violate the "0 for success, or -1 for error" convention. More importantly, if somebody wants to turn a top-level callback that currently returns 0 into a sub callback used by his callback callchain, he cannot change that existing callback to return 1 to tell him to short circuit, because for other callers returning 1 would be a violation.

What I was getting at is that we probably should officially declare that returning 1 to signal success is perfectly acceptable (and it probably should mean the caller who called the callback function as a sub callback should return immediately, taking it as a signal that the key has already been handled), as the primary purpose of this thread to discuss Peff's patch is to write these rules down.

Of course, all that relies on the audit of the git_config() machinery. I think it is written to accept non-negative as success, and that is why I said "I too think this should be acceptable" in the first place.