Re: [PATCH] make config --add behave correctly for empty and NULL values

[Junio C Hamano]

Jeff King p...@peff.net writes:

 Here is the patch I wrote, for reference (I also think breaking the
 matches function into a series of conditionals, as you showed, is way
 more readable):

OK, while reviewing the today's issue of What's cooking and making
topics graduate to 'master', I got annoyed that the bottom of jch
branch still needed to be kept.  Let's do this.

-- 8 --
From: Jeff King p...@peff.net
Date: Tue, 19 Aug 2014 02:20:00 -0400
Subject: [PATCH] config: avoid a funny sentinel value a^

Introduce CONFIG_REGEX_NONE as a more explicit sentinel value to say
we do not want to replace any existing entry and use it in the
implementation of git config --add.

Signed-off-by: Jeff King p...@peff.net
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 builtin/config.c |  3 ++-
 cache.h  |  2 ++
 config.c | 23 +--
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 8224699..bf1aa6b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -599,7 +599,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_source.file,
-  argv[0], value, a^, 0);
+  argv[0], value,
+  CONFIG_REGEX_NONE, 0);
}
else if (actions == ACTION_REPLACE_ALL) {
check_write();
diff --git a/cache.h b/cache.h
index c708062..8356168 100644
--- a/cache.h
+++ b/cache.h
@@ -1233,6 +1233,8 @@ extern int update_server_info(int);
 #define CONFIG_INVALID_PATTERN 6
 #define CONFIG_GENERIC_ERROR 7
 
+#define CONFIG_REGEX_NONE ((void *)1)
+
 struct git_config_source {
unsigned int use_stdin:1;
const char *file;
diff --git a/config.c b/config.c
index ffe0104..2e709bf 100644
--- a/config.c
+++ b/config.c
@@ -1230,10 +1230,15 @@ static struct {
 
 static int matches(const char *key, const char *value)
 {
-   return !strcmp(key, store.key) 
-   (store.value_regex == NULL ||
-(store.do_not_match ^
- (value  !regexec(store.value_regex, value, 0, NULL, 0;
+   if (strcmp(key, store.key))
+   return 0; /* not ours */
+   if (!store.value_regex)
+   return 1; /* always matches */
+   if (store.value_regex == CONFIG_REGEX_NONE)
+   return 0; /* never matches */
+
+   return store.do_not_match ^
+   (value  !regexec(store.value_regex, value, 0, NULL, 0));
 }
 
 static int store_aux(const char *key, const char *value, void *cb)
@@ -1495,6 +1500,8 @@ out_free_ret_1:
 /*
  * If value==NULL, unset in (remove from) config,
  * if value_regex!=NULL, disregard key/value pairs where value does not match.
+ * if value_regex==CONFIG_REGEX_NONE, do not match any existing values
+ * (only add a new one)
  * if multi_replace==0, nothing, or only one matching key/value is replaced,
  * else all matching key/values (regardless how many) are removed,
  * before the new pair is written.
@@ -1578,6 +1585,8 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
 
if (value_regex == NULL)
store.value_regex = NULL;
+   else if (value_regex == CONFIG_REGEX_NONE)
+   store.value_regex = CONFIG_REGEX_NONE;
else {
if (value_regex[0] == '!') {
store.do_not_match = 1;
@@ -1609,7 +1618,8 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
if (git_config_from_file(store_aux, config_filename, NULL)) {
error(invalid config file %s, config_filename);
free(store.key);
-   if (store.value_regex != NULL) {
+   if (store.value_regex != NULL 
+   store.value_regex != CONFIG_REGEX_NONE) {
regfree(store.value_regex);
free(store.value_regex);
}
@@ -1618,7 +1628,8 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
}
 
free(store.key);
-   if (store.value_regex != NULL) {
+   if (store.value_regex != NULL 
+   store.value_regex != CONFIG_REGEX_NONE) {
regfree(store.value_regex);
free(store.value_regex);
}
-- 
2.1.0-466-g6597b3e

--
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] make config --add behave correctly for empty and NULL values

[Jeff King]

On Thu, Sep 11, 2014 at 04:35:33PM -0700, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  Here is the patch I wrote, for reference (I also think breaking the
  matches function into a series of conditionals, as you showed, is way
  more readable):
 
 OK, while reviewing the today's issue of What's cooking and making
 topics graduate to 'master', I got annoyed that the bottom of jch
 branch still needed to be kept.  Let's do this.
 
 -- 8 --
 From: Jeff King p...@peff.net
 Date: Tue, 19 Aug 2014 02:20:00 -0400
 Subject: [PATCH] config: avoid a funny sentinel value a^
 
 Introduce CONFIG_REGEX_NONE as a more explicit sentinel value to say
 we do not want to replace any existing entry and use it in the
 implementation of git config --add.
 
 Signed-off-by: Jeff King p...@peff.net
 Signed-off-by: Junio C Hamano gits...@pobox.com

Looks good, and adding my signoff is fine. Thanks.

-Peff
--
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] make config --add behave correctly for empty and NULL values

[Junio C Hamano]

Jeff King p...@peff.net writes:

 I just used

   #define CONFIG_REGEX_NONE ((void *)1)

 as my magic sentinel value, both for the string and compiled regex
 versions. Adding a bit to the store struct is a lot less disgusting and
 error-prone. So I won't share mine here. :)

Actually, I wrote something like that aloud but did not type it out
;-).  Great minds think alike.

We already have some code paths that use ((void *)1) as a special
pointer value, so in that sense I would say it is not the end of the
world if you added a new one.  At the end-user level (i.e. people
who write callers to set-multivar-in-file function), I actually like
your idea of inventing our own string syntax and parse it at the
place where we strip '!' out and remember that the pattern's match
status needs to be negated.  For example, instead of a^ (to which
I cannot say with confidence that no implementation would match the
not-at-the-beginning caret literally), I would not mind if we taught
set-multivar-in-file that we use !* as a mark to tell this
pattern never matches, and have it assign your never matches
mark, i.e. (void *)1, to store.value_regex.  Then matches() would
become

static int matches(const char *key, const char *value)
{
if (strcmp(key, store.key))
return 0; /* not ours */
if (!store.value_regex)
return 1; /* always matches */
if (store.value_regex == CONFIG_REGEX_NONE)
return 0; /* never matches */
return store.do_not_match ^
!regexec(store.value_regex, value, 0, NULL, 0);
}

or something like that, and the ugly magic will be localized,
which may make it more palatable.
--
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] make config --add behave correctly for empty and NULL values

[Jeff King]

On Mon, Aug 18, 2014 at 11:03:51PM -0700, Junio C Hamano wrote:

 We already have some code paths that use ((void *)1) as a special
 pointer value, so in that sense I would say it is not the end of the
 world if you added a new one.

No, but if you use it to replace the regexp, you end up having to check
for it in the code paths that regfree() the result. I think a separate
bit is nicer for that reason.

 At the end-user level (i.e. people who write callers to
 set-multivar-in-file function), I actually like your idea of inventing
 our own string syntax and parse it at the place where we strip '!' out
 and remember that the pattern's match status needs to be negated.

I thought at first that ! by itself might be fine, but that is
actually a valid regex. And we may feed user input directly to the
function, so we have to be careful not to get too clever.

 For example, instead of a^ (to which
 I cannot say with confidence that no implementation would match the
 not-at-the-beginning caret literally), I would not mind if we taught
 set-multivar-in-file that we use !* as a mark to tell this
 pattern never matches,

That would work, but I think CONFIG_REGEX_NONE or similar is a bit less
cryptic, and not any harder to implement.

Here is the patch I wrote, for reference (I also think breaking the
matches function into a series of conditionals, as you showed, is way
more readable):

diff --git a/builtin/config.c b/builtin/config.c
index b9e7dce..7bba516 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -586,7 +586,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_source.file,
-  argv[0], value, a^, 0);
+  argv[0], value,
+  CONFIG_REGEX_NONE, 0);
}
else if (actions == ACTION_REPLACE_ALL) {
check_write();
diff --git a/cache.h b/cache.h
index fcb511d..dcf3a2a 100644
--- a/cache.h
+++ b/cache.h
@@ -1281,6 +1281,8 @@ extern int update_server_info(int);
 #define CONFIG_INVALID_PATTERN 6
 #define CONFIG_GENERIC_ERROR 7
 
+#define CONFIG_REGEX_NONE ((void *)1)
+
 struct git_config_source {
unsigned int use_stdin:1;
const char *file;
diff --git a/config.c b/config.c
index 67a7729..1199faf 100644
--- a/config.c
+++ b/config.c
@@ -1229,6 +1229,7 @@ static struct {
 static int matches(const char *key, const char *value)
 {
return !strcmp(key, store.key) 
+   store.value_regex != CONFIG_REGEX_NONE 
(store.value_regex == NULL ||
 (store.do_not_match ^
  (value  !regexec(store.value_regex, value, 0, NULL, 0;
@@ -1493,6 +1494,8 @@ out_free_ret_1:
 /*
  * If value==NULL, unset in (remove from) config,
  * if value_regex!=NULL, disregard key/value pairs where value does not match.
+ * if value_regex==CONFIG_REGEX_NONE, do not match any existing values
+ * (only add a new one)
  * if multi_replace==0, nothing, or only one matching key/value is replaced,
  * else all matching key/values (regardless how many) are removed,
  * before the new pair is written.
@@ -1576,6 +1579,8 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
 
if (value_regex == NULL)
store.value_regex = NULL;
+   else if (value_regex == CONFIG_REGEX_NONE)
+   store.value_regex = CONFIG_REGEX_NONE;
else {
if (value_regex[0] == '!') {
store.do_not_match = 1;
@@ -1607,7 +1612,8 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
if (git_config_from_file(store_aux, config_filename, NULL)) {
error(invalid config file %s, config_filename);
free(store.key);
-   if (store.value_regex != NULL) {
+   if (store.value_regex != NULL 
+   store.value_regex != CONFIG_REGEX_NONE) {
regfree(store.value_regex);
free(store.value_regex);
}
@@ -1616,7 +1622,8 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
}
 
free(store.key);
-   if (store.value_regex != NULL) {
+   if (store.value_regex != NULL 
+   store.value_regex != CONFIG_REGEX_NONE) {
regfree(store.value_regex);
free(store.value_regex);
}
--
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  

[PATCH] make config --add behave correctly for empty and NULL values

[Tanay Abhra]

Currently if we have a config file like,
[foo]
baz
bar =

and we try something like, git config --add foo.baz roll, Git will
segfault. Moreover, for git config --add foo.bar roll, it will
overwrite the original value instead of appending after the existing
empty value.

The problem lies with the regexp used for simulating --add in
`git_config_set_multivar_in_file()`, ^$, which in ideal case should
not match with any string but is true for empty strings. Instead use a
regexp like a^ which can not be true for any string, empty or not.

For removing the segfault add a check for NULL values in `matches()` in
config.c.

Signed-off-by: Tanay Abhra tanay...@gmail.com
---
 builtin/config.c|  2 +-
 config.c|  2 +-
 t/t1303-wacky-config.sh | 20 
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index fcd8474..b9e7dce 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -586,7 +586,7 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_source.file,
-  argv[0], value, ^$, 0);
+  argv[0], value, a^, 0);
}
else if (actions == ACTION_REPLACE_ALL) {
check_write();
diff --git a/config.c b/config.c
index 058505c..67a7729 100644
--- a/config.c
+++ b/config.c
@@ -1231,7 +1231,7 @@ static int matches(const char *key, const char *value)
return !strcmp(key, store.key) 
(store.value_regex == NULL ||
 (store.do_not_match ^
- !regexec(store.value_regex, value, 0, NULL, 0)));
+ (value  !regexec(store.value_regex, value, 0, NULL, 0;
 }
 
 static int store_aux(const char *key, const char *value, void *cb)
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 3a2c819..3b92083 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -111,4 +111,24 @@ test_expect_success 'unset many entries' '
test_must_fail git config section.key
 '
 
+test_expect_success '--add appends new value after existing empty value' '
+   cat expect -\EOF 
+
+
+   fool
+   roll
+   EOF
+   cp .git/config .git/config.old 
+   test_when_finished mv .git/config.old .git/config 
+   cat .git/config -\EOF 
+   [foo]
+   baz
+   baz =
+   baz = fool
+   EOF
+   git config --add foo.baz roll 
+   git config --get-all foo.baz output 
+   test_cmp expect output
+'
+
 test_done
-- 
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] make config --add behave correctly for empty and NULL values

[Matthieu Moy]

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

 Currently if we have a config file like,
 [foo]
 baz
 bar =

 and we try something like, git config --add foo.baz roll, Git will
 segfault. Moreover, for git config --add foo.bar roll, it will
 overwrite the original value instead of appending after the existing
 empty value.

 The problem lies with the regexp used for simulating --add in
 `git_config_set_multivar_in_file()`, ^$, which in ideal case should
 not match with any string but is true for empty strings. Instead use a
 regexp like a^ which can not be true for any string, empty or not.

 For removing the segfault add a check for NULL values in `matches()` in
 config.c.

I would have prefered two separate patches (or even better, 3, the first
one being demonstrate failure of ... with test_expect_failure) for
each issues.

But the fixes are straightforward, and the test actually test what it
has to test, so I think we can keep the patch as-is.

Thanks,

-- 
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] make config --add behave correctly for empty and NULL values

[Junio C Hamano]

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

 Currently if we have a config file like,
 [foo]
 baz
 bar =

 and we try something like, git config --add foo.baz roll, Git will
 segfault.

Thanks; this is a good find.

This is a tangent, but people please stop starting their sentence
with a somewhat irritating Currently; it does not help both
current and future readers very much without some mention of version
numbers.

I suspect this bug dates back to pretty much day one of git config
(dates at least back to 1.5.3).

 The problem lies with the regexp used for simulating --add in
 `git_config_set_multivar_in_file()`, ^$, which in ideal case should
 not match with any string but is true for empty strings. Instead use a
 regexp like a^ which can not be true for any string, empty or not.

Yuck, but we cannot pass NULL or some other special value that look
more meaningful to signal the fact that we do not want to match
anything, so this seems to be the easiest way out.  

Are we sure that a^, which cannot be true for any string, will not
be caught by anybody's regcomp() as an error?  I know regcomp()
accepts the expression and regexec() fails to match with GNU libc,
but that is not the whole of the world.

At least, please make it clear for those who read this code later
what is going on with this magic a^, perhaps with

#define REGEXP_THAT_NEVER_MATCHES a^
...
return git_config_set_multivar_in_file(given_config_source.file,
  argv[0], value,
  REGEXP_THAT_NEVER_MATCHES, 0);
and/or with in-code comment.

/*
 * set_multivar_in_file() removes existing values that match
 * the value_regexp argument and then adds this new value;
 * pass a pattern that never matches anything, as we do not
 * want to remove any existing value.
 */
return git_config_set_multivar_in_file(given_config_source.file,
  argv[0], value,
  REGEXP_THAT_NEVER_MATCHES, 0);

To be honest, I'd rather see this done right, by giving an option
to the caller to tell the function not to call regcomp/regexec in
matches().

 * Define a global exported via cache.h and defined in config.c

extern const char CONFIG_SET_MULTIVAR_NO_REPLACE[];

   and pass it from this calling site, instead of an arbitrary
   literal string e.g. a^

 * Add a bit to the store struct, e.g. unsigned value_never_matches:1;

 * In git_config_set_multivar_in_file() implementation, check for
   this constant address and set store.value_never_matches to true;

 * in matches(), check this bit and always return No, this existing
   value do not match when it is set.

or something like that.

 For removing the segfault add a check for NULL values in `matches()` in
 config.c.

The fact that you do a check is important, but it equally if not
more important what you do with the result.  Check for a NULL and
consider it as not matching is probably what you meant, but I'd
like to double check.

 Signed-off-by: Tanay Abhra tanay...@gmail.com
 ---
  builtin/config.c|  2 +-
  config.c|  2 +-
  t/t1303-wacky-config.sh | 20 
  3 files changed, 22 insertions(+), 2 deletions(-)

 diff --git a/builtin/config.c b/builtin/config.c
 index fcd8474..b9e7dce 100644
 --- a/builtin/config.c
 +++ b/builtin/config.c
 @@ -586,7 +586,7 @@ int cmd_config(int argc, const char **argv, const char 
 *prefix)
   check_argc(argc, 2, 2);
   value = normalize_value(argv[0], argv[1]);
   return git_config_set_multivar_in_file(given_config_source.file,
 -argv[0], value, ^$, 0);
 +argv[0], value, a^, 0);
   }
   else if (actions == ACTION_REPLACE_ALL) {
   check_write();
 diff --git a/config.c b/config.c
 index 058505c..67a7729 100644
 --- a/config.c
 +++ b/config.c
 @@ -1231,7 +1231,7 @@ static int matches(const char *key, const char *value)
   return !strcmp(key, store.key) 
   (store.value_regex == NULL ||
(store.do_not_match ^
 -   !regexec(store.value_regex, value, 0, NULL, 0)));
 +   (value  !regexec(store.value_regex, value, 0, NULL, 0;
  }
  
  static int store_aux(const char *key, const char *value, void *cb)
 diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
 index 3a2c819..3b92083 100755
 --- a/t/t1303-wacky-config.sh
 +++ b/t/t1303-wacky-config.sh
 @@ -111,4 +111,24 @@ test_expect_success 'unset many entries' '
   test_must_fail git config section.key
  '
  
 +test_expect_success '--add appends new value after existing empty value' '
 + cat expect -\EOF 
 +
 +
 + fool
 + roll
 + EOF
 + cp .git/config .git/config.old 
 + test_when_finished mv 

Re: [PATCH] make config --add behave correctly for empty and NULL values

[Jeff King]

On Mon, Aug 18, 2014 at 11:18:52AM -0700, Junio C Hamano wrote:

 Are we sure that a^, which cannot be true for any string, will not
 be caught by anybody's regcomp() as an error?  I know regcomp()
 accepts the expression and regexec() fails to match with GNU libc,
 but that is not the whole of the world.

We do support negation (ourselves) in the regexp, so !$foo would work,
where $foo is some regexp that always matches. But that may be digging
ourselves the opposite hole, trying to find a pattern that reliably
matches everything.

 To be honest, I'd rather see this done right, by giving an option
 to the caller to tell the function not to call regcomp/regexec in
 matches().

Yeah, that was my first thought, too on seeing the patch. I even worked
up an example before reading your message, but:

  * Define a global exported via cache.h and defined in config.c
 
   extern const char CONFIG_SET_MULTIVAR_NO_REPLACE[];
 
and pass it from this calling site, instead of an arbitrary
literal string e.g. a^
 
  * Add a bit to the store struct, e.g. unsigned value_never_matches:1;
 
  * In git_config_set_multivar_in_file() implementation, check for
this constant address and set store.value_never_matches to true;
 
  * in matches(), check this bit and always return No, this existing
value do not match when it is set.

I just used

  #define CONFIG_REGEX_NONE ((void *)1)

as my magic sentinel value, both for the string and compiled regex
versions. Adding a bit to the store struct is a lot less disgusting and
error-prone. So I won't share mine here. :)

-Peff
--
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