Re: [PATCH 4/4] teach config parsing to read from strbuf

[Heiko Voigt <hvoigt@hvoigt.net>]

Hi,

On Thu, Mar 07, 2013 at 06:42:43PM +0000, Ramsay Jones wrote:
> Heiko Voigt wrote:
> > +int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)
> > +{
> > +	struct config top;
> > +	struct config_strbuf str;
> > +
> > +	str.strbuf = strbuf;
> > +	str.pos = 0;
> > +
> > +	top.data = &str;
> 
> You will definitely want to initialise 'top.name' here, rather
> than let it take whatever value happens to be at that position
> on the stack. In your editor, search for 'cf->name' and contemplate
> each such occurrence.

Good catch, thanks. The initialization seems to got lost during
refactoring. In the codepaths we call with the new strbuf function it is
only used for error reporting so I think we need to get the name from
the user of this function so the error messages are useful.

I have extended the test to demonstrate how I imagine this name could
look like.

> Does the 'include' facility work from a strbuf? Should it?

No the 'include' facility does not work here. A special handling would
need to be implemented by the caller. For us 'include' values have no
special meaning and are parsed like normal values.

AFAICS when a config file is given to git config we do not currently
respect includes. So we can just do the same behavior here for the
moment. There is no roadblock against adding it later.

> Are you happy with the error handling/reporting?

Good point, while adding the name for strbuf I noticed that we currently
die on some parsing errors. We should probably make these errors
handleable for strbufs. Currently the main usecase is to read submodule
configurations from the database and in case someone commits a broken
configuration we should be able to continue with that. Otherwise the
repository might render into an unuseable state. I will look into that.

> Do the above additions to the test-suite give you confidence
> that the code works as you intend?

Well, I am reusing most of the existing infrastructure which I assume is
tested using config files. So what I want to test here is the
integration of this new function. As you mentioned the name variable I
have now added a test for the parsing error case as well. I have
refactored the test binary to read configs from stdin so its easiert to
implement further tests from the bash part of the testsuite.

I will send out another iteration shortly.

Cheers Heiko