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

[Jeff King <peff@peff.net>]

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


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

I do not care that much in this instance, since we have already
discussed it, and I know what is going on, though.

-Peff