Re: [PATCH 1/2] config: be strict on core.commentChar
Jonathan Nieder writes:
> Nguyễn Thái Ngọc Duy wrote:
>
>> --- a/config.c
>> +++ b/config.c
>> @@ -826,8 +826,12 @@ static int git_default_core_config(const char *var,
>> const char *value)
>> if (!strcmp(var, "core.commentchar")) {
>> const char *comment;
>> int ret = git_config_string(&comment, var, value);
>> -if (!ret)
>> -comment_line_char = comment[0];
>> +if (!ret) {
>> +if (comment[0] && !comment[1])
>> +comment_line_char = comment[0];
>> +else
>> +return error("core.commentChar should only be
>> one character");
>> +}
>
> Perhaps, to decrease indentation a little:
>
> if (ret)
> return ret;
> if (comment[0] && !comment[1])
> comment_line_char = comment[0];
> else
> return error(...);
> return 0;
>
> [...]
>> --- a/t/t7508-status.sh
>> +++ b/t/t7508-status.sh
>> @@ -1348,12 +1348,6 @@ test_expect_success "status (core.commentchar with
>> submodule summary)" '
>> test_i18ncmp expect output
>> '
>>
>> -test_expect_success "status (core.commentchar with two chars with submodule
>> summary)" '
>> -test_config core.commentchar ";;" &&
>> -git -c status.displayCommentPrefix=true status >output &&
>> -test_i18ncmp expect output
>
> Could keep the test to avoid regressions:
>
> test_config core.commentchar ";;" &&
> test_must_fail git -c status.displayCommentPrefix=true status
All good points, including your other review message.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] config: be strict on core.commentChar
Nguyễn Thái Ngọc Duy wrote:
> --- a/config.c
> +++ b/config.c
> @@ -826,8 +826,12 @@ static int git_default_core_config(const char *var,
> const char *value)
> if (!strcmp(var, "core.commentchar")) {
> const char *comment;
> int ret = git_config_string(&comment, var, value);
> - if (!ret)
> - comment_line_char = comment[0];
> + if (!ret) {
> + if (comment[0] && !comment[1])
> + comment_line_char = comment[0];
> + else
> + return error("core.commentChar should only be
> one character");
> + }
Perhaps, to decrease indentation a little:
if (ret)
return ret;
if (comment[0] && !comment[1])
comment_line_char = comment[0];
else
return error(...);
return 0;
[...]
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1348,12 +1348,6 @@ test_expect_success "status (core.commentchar with
> submodule summary)" '
> test_i18ncmp expect output
> '
>
> -test_expect_success "status (core.commentchar with two chars with submodule
> summary)" '
> - test_config core.commentchar ";;" &&
> - git -c status.displayCommentPrefix=true status >output &&
> - test_i18ncmp expect output
Could keep the test to avoid regressions:
test_config core.commentchar ";;" &&
test_must_fail git -c status.displayCommentPrefix=true status
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] config: be strict on core.commentChar
Nguyễn Thái Ngọc Duy wrote:
> We don't support comment _strings_ (at least not yet). And multi-byte
> character encoding could also be misinterpreted.
>
> The test with two commas is deleted because it violates this. It's
> added with the patch that introduces core.commentChar in eff80a9
> (Allow custom "comment char" - 2013-01-16). It's not clear to me _why_
> that behavior is wanted.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> config.c | 8 ++--
> t/t7508-status.sh | 6 --
> 2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/config.c b/config.c
> index a30cb5c..05d909b 100644
> --- a/config.c
> +++ b/config.c
> @@ -826,8 +826,12 @@ static int git_default_core_config(const char *var,
> const char *value)
> if (!strcmp(var, "core.commentchar")) {
> const char *comment;
> int ret = git_config_string(&comment, var, value);
> - if (!ret)
> - comment_line_char = comment[0];
> + if (!ret) {
> + if (comment[0] && !comment[1])
> + comment_line_char = comment[0];
> + else
> + return error("core.commentChar should only be
> one character");
> + }
Small nit:
if (ret)
return ret;
if (comment[0] && !comment[1])
comment_line_char = comment[0];
else
return error("core.commentChar should only be one character");
--
Felipe Contreras--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
