Re: Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs

[Heiko Voigt <hvoigt@hvoigt.net>]

On Fri, May 10, 2013 at 12:39:37AM +0200, Jeff King wrote:
> On Thu, May 09, 2013 at 06:21:02PM +0200, Heiko Voigt wrote:
> 
> > diff --git a/builtin/config.c b/builtin/config.c
> > index 8d01b7a..de32977 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -219,9 +219,11 @@ static int get_value(const char *key_, const char *regex_)
> >  		}
> >  	}
> >  
> > -	git_config_with_options(collect_config, &values,
> > +	ret = git_config_with_options(collect_config, &values,
> >  				given_config_file, given_config_blob,
> >  				respect_includes);
> > +	if (ret < 0)
> > +		goto free_values;
> >  
> >  	ret = !values.nr;
> >  
> > @@ -231,6 +233,7 @@ static int get_value(const char *key_, const char *regex_)
> >  			fwrite(buf->buf, 1, buf->len, stdout);
> >  		strbuf_release(buf);
> >  	}
> > +free_values:
> >  	free(values.items);
> >  
> >  free_strings:
> 
> Hmm. "values" is a strbuf_list. Don't we need to strbuf_release() its
> individual members before freeing it? The writing loop directly above
> your label frees each one after writing. It would probably make sense to
> just split it into two loops, one for writing, and then one for
> free-ing. This is not a critical performance path that we can't iterate
> over the (probably handful of) entries twice.

Thanks for catching that. I missed the strbuf_release in the loop
somehow. Will fix in the next iteration.

> > diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
> > index fdc257e..a4929eb 100755
> > --- a/t/t1307-config-blob.sh
> > +++ b/t/t1307-config-blob.sh
> > @@ -55,12 +55,17 @@ test_expect_success 'editing a blob is an error' '
> >  test_expect_success 'parse errors in blobs are properly attributed' '
> >  	cat >config <<-\EOF &&
> >  	[some]
> > -		value = "
> > +		value = 1
> > +		error = "
> >  	EOF
> >  	git add config &&
> >  	git commit -m broken &&
> >  
> > -	test_must_fail git config --blob=HEAD:config some.value 2>err &&
> > +	test_must_fail git config --blob=HEAD:config some.value 1>out 2>err &&
> > +
> > +	# Make sure there is no output of values on stdout
> > +	touch out.expect &&
> > +	test_cmp out.expect out &&
> 
> I'm not clear on what is being tested here. Were we outputting to stdout
> before this patch (I would have thought our die() meant we did not.

We where not outputting to stdout before so the test is correct there as
well. I extended the test when implementing the non-dying version of
git_config_with_options() so I could see the test fail. If just
returning the error it would still output the values read until then. So
if you think that it belongs into the initial version of this test
(maybe including some comment why we need an extra parseable value) I
am happy to move it there.

Cheers Heiko