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

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

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