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

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Mar 10, 2013, 16:39 UTC
Message-ID: <20130310163933.GA26857@sandbox-ub.fritz.box>
In-Reply-To: <5138DFA3.8040308@ramsay1.demon.co.uk>

Hi,

On Thu, Mar 07, 2013 at 06:42:43PM +0000, Ramsay Jones wrote:

Show 15 quoted lines

> 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