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

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Feb 8, 2012, 06:55 UTC
Message-ID: <CACsJy8B1cMBbrUznOp95u8YmfLf5bbzox8g9cuQUwgf-vY1XrQ@mail.gmail.com>
In-Reply-To: <7vmx8tzv3l.fsf@alter.siamese.dyndns.org>

On Wed, Feb 8, 2012 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

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

Or allow multiple callbacks from git_config(). The problem with is it adds another common set of config vars. Now it's common to chain var sets by doing

int config_callback(...)
{
    ....
    return yet_another_callback(...);
}

int main()
{
   git_config(config_callback, ...)
   ...

where yet_another_callback() hard codes another callback (usually git_default_config). This is inflexible. If we allow multiple callbacks, git_column_config() could be changed to return just 0 or -1 (i.e. 1 remains unsuccessful error code).

On second thought, I think calling git_config() multiple times, each time with one callback, may work too. We may want to cache config content to avoid accessing files too many times though.

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

-- 
Duy