Re: [PATCH v8 8/8] add tests for `git_config_get_string_const()`

2014-08-07 Thread Matthieu Moy
Junio C Hamano gits...@pobox.com writes:

 Matthieu Moy matthieu@grenoble-inp.fr writes:

 Tanay Abhra tanay...@gmail.com writes:

 ...
 +   grep line 7.*.git/config\|.git/config.*line 7 result
 +'

 This is still dependant on the locale (line is translated). You need
 to use test_i18ngrep instead of grep here (see its definition and
 comment in t/test-lib.sh).

 I don't think you need these two alternatives OTOH.

 BTW, Junio, I don't understand your remark This test is too tight (the
 full string) in the previous iteration. Can you elaborate?

 The original test was trying to match the string fully, i.e.

 +grep fatal: bad config variable '\''alias.br'\'' at file line 2 in 
 .git/config result

 As I already was feeling funny about seeing the phrase file line
 in the message and expecting it to be corrected, I thought I should
 encourage a check that does not depend on minor phrasing changes, if
 it can be done without bending backwards.

OK.

I personally prefer tight tests in this case, as the patch doing the
rephrase sees what changed by running the tests, and documents the
visible change by changing the expected in the tests scripts. But no
strong opinion here, I'd be fine with e.g.

test_i18ngrep fatal: .* alias.br.*line 2 in .git/config result

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v8 8/8] add tests for `git_config_get_string_const()`

2014-08-06 Thread Tanay Abhra
Add tests for `git_config_get_string_const()`, check whether it
dies printing the line number and the file name if a NULL
value is retrieved for the given key.

Signed-off-by: Tanay Abhra tanay...@gmail.com
---
 t/t1308-config-set.sh | 10 ++
 test-config.c | 10 ++
 2 files changed, 20 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 5ea0aa4..ecc757a 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,16 @@ test_expect_success 'find integer value for a key' '
check_config get_int lamb.chop 65
 '
 
+test_expect_success 'find string value for a key' '
+   check_config get_string case.baz hask 
+   check_config expect_code 1 get_string case.ba Value not found for 
\case.ba\
+'
+
+test_expect_success 'check line error when NULL string is queried' '
+   test_expect_code 128 test-config get_string case.foo 2result 
+   grep line 7.*.git/config\|.git/config.*line 7 result
+'
+
 test_expect_success 'find integer if value is non parse-able' '
check_config expect_code 128 get_int lamb.head
 '
diff --git a/test-config.c b/test-config.c
index 9dd1b22..6a77552 100644
--- a/test-config.c
+++ b/test-config.c
@@ -16,6 +16,8 @@
  *
  * get_bool - print bool value for the entered key or die
  *
+ * get_string - print string value for the entered key or die
+ *
  * configset_get_value - returns value with the highest priority for the 
entered key
  * from a config_set constructed from files entered as 
arguments.
  *
@@ -84,6 +86,14 @@ int main(int argc, char **argv)
printf(Value not found for \%s\\n, argv[2]);
goto exit1;
}
+   } else if (argc == 3  !strcmp(argv[1], get_string)) {
+   if (!git_config_get_string_const(argv[2], v)) {
+   printf(%s\n, v);
+   goto exit0;
+   } else {
+   printf(Value not found for \%s\\n, argv[2]);
+   goto exit1;
+   }
} else if (!strcmp(argv[1], configset_get_value)) {
for (i = 3; i  argc; i++) {
int err;
-- 
1.9.0.GIT

--
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 v8 8/8] add tests for `git_config_get_string_const()`

2014-08-06 Thread Matthieu Moy
Tanay Abhra tanay...@gmail.com writes:

 +test_expect_success 'find string value for a key' '
 + check_config get_string case.baz hask 
 + check_config expect_code 1 get_string case.ba Value not found for 
 \case.ba\
 +'
 +
 +test_expect_success 'check line error when NULL string is queried' '
 + test_expect_code 128 test-config get_string case.foo 2result 
 + grep line 7.*.git/config\|.git/config.*line 7 result
 +'

This is still dependant on the locale (line is translated). You need
to use test_i18ngrep instead of grep here (see its definition and
comment in t/test-lib.sh).

I don't think you need these two alternatives OTOH.

BTW, Junio, I don't understand your remark This test is too tight (the
full string) in the previous iteration. Can you elaborate?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v8 8/8] add tests for `git_config_get_string_const()`

2014-08-06 Thread Tanay Abhra


On 8/6/2014 9:02 PM, Matthieu Moy wrote:
 Tanay Abhra tanay...@gmail.com writes:
 
 +test_expect_success 'find string value for a key' '
 +check_config get_string case.baz hask 
 +check_config expect_code 1 get_string case.ba Value not found for 
 \case.ba\
 +'
 +
 +test_expect_success 'check line error when NULL string is queried' '
 +test_expect_code 128 test-config get_string case.foo 2result 
 +grep line 7.*.git/config\|.git/config.*line 7 result
 +'
 
 This is still dependant on the locale (line is translated). You need
 to use test_i18ngrep instead of grep here (see its definition and
 comment in t/test-lib.sh).


Oh, and I was searching t/README for a hint. Maybe we should add a line there
to hint future readers.
Thanks. :)

 I don't think you need these two alternatives OTOH.
 
 BTW, Junio, I don't understand your remark This test is too tight (the
 full string) in the previous iteration. Can you elaborate?


I think he meant we must search for the relevant snippets instead of the whole 
string.
--
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 v8 8/8] add tests for `git_config_get_string_const()`

2014-08-06 Thread Junio C Hamano
Matthieu Moy matthieu@grenoble-inp.fr writes:

 Tanay Abhra tanay...@gmail.com writes:

 ...
 +grep line 7.*.git/config\|.git/config.*line 7 result
 +'

 This is still dependant on the locale (line is translated). You need
 to use test_i18ngrep instead of grep here (see its definition and
 comment in t/test-lib.sh).

 I don't think you need these two alternatives OTOH.

 BTW, Junio, I don't understand your remark This test is too tight (the
 full string) in the previous iteration. Can you elaborate?

The original test was trying to match the string fully, i.e.

 + grep fatal: bad config variable '\''alias.br'\'' at file line 2 in 
 .git/config result

As I already was feeling funny about seeing the phrase file line
in the message and expecting it to be corrected, I thought I should
encourage a check that does not depend on minor phrasing changes, if
it can be done without bending backwards.

I do agree with you that using \| in grep a pattern to trigger
ERE Alternation counts as bending backwards as that is a GNU
extension and not portable.
--
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