RE: [PATCH 1/2] config: be strict on core.commentChar

2014-05-16 Thread Felipe Contreras 
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 pclo...@gmail.com
 ---
  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

Re: [PATCH 1/2] config: be strict on core.commentChar

2014-05-16 Thread Jonathan Nieder 
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

2014-05-16 Thread Junio C Hamano 
Jonathan Nieder jrnie...@gmail.com 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