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

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

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

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