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
[PATCH 1/2] config: be strict on core.commentChar
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");
+ }
return ret;
}
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c987b5e..98a9990 100755
--- 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
-'
-
test_expect_success "--ignore-submodules=all suppresses submodule summary" '
cat > expect << EOF &&
On branch master
--
1.9.1.346.ga2b5940
--
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
