Re: [RFC/PATCH 0/4] config include directives

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 27, 2012, 09:51 UTC
Message-ID: <CACBZZX59sur4_61LkN_sMOvXQ4Jdnt1P8O-UOgm0SooBQpjFdQ@mail.gmail.com>
In-Reply-To: <20120126073547.GA28689@sigill.intra.peff.net>

On Thu, Jan 26, 2012 at 08:35, Jeff King <peff@peff.net> wrote:

Show 20 quoted lines

> This series provides a way for config files to include other config
> files in two ways:
>
>  1. From other files in the filesystem. This is implemented by patch 1
>     below, and is hopefully straightforward and uncontroversial.  See
>     that patch for more rationale.
>
>  2. From blobs in the repo. This is implemented by patch 4, with
>     patches 2 and 3 providing the necessary refactoring. This
>     is one way of implementing the often asked-for "respect shared
>     config inside the repo" feature, but attempts to mitigate some of
>     the security concerns. The interface for using it safely is a bit
>     raw, but I think it's a sane building block, and somebody could
>     write a fancier shared-config updater on top of it if they wanted
>     to.
>
>  [1/4]: config: add include directive
>  [2/4]: config: factor out config file stack management
>  [3/4]: config: support parsing config data from buffers
>  [4/4]: config: allow including config from repository blobs

I expect you've thought about this, but our current API is (from add.c):

	git_config(add_config, NULL);

Followed by:

    static int add_config(const char *var, const char *value, void *cb)
    {
    	if (!strcmp(var, "add.ignoreerrors") ||
    	    !strcmp(var, "add.ignore-errors")) {
    		ignore_add_errors = git_config_bool(var, value);
    		return 0;
    	}
    	return git_default_config(var, value, cb);
    }

I.e. that function gets called with one key at a time, and stashes it to a local value.

If you write the function like that it means your patch series just works since values encountered later will override earlier ones, but have you checked git's code to make sure we don't have anything like:

    static int ignore_add_errors_is_set = 0;
    static int add_config(const char *var, const char *value, void *cb)
    {
    	if (!ignore_add_errors_is_set &&
            (!strcmp(var, "add.ignoreerrors") ||
    	     !strcmp(var, "add.ignore-errors"))) {
    		ignore_add_errors = git_config_bool(var, value);
            ignore_add_errors_is_set = 1;
    		return 0;
    	}
    	return git_default_config(var, value, cb);
    }

Which would mean that the include config support would be silently ignored.