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

[Heiko Voigt <hvoigt@hvoigt.net>]

On Tue, Mar 12, 2013 at 03:04:56PM -0400, Jeff King wrote:
> On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:
> 
> > > 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:
> > [...]
> > +/* 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
> 
> I think that is OK, but it may be even better to list the fields by
> name. Also, our multi-line comment style is:
> 
>   /*
>    * Multi-line comment.
>    */

Ok will do both.

> > 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.
> 
> It does not matter to the end result, but I find it helps with reviewing
> when the comment is added along with the function, and then expanded as
> the function is changed. It helps to understand the effects of later
> patches if they need to tweak comments.

To make the series more clear to others who read it later, I will add
the comment from the beginning.

Cheers Heiko