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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]