Re: [PATCH v2 1/4] config: factor out config file stack management

[Heiko Voigt <hvoigt@hvoigt.net>]

On Tue, Mar 12, 2013 at 06:52:00AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:57:53PM +0100, Heiko Voigt wrote:
> 
> > Because a config callback may start parsing a new file, the
> > global context regarding the current config file is stored
> > as a stack. Currently we only need to manage that stack from
> > git_config_from_file. Let's factor it out to allow new
> > sources of config data.
> >
> > [...]
> >
> > +static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
> > +{
> > +	int ret;
> > +
> > +	/* push config-file parsing state stack */
> > +	top->prev = cf;
> > +	top->linenr = 1;
> > +	top->eof = 0;
> > +	strbuf_init(&top->value, 1024);
> > +	strbuf_init(&top->var, 1024);
> > +	cf = top;
> > +
> > +	ret = git_parse_file(fn, data);
> > +
> > +	/* pop config-file parsing state stack */
> > +	strbuf_release(&top->value);
> > +	strbuf_release(&top->var);
> > +	cf = top->prev;
> > +
> > +	return ret;
> > +}
> 
> Can we throw in a comment at the top here with the expected usage? In
> particular, do_config_from is expecting the caller to have filled in
> certain fields (at this point, top->f and top->name), but there is
> nothing to make that clear.

Of course. Will do that in the next iteration. How about I squash this in:

diff --git a/config.c b/config.c
index b8c8640..b7632c9 100644
--- a/config.c
+++ b/config.c
@@ -948,6 +954,9 @@ int git_default_config(const char *var, const char *value, v
        return 0;
 }
 
+/* The fields data, name and the source specific callbacks of top need
+ * to be initialized before calling this function.
+ */
 static int do_config_from_source(struct config_source *top, config_fn_t fn, voi
 {
        int ret;


I would add that to the third patch:

	config: make parsing stack struct independent from actual data source

because that contains the final modification to config_file/config_source.

Cheers Heiko