[RFC/PATCH V2] alias.c: replace git_config with git_config_get_string

[RFC/PATCH V2] alias.c: replace git_config with git_config_get_string

[Tanay Abhra]

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 alias.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/alias.c b/alias.c
index 5efc3d6..0fe32bc 100644
--- a/alias.c
+++ b/alias.c
@@ -1,25 +1,17 @@
 #include "cache.h"
 
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
-	if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
-		if (!v)
-			return config_error_nonbool(k);
-		alias_val = xstrdup(v);
-		return 0;
-	}
-	return 0;
-}
-
 char *alias_lookup(const char *alias)
 {
-	alias_key = alias;
-	alias_val = NULL;
-	git_config(alias_lookup_cb, NULL);
-	return alias_val;
+	const char *v;
+	char *value;
+	struct strbuf key = STRBUF_INIT;
+	strbuf_addf(&key, "alias.%s", alias);
+	git_config_get_string(key.buf, &v);
+	if (!v)
+		config_error_nonbool(key.buf);
+	value = xstrdup(v);
+	strbuf_release(&key);
+	return value;
 }
 
 #define SPLIT_CMDLINE_BAD_ENDING 1
-- 
1.9.0.GIT

[RFC/PATCH V2] branch.c: replace git_config with git_config_get_string

[Tanay Abhra]

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 branch.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..c9a2a0d 100644
--- a/branch.c
+++ b/branch.c
@@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 	return 0;
 }
 
-struct branch_desc_cb {
+struct branch_desc {
 	const char *config_name;
 	const char *value;
 };
 
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
-	struct branch_desc_cb *desc = cb;
-	if (strcmp(desc->config_name, var))
-		return 0;
-	free((char *)desc->value);
-	return git_config_string(&desc->value, var, value);
-}
-
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
 {
-	struct branch_desc_cb cb;
+	const char *value = NULL;
+	struct branch_desc desc;
 	struct strbuf name = STRBUF_INIT;
 	strbuf_addf(&name, "branch.%s.description", branch_name);
-	cb.config_name = name.buf;
-	cb.value = NULL;
-	if (git_config(read_branch_desc_cb, &cb) < 0) {
+	desc.config_name = name.buf;
+	desc.value = NULL;
+	git_config_get_string(desc.config_name, &value);
+	if (git_config_string(&desc.value, desc.config_name, value) < 0) {
 		strbuf_release(&name);
 		return -1;
 	}
-	if (cb.value)
-		strbuf_addstr(buf, cb.value);
+	strbuf_addstr(buf, desc.value);
 	strbuf_release(&name);
 	return 0;
 }
-- 
1.9.0.GIT

Re: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Use git_config_get_string instead of git_config to take advantage of
> the config hash-table api which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  branch.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 660097b..c9a2a0d 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
>         return 0;
>  }
>
> -struct branch_desc_cb {
> +struct branch_desc {
>         const char *config_name;
>         const char *value;
>  };

What is the purpose of retaining this structure? Following your
changes, it is never used outside of read_branch_desc(), and
'config_name' and 'value' would be more naturally declared as
variables local to that function.

> -static int read_branch_desc_cb(const char *var, const char *value, void *cb)
> -{
> -       struct branch_desc_cb *desc = cb;
> -       if (strcmp(desc->config_name, var))
> -               return 0;
> -       free((char *)desc->value);
> -       return git_config_string(&desc->value, var, value);
> -}
> -
>  int read_branch_desc(struct strbuf *buf, const char *branch_name)
>  {
> -       struct branch_desc_cb cb;
> +       const char *value = NULL;
> +       struct branch_desc desc;
>         struct strbuf name = STRBUF_INIT;
>         strbuf_addf(&name, "branch.%s.description", branch_name);
> -       cb.config_name = name.buf;
> -       cb.value = NULL;
> -       if (git_config(read_branch_desc_cb, &cb) < 0) {
> +       desc.config_name = name.buf;
> +       desc.value = NULL;
> +       git_config_get_string(desc.config_name, &value);
> +       if (git_config_string(&desc.value, desc.config_name, value) < 0) {

Although it works in this case, it's somewhat ugly that you ignore the
return value of git_config_get_string(), and a person reading the code
has to spend extra time digging into git_config_string() to figure out
why this is safe. If might be clearer for future readers by rephrasing
like this:

    if (git_config_get_string(desc.config_name, &value) < 0 ||
        git_config_string(&desc.value, desc.config_name, value) < 0) {

>                 strbuf_release(&name);
>                 return -1;
>         }
> -       if (cb.value)
> -               strbuf_addstr(buf, cb.value);
> +       strbuf_addstr(buf, desc.value);
>         strbuf_release(&name);
>         return 0;
>  }
> --
> 1.9.0.GIT

Re: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/25/2014 10:15 AM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:

>> diff --git a/branch.c b/branch.c
>> index 660097b..c9a2a0d 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
>>         return 0;
>>  }
>>
>> -struct branch_desc_cb {
>> +struct branch_desc {
>>         const char *config_name;
>>         const char *value;
>>  };
> 
> What is the purpose of retaining this structure? Following your
> changes, it is never used outside of read_branch_desc(), and
> 'config_name' and 'value' would be more naturally declared as
> variables local to that function.

Done. :)

> 
>> -static int read_branch_desc_cb(const char *var, const char *value, void *cb)
>> -{
>> -       struct branch_desc_cb *desc = cb;
>> -       if (strcmp(desc->config_name, var))
>> -               return 0;
>> -       free((char *)desc->value);
>> -       return git_config_string(&desc->value, var, value);
>> -}
>> -
>>  int read_branch_desc(struct strbuf *buf, const char *branch_name)
>>  {
>> -       struct branch_desc_cb cb;
>> +       const char *value = NULL;
>> +       struct branch_desc desc;
>>         struct strbuf name = STRBUF_INIT;
>>         strbuf_addf(&name, "branch.%s.description", branch_name);
>> -       cb.config_name = name.buf;
>> -       cb.value = NULL;
>> -       if (git_config(read_branch_desc_cb, &cb) < 0) {
>> +       desc.config_name = name.buf;
>> +       desc.value = NULL;
>> +       git_config_get_string(desc.config_name, &value);
>> +       if (git_config_string(&desc.value, desc.config_name, value) < 0) {
> 
> Although it works in this case, it's somewhat ugly that you ignore the
> return value of git_config_get_string(), and a person reading the code
> has to spend extra time digging into git_config_string() to figure out
> why this is safe. If might be clearer for future readers by rephrasing
> like this:
> 
>     if (git_config_get_string(desc.config_name, &value) < 0 ||
>         git_config_string(&desc.value, desc.config_name, value) < 0) {
>

Noted, also didn't the old code leak desc.value as it was xstrduped
by git_config_string()? Thanks for the review.

>>                 strbuf_release(&name);
>>                 return -1;
>>         }
>> -       if (cb.value)
>> -               strbuf_addstr(buf, cb.value);
>> +       strbuf_addstr(buf, desc.value);
>>         strbuf_release(&name);
>>         return 0;
>>  }
>> --
>> 1.9.0.GIT
> 

Re: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Thu, Jun 26, 2014 at 4:09 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>
> On 6/25/2014 10:15 AM, Eric Sunshine wrote:
>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>> diff --git a/branch.c b/branch.c
>>> index 660097b..c9a2a0d 100644
>>> --- a/branch.c
>>> +++ b/branch.c
>>> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
>>>  int read_branch_desc(struct strbuf *buf, const char *branch_name)
>>>  {
>>> -       struct branch_desc_cb cb;
>>> +       const char *value = NULL;
>>> +       struct branch_desc desc;
>>>         struct strbuf name = STRBUF_INIT;
>>>         strbuf_addf(&name, "branch.%s.description", branch_name);
>>> -       cb.config_name = name.buf;
>>> -       cb.value = NULL;
>>> -       if (git_config(read_branch_desc_cb, &cb) < 0) {
>>> +       desc.config_name = name.buf;
>>> +       desc.value = NULL;
>>> +       git_config_get_string(desc.config_name, &value);
>>> +       if (git_config_string(&desc.value, desc.config_name, value) < 0) {
>>
>> Although it works in this case, it's somewhat ugly that you ignore the
>> return value of git_config_get_string(), and a person reading the code
>> has to spend extra time digging into git_config_string() to figure out
>> why this is safe. If might be clearer for future readers by rephrasing
>> like this:
>>
>>     if (git_config_get_string(desc.config_name, &value) < 0 ||
>>         git_config_string(&desc.value, desc.config_name, value) < 0) {
>>
>
> Noted, also didn't the old code leak desc.value as it was xstrduped
> by git_config_string()? Thanks for the review.

Looks that way.

[RFC/PATCH] imap-send.c: replace git_config with git_config_get_string

[Tanay Abhra]

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 imap-send.c | 68 ++++++++++++++++++++++++++-----------------------------------
 1 file changed, 29 insertions(+), 39 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 83a6ed2..87bd418 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1326,47 +1326,37 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)
 
 static char *imap_folder;
 
-static int git_imap_config(const char *key, const char *val, void *cb)
+static void git_imap_config(void)
 {
-	char imap_key[] = "imap.";
-
-	if (strncmp(key, imap_key, sizeof imap_key - 1))
-		return 0;
-
-	key += sizeof imap_key - 1;
-
-	/* check booleans first, and barf on others */
-	if (!strcmp("sslverify", key))
-		server.ssl_verify = git_config_bool(key, val);
-	else if (!strcmp("preformattedhtml", key))
-		server.use_html = git_config_bool(key, val);
-	else if (!val)
-		return config_error_nonbool(key);
-
-	if (!strcmp("folder", key)) {
-		imap_folder = xstrdup(val);
-	} else if (!strcmp("host", key)) {
-		if (starts_with(val, "imap:"))
-			val += 5;
-		else if (starts_with(val, "imaps:")) {
-			val += 6;
+	const char *value;
+
+	if (!git_config_get_string("imap.sslverify", &value))
+		server.ssl_verify = git_config_bool("sslverify", value);
+	if (!git_config_get_string("imap.preformattedhtml", &value))
+		server.use_html = git_config_bool("preformattedhtml", value);
+	if (!git_config_get_string("imap.folder", &value))
+		imap_folder = xstrdup(value);
+	if (!git_config_get_string("imap.host", &value)) {
+		if (starts_with(value, "imap:"))
+			value += 5;
+		else if (starts_with(value, "imaps:")) {
+			value += 6;
 			server.use_ssl = 1;
 		}
-		if (starts_with(val, "//"))
-			val += 2;
-		server.host = xstrdup(val);
-	} else if (!strcmp("user", key))
-		server.user = xstrdup(val);
-	else if (!strcmp("pass", key))
-		server.pass = xstrdup(val);
-	else if (!strcmp("port", key))
-		server.port = git_config_int(key, val);
-	else if (!strcmp("tunnel", key))
-		server.tunnel = xstrdup(val);
-	else if (!strcmp("authmethod", key))
-		server.auth_method = xstrdup(val);
-
-	return 0;
+		if (starts_with(value, "//"))
+			value += 2;
+		server.host = xstrdup(value);
+	}
+	if (!git_config_get_string("imap.user", &value))
+		server.user = xstrdup(value);
+	if (!git_config_get_string("imap.pass", &value))
+		server.pass = xstrdup(value);
+	if (!git_config_get_string("imap.port", &value))
+		server.port = git_config_int("port", value);
+	if (!git_config_get_string("imap.tunnel", &value))
+		server.tunnel = xstrdup(value);
+	if (!git_config_get_string("imap.authmethod", &value))
+		server.auth_method = xstrdup(value);
 }
 
 int main(int argc, char **argv)
@@ -1387,7 +1377,7 @@ int main(int argc, char **argv)
 		usage(imap_send_usage);
 
 	setup_git_directory_gently(&nongit_ok);
-	git_config(git_imap_config, NULL);
+	git_imap_config();
 
 	if (!server.port)
 		server.port = server.use_ssl ? 993 : 143;
-- 
1.9.0.GIT

Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Use git_config_get_string instead of git_config to take advantage of
> the config hash-table api which provides a cleaner control flow.

You may want to mention as a side-note the slight behavior change
introduced by this patch. The original code complained about any
unknown boolean "imap.*" key, whereas the new code does not.

More below.

> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  imap-send.c | 68 ++++++++++++++++++++++++++-----------------------------------
>  1 file changed, 29 insertions(+), 39 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 83a6ed2..87bd418 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1326,47 +1326,37 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)
>
>  static char *imap_folder;
>
> -static int git_imap_config(const char *key, const char *val, void *cb)
> +static void git_imap_config(void)
>  {
> -       char imap_key[] = "imap.";
> -
> -       if (strncmp(key, imap_key, sizeof imap_key - 1))
> -               return 0;
> -
> -       key += sizeof imap_key - 1;
> -
> -       /* check booleans first, and barf on others */
> -       if (!strcmp("sslverify", key))
> -               server.ssl_verify = git_config_bool(key, val);
> -       else if (!strcmp("preformattedhtml", key))
> -               server.use_html = git_config_bool(key, val);
> -       else if (!val)
> -               return config_error_nonbool(key);
> -
> -       if (!strcmp("folder", key)) {
> -               imap_folder = xstrdup(val);
> -       } else if (!strcmp("host", key)) {
> -               if (starts_with(val, "imap:"))
> -                       val += 5;
> -               else if (starts_with(val, "imaps:")) {
> -                       val += 6;
> +       const char *value;

Observation: If you name this variable 'val', which is the name of the
argument to the function in the original code, you will get a slightly
smaller and more readable diff. In this case, the improvement in the
diff is so slight that it might not be worth re-using the old variable
name, but in general, it's helpful to keep in mind that the smaller
and simpler the diff, the easier the patch is to review.

> +       if (!git_config_get_string("imap.sslverify", &value))
> +               server.ssl_verify = git_config_bool("sslverify", value);

I realize that you are just replicating the behavior of the original
code, but the error message emitted here for a non-bool value is less
than desirable since it throws away context (namely, the "imap."
prefix). You can improve the message, and help the user resolve the
error more quickly, by presenting the full configuration key (namely,
"imap.sslverify"). Such a change would deserve mention in the commit
message. Alternately, it could be fixed in a follow-up patch.

> +       if (!git_config_get_string("imap.preformattedhtml", &value))
> +               server.use_html = git_config_bool("preformattedhtml", value);

Ditto regarding error message: "imap.preformattedhtml"

> +       if (!git_config_get_string("imap.folder", &value))
> +               imap_folder = xstrdup(value);
> +       if (!git_config_get_string("imap.host", &value)) {
> +               if (starts_with(value, "imap:"))
> +                       value += 5;
> +               else if (starts_with(value, "imaps:")) {
> +                       value += 6;
>                         server.use_ssl = 1;
>                 }
> -               if (starts_with(val, "//"))
> -                       val += 2;
> -               server.host = xstrdup(val);
> -       } else if (!strcmp("user", key))
> -               server.user = xstrdup(val);
> -       else if (!strcmp("pass", key))
> -               server.pass = xstrdup(val);
> -       else if (!strcmp("port", key))
> -               server.port = git_config_int(key, val);
> -       else if (!strcmp("tunnel", key))
> -               server.tunnel = xstrdup(val);
> -       else if (!strcmp("authmethod", key))
> -               server.auth_method = xstrdup(val);
> -
> -       return 0;
> +               if (starts_with(value, "//"))
> +                       value += 2;
> +               server.host = xstrdup(value);
> +       }
> +       if (!git_config_get_string("imap.user", &value))
> +               server.user = xstrdup(value);
> +       if (!git_config_get_string("imap.pass", &value))
> +               server.pass = xstrdup(value);
> +       if (!git_config_get_string("imap.port", &value))
> +               server.port = git_config_int("port", value);

Same regarding diagnostic: "imap.port"

> +       if (!git_config_get_string("imap.tunnel", &value))
> +               server.tunnel = xstrdup(value);
> +       if (!git_config_get_string("imap.authmethod", &value))
> +               server.auth_method = xstrdup(value);
>  }
>
>  int main(int argc, char **argv)
> @@ -1387,7 +1377,7 @@ int main(int argc, char **argv)
>                 usage(imap_send_usage);
>
>         setup_git_directory_gently(&nongit_ok);
> -       git_config(git_imap_config, NULL);
> +       git_imap_config();
>
>         if (!server.port)
>                 server.port = server.use_ssl ? 993 : 143;
> --
> 1.9.0.GIT

Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/25/2014 12:39 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
> 
> You may want to mention as a side-note the slight behavior change
> introduced by this patch. The original code complained about any
> unknown boolean "imap.*" key, whereas the new code does not.
> 

Also, my code is error prone. Previous one had all NULL values returned
as config_non_boolean. But, now I have to add a NULL check to every strdup
in the code.

More below,

> 
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  imap-send.c | 68 ++++++++++++++++++++++++++-----------------------------------
>>  1 file changed, 29 insertions(+), 39 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 83a6ed2..87bd418 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1326,47 +1326,37 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)
>>
>>  static char *imap_folder;
>>
>> -static int git_imap_config(const char *key, const char *val, void *cb)
>> +static void git_imap_config(void)
>>  {
>> -       char imap_key[] = "imap.";
>> -
>> -       if (strncmp(key, imap_key, sizeof imap_key - 1))
>> -               return 0;
>> -
>> -       key += sizeof imap_key - 1;
>> -
>> -       /* check booleans first, and barf on others */
>> -       if (!strcmp("sslverify", key))
>> -               server.ssl_verify = git_config_bool(key, val);
>> -       else if (!strcmp("preformattedhtml", key))
>> -               server.use_html = git_config_bool(key, val);
>> -       else if (!val)
>> -               return config_error_nonbool(key);
>> -
>> -       if (!strcmp("folder", key)) {
>> -               imap_folder = xstrdup(val);
>> -       } else if (!strcmp("host", key)) {
>> -               if (starts_with(val, "imap:"))
>> -                       val += 5;
>> -               else if (starts_with(val, "imaps:")) {
>> -                       val += 6;
>> +       const char *value;
> 
> Observation: If you name this variable 'val', which is the name of the
> argument to the function in the original code, you will get a slightly
> smaller and more readable diff. 

Noted.

> 
>> +       if (!git_config_get_string("imap.sslverify", &value))
>> +               server.ssl_verify = git_config_bool("sslverify", value);
> 
> I realize that you are just replicating the behavior of the original
> code, but the error message emitted here for a non-bool value is less
> than desirable since it throws away context (namely, the "imap."
> prefix). You can improve the message, and help the user resolve the
> error more quickly, by presenting the full configuration key (namely,
> "imap.sslverify"). Such a change would deserve mention in the commit
> message. Alternately, it could be fixed in a follow-up patch.
> 

Yes, I thought so also when writing the patch. Will change it in the next
iteration.

Thanks.
Tanay Abhra.

>> +       if (!git_config_get_string("imap.preformattedhtml", &value))
>> +               server.use_html = git_config_bool("preformattedhtml", value);
> 
> Ditto regarding error message: "imap.preformattedhtml"
> 
>> +       if (!git_config_get_string("imap.folder", &value))
>> +               imap_folder = xstrdup(value);
>> +       if (!git_config_get_string("imap.host", &value)) {
>> +               if (starts_with(value, "imap:"))
>> +                       value += 5;
>> +               else if (starts_with(value, "imaps:")) {
>> +                       value += 6;
>>                         server.use_ssl = 1;
>>                 }
>> -               if (starts_with(val, "//"))
>> -                       val += 2;
>> -               server.host = xstrdup(val);
>> -       } else if (!strcmp("user", key))
>> -               server.user = xstrdup(val);
>> -       else if (!strcmp("pass", key))
>> -               server.pass = xstrdup(val);
>> -       else if (!strcmp("port", key))
>> -               server.port = git_config_int(key, val);
>> -       else if (!strcmp("tunnel", key))
>> -               server.tunnel = xstrdup(val);
>> -       else if (!strcmp("authmethod", key))
>> -               server.auth_method = xstrdup(val);
>> -
>> -       return 0;
>> +               if (starts_with(value, "//"))
>> +                       value += 2;
>> +               server.host = xstrdup(value);
>> +       }
>> +       if (!git_config_get_string("imap.user", &value))
>> +               server.user = xstrdup(value);
>> +       if (!git_config_get_string("imap.pass", &value))
>> +               server.pass = xstrdup(value);
>> +       if (!git_config_get_string("imap.port", &value))
>> +               server.port = git_config_int("port", value);
> 
> Same regarding diagnostic: "imap.port"
> 
>> +       if (!git_config_get_string("imap.tunnel", &value))
>> +               server.tunnel = xstrdup(value);
>> +       if (!git_config_get_string("imap.authmethod", &value))
>> +               server.auth_method = xstrdup(value);
>>  }
>>
>>  int main(int argc, char **argv)
>> @@ -1387,7 +1377,7 @@ int main(int argc, char **argv)
>>                 usage(imap_send_usage);
>>
>>         setup_git_directory_gently(&nongit_ok);
>> -       git_config(git_imap_config, NULL);
>> +       git_imap_config();
>>
>>         if (!server.port)
>>                 server.port = server.use_ssl ? 993 : 143;
>> --
>> 1.9.0.GIT
> 

Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +	if (!git_config_get_string("imap.user", &value))
> +		server.user = xstrdup(value);
> +	if (!git_config_get_string("imap.pass", &value))
> +		server.pass = xstrdup(value);
> +	if (!git_config_get_string("imap.port", &value))
> +		server.port = git_config_int("port", value);
> +	if (!git_config_get_string("imap.tunnel", &value))
> +		server.tunnel = xstrdup(value);
> +	if (!git_config_get_string("imap.authmethod", &value))
> +		server.auth_method = xstrdup(value);

Given this kind of systematic code, I find it very tempting to factor
this with a new helper function as

...
git_config_get_string_dup("imap.tunnel", &server.tunnel)
git_config_get_string_dup("imap.authmethod", &server.auth_method)

Is there any reason not to do so?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 26.06.2014 18:50, schrieb Matthieu Moy:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> +	if (!git_config_get_string("imap.user", &value))
>> +		server.user = xstrdup(value);
>> +	if (!git_config_get_string("imap.pass", &value))
>> +		server.pass = xstrdup(value);
>> +	if (!git_config_get_string("imap.port", &value))
>> +		server.port = git_config_int("port", value);
>> +	if (!git_config_get_string("imap.tunnel", &value))
>> +		server.tunnel = xstrdup(value);
>> +	if (!git_config_get_string("imap.authmethod", &value))
>> +		server.auth_method = xstrdup(value);
> 
> Given this kind of systematic code, I find it very tempting to factor
> this with a new helper function as
> 
> ...
> git_config_get_string_dup("imap.tunnel", &server.tunnel)
> git_config_get_string_dup("imap.authmethod", &server.auth_method)
> 
> Is there any reason not to do so?
> 


With a pull-style API, you no longer need global variables for
everything, so IMO the helper functions should _return_ the values
rather than taking an output parameter.

E.g. with helper functions as suggested here [1] we could have:

  if (git_config_get_bool("imap.preformattedhtml", 0))
    wrap_in_html(&msg);

...rather than needing an extra variable:

  int bool_value;
  git_config_get_bool("imap.preformattedhtml", &bool_value);
  if (bool_value)
    wrap_in_html(&msg);

...and specify default values along with their respective keys:

  server.ssl_verify = git_config_get_bool("imap.sslverify", 1);
  server.port = git_config_get_int("imap.port", server.use_ssl ? 993 : 143);

...rather than ~1300 lines apart (yuck):

  static struct imap_server_conf server = {
    NULL,  /* name */
    NULL,  /* tunnel */
    NULL,  /* host */
    0,     /* port */
    NULL,  /* user */
    NULL,  /* pass */
    0,     /* use_ssl */
    1,     /* ssl_verify */
    0,     /* use_html */
    NULL,  /* auth_method */
  };


Regarding xstrdup(), I think this is a remnant from the callback
version, which _requires_ you to xstrdup() (the value parameter is
invalidated after returning from the callback).

Side note: with the current callback design, config variables may
get passed to the callback multiple times (last value wins), so
each xstrdup() in current 'git_*_config' functions actually
causes memory leaks (unless prefixed with 'free(my_config_var);').

[1] http://git.661346.n2.nabble.com/PATCH-v3-0-3-git-config-cache-special-querying-api-utilizing-the-cache-tp7613911p7614050.html

[RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Tanay Abhra]

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 notes-utils.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
 		return NULL;
 }
 
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
 {
-	struct notes_rewrite_cfg *c = cb;
-	if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
-		c->enabled = git_config_bool(k, v);
-		return 0;
-	} else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
+	struct strbuf key = STRBUF_INIT;
+	const char *v;
+	strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
+
+	if (!git_config_get_string(key.buf, &v))
+		c->enabled = git_config_bool(key.buf, v);
+
+	if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
 		if (!v)
-			return config_error_nonbool(k);
+			config_error_nonbool("notes.rewritemode");
 		c->combine = parse_combine_notes_fn(v);
-		if (!c->combine) {
+		if (!c->combine)
 			error(_("Bad notes.rewriteMode value: '%s'"), v);
-			return 1;
-		}
-		return 0;
-	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
+	}
+	if (!c->refs_from_env && !git_config_get_string("notes.rewriteref", &v)) {
 		/* note that a refs/ prefix is implied in the
 		 * underlying for_each_glob_ref */
 		if (starts_with(v, "refs/notes/"))
@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
 		else
 			warning(_("Refusing to rewrite notes in %s"
 				" (outside of refs/notes/)"), v);
-		return 0;
 	}
-
-	return 0;
+	strbuf_release(&key);
 }
 
 
@@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 		c->refs_from_env = 1;
 		string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
 	}
-	git_config(notes_rewrite_config, c);
+	notes_rewrite_config(c);
 	if (!c->enabled || !c->refs->nr) {
 		string_list_clear(c->refs, 0);
 		free(c->refs);
-- 
1.9.0.GIT

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Use git_config_get_string instead of git_config to take advantage of
> the config hash-table api which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  notes-utils.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/notes-utils.c b/notes-utils.c
> index a0b1d7b..fdc9912 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>                 return NULL;
>  }
>
> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>  {
> -       struct notes_rewrite_cfg *c = cb;
> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
> -               c->enabled = git_config_bool(k, v);
> -               return 0;
> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
> +       struct strbuf key = STRBUF_INIT;
> +       const char *v;
> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
> +
> +       if (!git_config_get_string(key.buf, &v))
> +               c->enabled = git_config_bool(key.buf, v);
> +
> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>                 if (!v)
> -                       return config_error_nonbool(k);
> +                       config_error_nonbool("notes.rewritemode");

There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.

>                 c->combine = parse_combine_notes_fn(v);

Worse: Though you correctly emit an error when 'v' is NULL, you then
(incorrectly) invoke parse_combine_notes_fn() with that NULL value,
which will result in a crash.

> -               if (!c->combine) {
> +               if (!c->combine)
>                         error(_("Bad notes.rewriteMode value: '%s'"), v);
> -                       return 1;
> -               }
> -               return 0;
> -       } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
> +       }
> +       if (!c->refs_from_env && !git_config_get_string("notes.rewriteref", &v)) {
>                 /* note that a refs/ prefix is implied in the
>                  * underlying for_each_glob_ref */
>                 if (starts_with(v, "refs/notes/"))
> @@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
>                 else
>                         warning(_("Refusing to rewrite notes in %s"
>                                 " (outside of refs/notes/)"), v);
> -               return 0;
>         }
> -
> -       return 0;
> +       strbuf_release(&key);

It would be better to release the strbuf immediately after its final
use rather than waiting until the end of function. Not only does that
reduce cognitive load on people reading the code, but it also reduces
likelihood of 'key' being leaked if some future programmer inserts an
early 'return' into the function for some reason.

>  }
>
>
> @@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
>                 c->refs_from_env = 1;
>                 string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
>         }
> -       git_config(notes_rewrite_config, c);
> +       notes_rewrite_config(c);
>         if (!c->enabled || !c->refs->nr) {
>                 string_list_clear(c->refs, 0);
>                 free(c->refs);
> --
> 1.9.0.GIT

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/25/2014 1:24 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  notes-utils.c | 31 +++++++++++++++----------------
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/notes-utils.c b/notes-utils.c
>> index a0b1d7b..fdc9912 100644
>> --- a/notes-utils.c
>> +++ b/notes-utils.c
>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>                 return NULL;
>>  }
>>
>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>  {
>> -       struct notes_rewrite_cfg *c = cb;
>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>> -               c->enabled = git_config_bool(k, v);
>> -               return 0;
>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>> +       struct strbuf key = STRBUF_INIT;
>> +       const char *v;
>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>> +
>> +       if (!git_config_get_string(key.buf, &v))
>> +               c->enabled = git_config_bool(key.buf, v);
>> +
>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>                 if (!v)
>> -                       return config_error_nonbool(k);
>> +                       config_error_nonbool("notes.rewritemode");
> 
> There's a behavior change here. In the original code, the callback
> function would return -1, which would cause the program to die() if
> the config.c:die_on_error flag was set. The new code merely emits an
> error.
> 

Is this change serious enough? Can I ignore it?

>>                 c->combine = parse_combine_notes_fn(v);
> 
> Worse: Though you correctly emit an error when 'v' is NULL, you then
> (incorrectly) invoke parse_combine_notes_fn() with that NULL value,
> which will result in a crash.
> 

Noted.

>> -               if (!c->combine) {
>> +               if (!c->combine)
>>                         error(_("Bad notes.rewriteMode value: '%s'"), v);
>> -                       return 1;
>> -               }
>> -               return 0;
>> -       } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
>> +       }
>> +       if (!c->refs_from_env && !git_config_get_string("notes.rewriteref", &v)) {
>>                 /* note that a refs/ prefix is implied in the
>>                  * underlying for_each_glob_ref */
>>                 if (starts_with(v, "refs/notes/"))
>> @@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>                 else
>>                         warning(_("Refusing to rewrite notes in %s"
>>                                 " (outside of refs/notes/)"), v);
>> -               return 0;
>>         }
>> -
>> -       return 0;
>> +       strbuf_release(&key);
> 
> It would be better to release the strbuf immediately after its final
> use rather than waiting until the end of function. Not only does that
> reduce cognitive load on people reading the code, but it also reduces
> likelihood of 'key' being leaked if some future programmer inserts an
> early 'return' into the function for some reason.
> 

Noted. Thanks.

>>  }
>>
>>
>> @@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
>>                 c->refs_from_env = 1;
>>                 string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
>>         }
>> -       git_config(notes_rewrite_config, c);
>> +       notes_rewrite_config(c);
>>         if (!c->enabled || !c->refs->nr) {
>>                 string_list_clear(c->refs, 0);
>>                 free(c->refs);
>> --
>> 1.9.0.GIT
> 

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>> Use git_config_get_string instead of git_config to take advantage of
>>> the config hash-table api which provides a cleaner control flow.
>>>
>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>> ---
>>>  notes-utils.c | 31 +++++++++++++++----------------
>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/notes-utils.c b/notes-utils.c
>>> index a0b1d7b..fdc9912 100644
>>> --- a/notes-utils.c
>>> +++ b/notes-utils.c
>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>>                 return NULL;
>>>  }
>>>
>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>  {
>>> -       struct notes_rewrite_cfg *c = cb;
>>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>> -               c->enabled = git_config_bool(k, v);
>>> -               return 0;
>>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>>> +       struct strbuf key = STRBUF_INIT;
>>> +       const char *v;
>>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>> +
>>> +       if (!git_config_get_string(key.buf, &v))
>>> +               c->enabled = git_config_bool(key.buf, v);
>>> +
>>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>>                 if (!v)
>>> -                       return config_error_nonbool(k);
>>> +                       config_error_nonbool("notes.rewritemode");
>>
>> There's a behavior change here. In the original code, the callback
>> function would return -1, which would cause the program to die() if
>> the config.c:die_on_error flag was set. The new code merely emits an
>> error.
>
> Is this change serious enough? Can I ignore it?

I don't know. Even within this single function there is no consistency
about whether such problems should die() or just emit a message and
continue. For instance:

- if "notes.rewritemode" is bool, it die()s.

- if "notes.rewritemode" doesn't specify a recognized mode, it
error()s but continues

- if "notes.rewriteref" doesn't start with "refs/notes/, it warning()s
and continues

It would be nice to hear an opinion from someone more invested in the
config system.

>>>                 c->combine = parse_combine_notes_fn(v);
>>
>> Worse: Though you correctly emit an error when 'v' is NULL, you then
>> (incorrectly) invoke parse_combine_notes_fn() with that NULL value,
>> which will result in a crash.
>>
>
> Noted.
>
>>> -               if (!c->combine) {
>>> +               if (!c->combine)
>>>                         error(_("Bad notes.rewriteMode value: '%s'"), v);
>>> -                       return 1;
>>> -               }
>>> -               return 0;
>>> -       } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
>>> +       }
>>> +       if (!c->refs_from_env && !git_config_get_string("notes.rewriteref", &v)) {
>>>                 /* note that a refs/ prefix is implied in the
>>>                  * underlying for_each_glob_ref */
>>>                 if (starts_with(v, "refs/notes/"))
>>> @@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>>                 else
>>>                         warning(_("Refusing to rewrite notes in %s"
>>>                                 " (outside of refs/notes/)"), v);
>>> -               return 0;
>>>         }
>>> -
>>> -       return 0;
>>> +       strbuf_release(&key);
>>
>> It would be better to release the strbuf immediately after its final
>> use rather than waiting until the end of function. Not only does that
>> reduce cognitive load on people reading the code, but it also reduces
>> likelihood of 'key' being leaked if some future programmer inserts an
>> early 'return' into the function for some reason.
>>
>
> Noted. Thanks.
>
>>>  }
>>>
>>>
>>> @@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
>>>                 c->refs_from_env = 1;
>>>                 string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
>>>         }
>>> -       git_config(notes_rewrite_config, c);
>>> +       notes_rewrite_config(c);
>>>         if (!c->enabled || !c->refs->nr) {
>>>                 string_list_clear(c->refs, 0);
>>>                 free(c->refs);
>>> --
>>> 1.9.0.GIT
>>

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 29.06.2014 13:01, schrieb Eric Sunshine:
> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>> Use git_config_get_string instead of git_config to take advantage of
>>>> the config hash-table api which provides a cleaner control flow.
>>>>
>>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>>> ---
>>>>  notes-utils.c | 31 +++++++++++++++----------------
>>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/notes-utils.c b/notes-utils.c
>>>> index a0b1d7b..fdc9912 100644
>>>> --- a/notes-utils.c
>>>> +++ b/notes-utils.c
>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>>>                 return NULL;
>>>>  }
>>>>
>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>>  {
>>>> -       struct notes_rewrite_cfg *c = cb;
>>>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>>> -               c->enabled = git_config_bool(k, v);
>>>> -               return 0;
>>>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>>>> +       struct strbuf key = STRBUF_INIT;
>>>> +       const char *v;
>>>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>>> +
>>>> +       if (!git_config_get_string(key.buf, &v))
>>>> +               c->enabled = git_config_bool(key.buf, v);
>>>> +
>>>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>>>                 if (!v)
>>>> -                       return config_error_nonbool(k);
>>>> +                       config_error_nonbool("notes.rewritemode");
>>>
>>> There's a behavior change here. In the original code, the callback
>>> function would return -1, which would cause the program to die() if
>>> the config.c:die_on_error flag was set. The new code merely emits an
>>> error.
>>
>> Is this change serious enough? Can I ignore it?

IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).

> 
> I don't know. Even within this single function there is no consistency
> about whether such problems should die() or just emit a message and
> continue. For instance:
> 
> - if "notes.rewritemode" is bool, it die()s.
> 
> - if "notes.rewritemode" doesn't specify a recognized mode, it
> error()s but continues
> 

I think this would also die in git_parse_source():
...
    if (get_value(fn, data, var) < 0)
      break;
  }
  if (cf->die_on_error)
    die("bad config file line %d in %s", cf->linenr, cf->name);
...

(AFAICT, die_on_error is always true, except if invoked via 'git-config
--blob', which isn't used anywhere...)


This, however, raises another issue: switching to the config cache looses
file/line-precise error reporting for semantic errors. I don't know if
this feature is important enough to do something about it, though. A
message of the form "Key 'xyz' is bad" should usually enable a user to
locate the problematic file and line.

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>>> Use git_config_get_string instead of git_config to take advantage of
>>>>> the config hash-table api which provides a cleaner control flow.
>>>>>
>>>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>>>> ---
>>>>>  notes-utils.c | 31 +++++++++++++++----------------
>>>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/notes-utils.c b/notes-utils.c
>>>>> index a0b1d7b..fdc9912 100644
>>>>> --- a/notes-utils.c
>>>>> +++ b/notes-utils.c
>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>>>>                 return NULL;
>>>>>  }
>>>>>
>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>>>  {
>>>>> -       struct notes_rewrite_cfg *c = cb;
>>>>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>>>> -               c->enabled = git_config_bool(k, v);
>>>>> -               return 0;
>>>>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>>>>> +       struct strbuf key = STRBUF_INIT;
>>>>> +       const char *v;
>>>>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>>>> +
>>>>> +       if (!git_config_get_string(key.buf, &v))
>>>>> +               c->enabled = git_config_bool(key.buf, v);
>>>>> +
>>>>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>>>>                 if (!v)
>>>>> -                       return config_error_nonbool(k);
>>>>> +                       config_error_nonbool("notes.rewritemode");
>>>>
>>>> There's a behavior change here. In the original code, the callback
>>>> function would return -1, which would cause the program to die() if
>>>> the config.c:die_on_error flag was set. The new code merely emits an
>>>> error.
>>>
>>> Is this change serious enough? Can I ignore it?
>
> IMO its better to Fail Fast than continue with some invalid config (which
> may lead to more severe errors such as data corruption / data loss).
>
>>
>> I don't know. Even within this single function there is no consistency
>> about whether such problems should die() or just emit a message and
>> continue. For instance:
>>
>> - if "notes.rewritemode" is bool, it die()s.
>>
>> - if "notes.rewritemode" doesn't specify a recognized mode, it
>> error()s but continues
>>
>
> I think this would also die in git_parse_source():
> ...
>     if (get_value(fn, data, var) < 0)
>       break;
>   }
>   if (cf->die_on_error)
>     die("bad config file line %d in %s", cf->linenr, cf->name);
> ...

One would expect so, but notes-utils.c:notes_rewrite_config() is
actually doing this:

    if (!c->combine) {
        error(_("Bad notes.rewriteMode value: '%s'"), v);
        return 1;
    }

Rather than returning the -1 result of error(), which would make
git_parse_source() die(), it's explicitly returning 1, which
get_parse_source() ignores.

> (AFAICT, die_on_error is always true, except if invoked via 'git-config
> --blob', which isn't used anywhere...)
>
> This, however, raises another issue: switching to the config cache looses
> file/line-precise error reporting for semantic errors. I don't know if
> this feature is important enough to do something about it, though. A
> message of the form "Key 'xyz' is bad" should usually enable a user to
> locate the problematic file and line.

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 30.06.2014 16:32, schrieb Eric Sunshine:
> On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
>> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>>>> Use git_config_get_string instead of git_config to take advantage of
>>>>>> the config hash-table api which provides a cleaner control flow.
>>>>>>
>>>>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>>>>> ---
>>>>>>  notes-utils.c | 31 +++++++++++++++----------------
>>>>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/notes-utils.c b/notes-utils.c
>>>>>> index a0b1d7b..fdc9912 100644
>>>>>> --- a/notes-utils.c
>>>>>> +++ b/notes-utils.c
>>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>>>>>                 return NULL;
>>>>>>  }
>>>>>>
>>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>>>>  {
>>>>>> -       struct notes_rewrite_cfg *c = cb;
>>>>>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>>>>> -               c->enabled = git_config_bool(k, v);
>>>>>> -               return 0;
>>>>>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>>>>>> +       struct strbuf key = STRBUF_INIT;
>>>>>> +       const char *v;
>>>>>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>>>>> +
>>>>>> +       if (!git_config_get_string(key.buf, &v))
>>>>>> +               c->enabled = git_config_bool(key.buf, v);
>>>>>> +
>>>>>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>>>>>                 if (!v)
>>>>>> -                       return config_error_nonbool(k);
>>>>>> +                       config_error_nonbool("notes.rewritemode");
>>>>>
>>>>> There's a behavior change here. In the original code, the callback
>>>>> function would return -1, which would cause the program to die() if
>>>>> the config.c:die_on_error flag was set. The new code merely emits an
>>>>> error.
>>>>
>>>> Is this change serious enough? Can I ignore it?
>>
>> IMO its better to Fail Fast than continue with some invalid config (which
>> may lead to more severe errors such as data corruption / data loss).
>>
>>>
>>> I don't know. Even within this single function there is no consistency
>>> about whether such problems should die() or just emit a message and
>>> continue. For instance:
>>>
>>> - if "notes.rewritemode" is bool, it die()s.
>>>
>>> - if "notes.rewritemode" doesn't specify a recognized mode, it
>>> error()s but continues
>>>
>>
>> I think this would also die in git_parse_source():
>> ...
>>     if (get_value(fn, data, var) < 0)
>>       break;
>>   }
>>   if (cf->die_on_error)
>>     die("bad config file line %d in %s", cf->linenr, cf->name);
>> ...
> 
> One would expect so, but notes-utils.c:notes_rewrite_config() is
> actually doing this:
> 
>     if (!c->combine) {
>         error(_("Bad notes.rewriteMode value: '%s'"), v);
>         return 1;
>     }
> 
> Rather than returning the -1 result of error(), which would make
> git_parse_source() die(), it's explicitly returning 1, which
> get_parse_source() ignores.
> 

Ahh...I missed the '< 0', sorry.

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/30/2014 7:04 PM, Karsten Blees wrote:
> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>>> Use git_config_get_string instead of git_config to take advantage of
>>>>> the config hash-table api which provides a cleaner control flow.
>>>>>
>>>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>>>> ---
>>>>>  notes-utils.c | 31 +++++++++++++++----------------
>>>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/notes-utils.c b/notes-utils.c
>>>>> index a0b1d7b..fdc9912 100644
>>>>> --- a/notes-utils.c
>>>>> +++ b/notes-utils.c
>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>>>>                 return NULL;
>>>>>  }
>>>>>
>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>>>  {
>>>>> -       struct notes_rewrite_cfg *c = cb;
>>>>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>>>> -               c->enabled = git_config_bool(k, v);
>>>>> -               return 0;
>>>>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>>>>> +       struct strbuf key = STRBUF_INIT;
>>>>> +       const char *v;
>>>>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>>>> +
>>>>> +       if (!git_config_get_string(key.buf, &v))
>>>>> +               c->enabled = git_config_bool(key.buf, v);
>>>>> +
>>>>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>>>>                 if (!v)
>>>>> -                       return config_error_nonbool(k);
>>>>> +                       config_error_nonbool("notes.rewritemode");
>>>>
>>>> There's a behavior change here. In the original code, the callback
>>>> function would return -1, which would cause the program to die() if
>>>> the config.c:die_on_error flag was set. The new code merely emits an
>>>> error.
>>>
>>> Is this change serious enough? Can I ignore it?
> 
> IMO its better to Fail Fast than continue with some invalid config (which
> may lead to more severe errors such as data corruption / data loss).

Noted but, what I am trying to do with the rewrite is emit an error and
not set the value if the value found is a NULL. The only change is that
program will not crash in this case and warn the user not set a NULL value for
a non boolean key.
This won't lead to severe errors as the value will not be set if found value
is a NULL.

>>
>> I don't know. Even within this single function there is no consistency
>> about whether such problems should die() or just emit a message and
>> continue. For instance:
>>
>> - if "notes.rewritemode" is bool, it die()s.
>>
>> - if "notes.rewritemode" doesn't specify a recognized mode, it
>> error()s but continues
>>
> 
> I think this would also die in git_parse_source():
> ...
>     if (get_value(fn, data, var) < 0)
>       break;
>   }
>   if (cf->die_on_error)
>     die("bad config file line %d in %s", cf->linenr, cf->name);
> ...
> 
> (AFAICT, die_on_error is always true, except if invoked via 'git-config
> --blob', which isn't used anywhere...)
> 

Noted.

> This, however, raises another issue: switching to the config cache looses
> file/line-precise error reporting for semantic errors. I don't know if
> this feature is important enough to do something about it, though. A
> message of the form "Key 'xyz' is bad" should usually enable a user to
> locate the problematic file and line.
> 

Hmn, but during the config cache construction we parse key-value pairs through
git_config() which still warns users about semantic errors. This happened
yesterday only when I was writing tests for the new API.

Thanks for the review.

Cheers,
Tanay Abhra.

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 30.06.2014 16:39, schrieb Tanay Abhra:
> 
> On 6/30/2014 7:04 PM, Karsten Blees wrote:
>> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>>>> Use git_config_get_string instead of git_config to take advantage of
>>>>>> the config hash-table api which provides a cleaner control flow.
>>>>>>
>>>>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>>>>> ---
>>>>>>  notes-utils.c | 31 +++++++++++++++----------------
>>>>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/notes-utils.c b/notes-utils.c
>>>>>> index a0b1d7b..fdc9912 100644
>>>>>> --- a/notes-utils.c
>>>>>> +++ b/notes-utils.c
>>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>>>>>                 return NULL;
>>>>>>  }
>>>>>>
>>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>>>>  {
>>>>>> -       struct notes_rewrite_cfg *c = cb;
>>>>>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>>>>> -               c->enabled = git_config_bool(k, v);
>>>>>> -               return 0;
>>>>>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>>>>>> +       struct strbuf key = STRBUF_INIT;
>>>>>> +       const char *v;
>>>>>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>>>>> +
>>>>>> +       if (!git_config_get_string(key.buf, &v))
>>>>>> +               c->enabled = git_config_bool(key.buf, v);
>>>>>> +
>>>>>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>>>>>                 if (!v)
>>>>>> -                       return config_error_nonbool(k);
>>>>>> +                       config_error_nonbool("notes.rewritemode");
>>>>>
>>>>> There's a behavior change here. In the original code, the callback
>>>>> function would return -1, which would cause the program to die() if
>>>>> the config.c:die_on_error flag was set. The new code merely emits an
>>>>> error.
>>>>
>>>> Is this change serious enough? Can I ignore it?
>>
>> IMO its better to Fail Fast than continue with some invalid config (which
>> may lead to more severe errors such as data corruption / data loss).
> 
> Noted but, what I am trying to do with the rewrite is emit an error and
> not set the value if the value found is a NULL.

If you don't set the value and continue, git will proceed with the variable's
default setting.

Which may not be too harmful in some cases, but if a user changes:

 gc.pruneexpire=4.weeks.ago

to

 gc.pruneexpire=4.monhts.ago

(note the typo), the next git-gc will warn the user and then happily throw
away data that the user intended to keep (default is 2.weeks.ago).

Thus I think git should die() if it encounters an invalid config setting.

> 
>> This, however, raises another issue: switching to the config cache looses
>> file/line-precise error reporting for semantic errors. I don't know if
>> this feature is important enough to do something about it, though. A
>> message of the form "Key 'xyz' is bad" should usually enable a user to
>> locate the problematic file and line.
>>
> 
> Hmn, but during the config cache construction we parse key-value pairs through
> git_config() which still warns users about semantic errors.

If I'm not mistaken you only detect _syntax_ errors when loading the file (i.e.
whether the config file is structurally correct).

The semantic value and correctness of a key (e.g. whether its a boolean or an
int or a string that denotes a known merge algorithm) is only checked when it is
accessed via git_config_get_<type>. And at this point, <file>:<line> information
is already lost.

With the callback approach, both syntactic (structure) and semantic (meaning)
errors were checked at load time, resulting in

  die("bad config file line %d in %s", cf->linenr, cf->name);

if the callback returned -1.

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/30/2014 9:26 PM, Karsten Blees wrote:
> Am 30.06.2014 16:39, schrieb Tanay Abhra:
>>
>> On 6/30/2014 7:04 PM, Karsten Blees wrote:
>>> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>>>>> Use git_config_get_string instead of git_config to take advantage of
>>>>>>> the config hash-table api which provides a cleaner control flow.
>>>>>>>
>>>>>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>>>>>> ---
>>>>>>>  notes-utils.c | 31 +++++++++++++++----------------
>>>>>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>>>>>
>>>>>>> diff --git a/notes-utils.c b/notes-utils.c
>>>>>>> index a0b1d7b..fdc9912 100644
>>>>>>> --- a/notes-utils.c
>>>>>>> +++ b/notes-utils.c
>>>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>>>>>>                 return NULL;
>>>>>>>  }
>>>>>>>
>>>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>>>>>  {
>>>>>>> -       struct notes_rewrite_cfg *c = cb;
>>>>>>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>>>>>> -               c->enabled = git_config_bool(k, v);
>>>>>>> -               return 0;
>>>>>>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>>>>>>> +       struct strbuf key = STRBUF_INIT;
>>>>>>> +       const char *v;
>>>>>>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>>>>>> +
>>>>>>> +       if (!git_config_get_string(key.buf, &v))
>>>>>>> +               c->enabled = git_config_bool(key.buf, v);
>>>>>>> +
>>>>>>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>>>>>>                 if (!v)
>>>>>>> -                       return config_error_nonbool(k);
>>>>>>> +                       config_error_nonbool("notes.rewritemode");
>>>>>>
>>>>>> There's a behavior change here. In the original code, the callback
>>>>>> function would return -1, which would cause the program to die() if
>>>>>> the config.c:die_on_error flag was set. The new code merely emits an
>>>>>> error.
>>>>>
>>>>> Is this change serious enough? Can I ignore it?
>>>
>>> IMO its better to Fail Fast than continue with some invalid config (which
>>> may lead to more severe errors such as data corruption / data loss).
>>
>> Noted but, what I am trying to do with the rewrite is emit an error and
>> not set the value if the value found is a NULL.
> 
> If you don't set the value and continue, git will proceed with the variable's
> default setting.
> 
> Which may not be too harmful in some cases, but if a user changes:
> 
>  gc.pruneexpire=4.weeks.ago
> 
> to
> 
>  gc.pruneexpire=4.monhts.ago
> 
> (note the typo), the next git-gc will warn the user and then happily throw
> away data that the user intended to keep (default is 2.weeks.ago).
> 
> Thus I think git should die() if it encounters an invalid config setting.

Okay, point noted.

>>
>>> This, however, raises another issue: switching to the config cache looses
>>> file/line-precise error reporting for semantic errors. I don't know if
>>> this feature is important enough to do something about it, though. A
>>> message of the form "Key 'xyz' is bad" should usually enable a user to
>>> locate the problematic file and line.
>>>
>>
>> Hmn, but during the config cache construction we parse key-value pairs through
>> git_config() which still warns users about semantic errors.
> 
> If I'm not mistaken you only detect _syntax_ errors when loading the file (i.e.
> whether the config file is structurally correct).
> 
> The semantic value and correctness of a key (e.g. whether its a boolean or an
> int or a string that denotes a known merge algorithm) is only checked when it is
> accessed via git_config_get_<type>. And at this point, <file>:<line> information
> is already lost.
> 
> With the callback approach, both syntactic (structure) and semantic (meaning)
> errors were checked at load time, resulting in
> 
>   die("bad config file line %d in %s", cf->linenr, cf->name);
> 
> if the callback returned -1.
> 

Yup, you are right, we check only syntax error when loading the file for the cache.
I could save the <filename>:<linenr> when the loading the file for future error
reporting. Thanks.

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Junio C Hamano]

Karsten Blees <karsten.blees@gmail.com> writes:

> Which may not be too harmful in some cases, but if a user changes:
>
>  gc.pruneexpire=4.weeks.ago
>
> to
>
>  gc.pruneexpire=4.monhts.ago
>
> (note the typo), the next git-gc will warn the user and then happily throw
> away data that the user intended to keep (default is 2.weeks.ago).
>
> Thus I think git should die() if it encounters an invalid config setting.

Yes but not at the parsing time.  Only when we are about to _use_
the value for pruneexpire as a time duration we should die of the
error.  Diagnosing an error early is a separate matter, but if the
operation does not care about the typo we shouldn't die.

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 6/30/2014 7:04 PM, Karsten Blees wrote:
>> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>>>> Use git_config_get_string instead of git_config to take advantage of
>>>>>> the config hash-table api which provides a cleaner control flow.
>>>>>>
>>>>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>>>>> ---
>>>>>>  notes-utils.c | 31 +++++++++++++++----------------
>>>>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/notes-utils.c b/notes-utils.c
>>>>>> index a0b1d7b..fdc9912 100644
>>>>>> --- a/notes-utils.c
>>>>>> +++ b/notes-utils.c
>>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
>>>>>>                 return NULL;
>>>>>>  }
>>>>>>
>>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>>>>  {
>>>>>> -       struct notes_rewrite_cfg *c = cb;
>>>>>> -       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>>>>> -               c->enabled = git_config_bool(k, v);
>>>>>> -               return 0;
>>>>>> -       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>>>>>> +       struct strbuf key = STRBUF_INIT;
>>>>>> +       const char *v;
>>>>>> +       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>>>>> +
>>>>>> +       if (!git_config_get_string(key.buf, &v))
>>>>>> +               c->enabled = git_config_bool(key.buf, v);
>>>>>> +
>>>>>> +       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
>>>>>>                 if (!v)
>>>>>> -                       return config_error_nonbool(k);
>>>>>> +                       config_error_nonbool("notes.rewritemode");
>>>>>
>>>>> There's a behavior change here. In the original code, the callback
>>>>> function would return -1, which would cause the program to die() if
>>>>> the config.c:die_on_error flag was set. The new code merely emits an
>>>>> error.
>>>>
>>>> Is this change serious enough? Can I ignore it?
>> 
>> IMO its better to Fail Fast than continue with some invalid config (which
>> may lead to more severe errors such as data corruption / data loss).
>
> Noted but, what I am trying to do with the rewrite is emit an error and
> not set the value if the value found is a NULL. The only change is that
> program will not crash in this case and warn the user not set a NULL value for
> a non boolean key.
> This won't lead to severe errors as the value will not be set if found value
> is a NULL.

The change probably makes sense, but as much as possible, keep
refactoring patches and patches introducing a semantic change separate.
It's much easier to review, and helps user digging history and finding
one of the commits.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[RFC/PATCH] notes.c: replace git_config with git_config_get_string

[Tanay Abhra]

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 notes.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/notes.c b/notes.c
index 5fe691d..fc92eec 100644
--- a/notes.c
+++ b/notes.c
@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 	free(globs_copy);
 }
 
-static int notes_display_config(const char *k, const char *v, void *cb)
-{
-	int *load_refs = cb;
-
-	if (*load_refs && !strcmp(k, "notes.displayref")) {
-		if (!v)
-			config_error_nonbool(k);
-		string_list_add_refs_by_glob(&display_notes_refs, v);
-	}
-
-	return 0;
-}
-
 const char *default_notes_ref(void)
 {
 	const char *notes_ref = NULL;
@@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 void init_display_notes(struct display_notes_opt *opt)
 {
 	char *display_ref_env;
+	const char *value;
 	int load_config_refs = 0;
 	display_notes_refs.strdup_strings = 1;
 
@@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)
 			load_config_refs = 1;
 	}
 
-	git_config(notes_display_config, &load_config_refs);
+	if (load_config_refs && !git_config_get_string("notes.displayref", &value)) {
+		if (!value)
+			config_error_nonbool("notes.displayref");
+		string_list_add_refs_by_glob(&display_notes_refs, value);
+	}
 
 	if (opt) {
 		struct string_list_item *item;
-- 
1.9.0.GIT

Re: [RFC/PATCH] notes.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Use git_config_get_string instead of git_config to take advantage of
> the config hash-table api which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  notes.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/notes.c b/notes.c
> index 5fe691d..fc92eec 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
>         free(globs_copy);
>  }
>
> -static int notes_display_config(const char *k, const char *v, void *cb)
> -{
> -       int *load_refs = cb;
> -
> -       if (*load_refs && !strcmp(k, "notes.displayref")) {
> -               if (!v)
> -                       config_error_nonbool(k);
> -               string_list_add_refs_by_glob(&display_notes_refs, v);
> -       }
> -
> -       return 0;
> -}
> -
>  const char *default_notes_ref(void)
>  {
>         const char *notes_ref = NULL;
> @@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
>  void init_display_notes(struct display_notes_opt *opt)
>  {
>         char *display_ref_env;
> +       const char *value;
>         int load_config_refs = 0;
>         display_notes_refs.strdup_strings = 1;
>
> @@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)
>                         load_config_refs = 1;
>         }
>
> -       git_config(notes_display_config, &load_config_refs);
> +       if (load_config_refs && !git_config_get_string("notes.displayref", &value)) {
> +               if (!value)
> +                       config_error_nonbool("notes.displayref");
> +               string_list_add_refs_by_glob(&display_notes_refs, value);

Although you correctly diagnose a NULL 'value', you then invoke
string_list_add_refs_by_glob() with that NULL, which will result in a
crash.

This is not a new error. It dates back to 894a9d33 (Support showing
notes from more than one notes tree; 2010-03-12), but your rewrite
should not retain the brokenness. Whether you fix it in this patch or
a lead-in fix-up patch, the fix deserves mention in the commit
message.

> +       }
>
>         if (opt) {
>                 struct string_list_item *item;
> --
> 1.9.0.GIT

Re: [RFC/PATCH] notes.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/25/2014 1:36 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  notes.c | 20 ++++++--------------
>>  1 file changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/notes.c b/notes.c
>> index 5fe691d..fc92eec 100644
>> --- a/notes.c
>> +++ b/notes.c
>> @@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
>>         free(globs_copy);
>>  }
>>
>> -static int notes_display_config(const char *k, const char *v, void *cb)
>> -{
>> -       int *load_refs = cb;
>> -
>> -       if (*load_refs && !strcmp(k, "notes.displayref")) {
>> -               if (!v)
>> -                       config_error_nonbool(k);
>> -               string_list_add_refs_by_glob(&display_notes_refs, v);
>> -       }
>> -
>> -       return 0;
>> -}
>> -
>>  const char *default_notes_ref(void)
>>  {
>>         const char *notes_ref = NULL;
>> @@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
>>  void init_display_notes(struct display_notes_opt *opt)
>>  {
>>         char *display_ref_env;
>> +       const char *value;
>>         int load_config_refs = 0;
>>         display_notes_refs.strdup_strings = 1;
>>
>> @@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)
>>                         load_config_refs = 1;
>>         }
>>
>> -       git_config(notes_display_config, &load_config_refs);
>> +       if (load_config_refs && !git_config_get_string("notes.displayref", &value)) {
>> +               if (!value)
>> +                       config_error_nonbool("notes.displayref");
>> +               string_list_add_refs_by_glob(&display_notes_refs, value);
> 
> Although you correctly diagnose a NULL 'value', you then invoke
> string_list_add_refs_by_glob() with that NULL, which will result in a
> crash.
> 
> This is not a new error. It dates back to 894a9d33 (Support showing
> notes from more than one notes tree; 2010-03-12), but your rewrite
> should not retain the brokenness. Whether you fix it in this patch or
> a lead-in fix-up patch, the fix deserves mention in the commit
> message.

Done. Thanks.

>> +       }
>>
>>         if (opt) {
>>                 struct string_list_item *item;
>> --
>> 1.9.0.GIT
> 

[RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Tanay Abhra]

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 pager.c | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/pager.c b/pager.c
index 8b5cbc5..96abe6d 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
 #define DEFAULT_PAGER "less"
 #endif
 
-struct pager_config {
-	const char *cmd;
-	int want;
-	char *value;
-};
-
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
 	return width;
 }
 
-static int pager_command_config(const char *var, const char *value, void *data)
-{
-	struct pager_config *c = data;
-	if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
-		int b = git_config_maybe_bool(var, value);
-		if (b >= 0)
-			c->want = b;
-		else {
-			c->want = 1;
-			c->value = xstrdup(value);
-		}
-	}
-	return 0;
-}
-
 /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
 int check_pager_config(const char *cmd)
 {
-	struct pager_config c;
-	c.cmd = cmd;
-	c.want = -1;
-	c.value = NULL;
-	git_config(pager_command_config, &c);
-	if (c.value)
-		pager_program = c.value;
-	return c.want;
+	struct strbuf key = STRBUF_INIT;
+	int want = -1;
+	const char *value = NULL;
+	strbuf_addf(&key, "pager.%s", cmd);
+	if (!git_config_get_string(key.buf, &value)) {
+		int b = git_config_maybe_bool(key.buf, value);
+		if (b >= 0)
+			want = b;
+		else
+			want = 1;
+	}
+	if (value)
+		pager_program = value;
+	strbuf_release(&key);
+	return want;
 }
-- 
1.9.0.GIT

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Use git_config_get_string instead of git_config to take advantage of
> the config hash-table api which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  pager.c | 44 +++++++++++++++-----------------------------
>  1 file changed, 15 insertions(+), 29 deletions(-)
>
> diff --git a/pager.c b/pager.c
> index 8b5cbc5..96abe6d 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -6,12 +6,6 @@
>  #define DEFAULT_PAGER "less"
>  #endif
>
> -struct pager_config {
> -       const char *cmd;
> -       int want;
> -       char *value;
> -};
> -
>  /*
>   * This is split up from the rest of git so that we can do
>   * something different on Windows.
> @@ -155,30 +149,22 @@ int decimal_width(int number)
>         return width;
>  }
>
> -static int pager_command_config(const char *var, const char *value, void *data)
> -{
> -       struct pager_config *c = data;
> -       if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
> -               int b = git_config_maybe_bool(var, value);
> -               if (b >= 0)
> -                       c->want = b;
> -               else {
> -                       c->want = 1;
> -                       c->value = xstrdup(value);
> -               }
> -       }
> -       return 0;
> -}
> -
>  /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
>  int check_pager_config(const char *cmd)
>  {
> -       struct pager_config c;
> -       c.cmd = cmd;
> -       c.want = -1;
> -       c.value = NULL;
> -       git_config(pager_command_config, &c);
> -       if (c.value)
> -               pager_program = c.value;
> -       return c.want;
> +       struct strbuf key = STRBUF_INIT;
> +       int want = -1;
> +       const char *value = NULL;
> +       strbuf_addf(&key, "pager.%s", cmd);
> +       if (!git_config_get_string(key.buf, &value)) {
> +               int b = git_config_maybe_bool(key.buf, value);
> +               if (b >= 0)
> +                       want = b;
> +               else
> +                       want = 1;
> +       }
> +       if (value)
> +               pager_program = value;

Two issues:

First, why is 'if(value)' standing by itself? Although this works, it
seems to imply that 'value' might be able to become non-NULL by some
mechanism other than the get_config_maybe_bool() call, which means
that people reading this code have to spend extra time trying to
understand the overall logic. If you follow the example of the
original code, where 'value' is only ever set when 'b < 0', then it is
obvious even to the most casual reader that 'pager_program' is
assigned only for that one condition.

Second, don't you want to xstrdup(value) when assigning to
'pager_program'? If you don't, then 'pager_program' will become a
dangling pointer when config_cache_free() is invoked.

> +       strbuf_release(&key);
> +       return want;
>  }
> --
> 1.9.0.GIT

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/25/2014 9:29 AM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  pager.c | 44 +++++++++++++++-----------------------------
>>  1 file changed, 15 insertions(+), 29 deletions(-)
>>
>> diff --git a/pager.c b/pager.c
>> index 8b5cbc5..96abe6d 100644
>> --- a/pager.c
>> +++ b/pager.c
>> @@ -6,12 +6,6 @@
>>  #define DEFAULT_PAGER "less"
>>  #endif
>>
>> -struct pager_config {
>> -       const char *cmd;
>> -       int want;
>> -       char *value;
>> -};
>> -
>>  /*
>>   * This is split up from the rest of git so that we can do
>>   * something different on Windows.
>> @@ -155,30 +149,22 @@ int decimal_width(int number)
>>         return width;
>>  }
>>
>> -static int pager_command_config(const char *var, const char *value, void *data)
>> -{
>> -       struct pager_config *c = data;
>> -       if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
>> -               int b = git_config_maybe_bool(var, value);
>> -               if (b >= 0)
>> -                       c->want = b;
>> -               else {
>> -                       c->want = 1;
>> -                       c->value = xstrdup(value);
>> -               }
>> -       }
>> -       return 0;
>> -}
>> -
>>  /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
>>  int check_pager_config(const char *cmd)
>>  {
>> -       struct pager_config c;
>> -       c.cmd = cmd;
>> -       c.want = -1;
>> -       c.value = NULL;
>> -       git_config(pager_command_config, &c);
>> -       if (c.value)
>> -               pager_program = c.value;
>> -       return c.want;
>> +       struct strbuf key = STRBUF_INIT;
>> +       int want = -1;
>> +       const char *value = NULL;
>> +       strbuf_addf(&key, "pager.%s", cmd);
>> +       if (!git_config_get_string(key.buf, &value)) {
>> +               int b = git_config_maybe_bool(key.buf, value);
>> +               if (b >= 0)
>> +                       want = b;
>> +               else
>> +                       want = 1;
>> +       }
>> +       if (value)
>> +               pager_program = value;
> 
> Two issues:
> 
> First, why is 'if(value)' standing by itself? Although this works, it
> seems to imply that 'value' might be able to become non-NULL by some
> mechanism other than the get_config_maybe_bool() call, which means
> that people reading this code have to spend extra time trying to
> understand the overall logic. If you follow the example of the
> original code, where 'value' is only ever set when 'b < 0', then it is
> obvious even to the most casual reader that 'pager_program' is
> assigned only for that one condition.
> 

Noted.

> Second, don't you want to xstrdup(value) when assigning to
> 'pager_program'? If you don't, then 'pager_program' will become a
> dangling pointer when config_cache_free() is invoked.
> 

Noted. Thanks.

>> +       strbuf_release(&key);
>> +       return want;
>>  }
>> --
>> 1.9.0.GIT
> 

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 25.06.2014 05:59, schrieb Eric Sunshine:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:

[...]

>>  /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
>>  int check_pager_config(const char *cmd)
>>  {
>> -       struct pager_config c;
>> -       c.cmd = cmd;
>> -       c.want = -1;
>> -       c.value = NULL;
>> -       git_config(pager_command_config, &c);
>> -       if (c.value)
>> -               pager_program = c.value;
>> -       return c.want;
>> +       struct strbuf key = STRBUF_INIT;
>> +       int want = -1;
>> +       const char *value = NULL;
>> +       strbuf_addf(&key, "pager.%s", cmd);
>> +       if (!git_config_get_string(key.buf, &value)) {
>> +               int b = git_config_maybe_bool(key.buf, value);
>> +               if (b >= 0)
>> +                       want = b;
>> +               else
>> +                       want = 1;
>> +       }
>> +       if (value)
>> +               pager_program = value;

[...]
> 
> Second, don't you want to xstrdup(value) when assigning to
> 'pager_program'? If you don't, then 'pager_program' will become a
> dangling pointer when config_cache_free() is invoked.
> 

I don't think that values from the global config cache should be xstrdup()ed.
After all, caching the values during the lifetime of the git process is the
entire point of the config cache, isn't it?

The only reason to call config_cache_free() is to load a _different_
configuration. In this case, however, you would also need to call the relevant
config functions again, leaking all xstrdup()ed strings.

If for some reason a config string is accessed after config_cache_free()
(which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git
will continue to run with some invalid configuration). This is IMO much worse
than failing with segfault.

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Matthieu Moy]

Karsten Blees <karsten.blees@gmail.com> writes:

> If for some reason a config string is accessed after config_cache_free()
> (which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git
> will continue to run with some invalid configuration). This is IMO much worse
> than failing with segfault.

I disagree. In most case, continuing to use the old config value is the
right thing.

First, config_cache_free() is called whenever _any_ configuration
variable is set. So, setting "core.foo" also invalidates "core.bar" in
the cache, while a user of "core.bar" could continue working with the
old, unchanged value.

Then, allowing the invalidation of a config variable at any time raises
a lot of tricky cases (if a command uses a configuration variable twice,
do we really want to implement and test the behavior for all combination
of old/new values for both usages?). More tricky cases usually means
more bugs on the user-side ...

When the code really want the freshest value, it's cheap to re-query the
config cache anyway.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 27.06.2014 13:55, schrieb Matthieu Moy:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> If for some reason a config string is accessed after config_cache_free()
>> (which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git
>> will continue to run with some invalid configuration). This is IMO much worse
>> than failing with segfault.
> 
> I disagree. In most case, continuing to use the old config value is the
> right thing.
> 
> First, config_cache_free() is called whenever _any_ configuration
> variable is set.

This is illogical. An API should do the right thing, and if the
right thing is to keep the old values, as you pointed out, then
setting a config value shouldn't invalidate the cache (at least
not automatically).

I can think of only git-clone and git-init that may want to refresh
the global config cache, otherwise, config_cache_free() should
probably _never_ be called. Least of all as a side effect of
git_config_set_multivar_in_file(), which is used to write all kinds
of unrelated files that just happen to have config-file format (e.g.
'.gitmodules', '.git/sequencer/opts').

> So, setting "core.foo" also invalidates "core.bar" in
> the cache, while a user of "core.bar" could continue working with the
> old, unchanged value.

But a user of an xstrdup()ed copy of "core.foo" would also continue
to work with the old value. So if everyone keeps copies of config
strings, invalidating the cache when setting a value has no effect.
We could just as well not invalidate the cache and not xstrdup()
strings.

Perhaps we should see this as an opportunity to get rid of all the
memory leaks in current config code (each string value defined in
system / global config and overridden locally will currently be
leaked with the 'standard' xstrdup() pattern).

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Matthieu Moy]

Karsten Blees <karsten.blees@gmail.com> writes:

> Am 27.06.2014 13:55, schrieb Matthieu Moy:
>> Karsten Blees <karsten.blees@gmail.com> writes:
>> 
>>> If for some reason a config string is accessed after config_cache_free()
>>> (which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git
>>> will continue to run with some invalid configuration). This is IMO much worse
>>> than failing with segfault.
>> 
>> I disagree. In most case, continuing to use the old config value is the
>> right thing.
>> 
>> First, config_cache_free() is called whenever _any_ configuration
>> variable is set.
>
> This is illogical. An API should do the right thing, and if the
> right thing is to keep the old values, as you pointed out, then
> setting a config value shouldn't invalidate the cache (at least
> not automatically).

The reason for which setting any config value invalidates the cache is
that it is _much_ simpler to implement. Less complexity, less
corner-cases, less bugs.

>> So, setting "core.foo" also invalidates "core.bar" in
>> the cache, while a user of "core.bar" could continue working with the
>> old, unchanged value.
>
> But a user of an xstrdup()ed copy of "core.foo" would also continue
> to work with the old value.

Exactly like the current code does. Has it ever been even close to being
a problem?

What you're suggesting brings a lot more complexity in the code, and I
can't imagine a case where it would have a real benefit. So, why bother?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 27.06.2014 21:19, schrieb Matthieu Moy:
> 
> The reason for which setting any config value invalidates the cache is
> that it is _much_ simpler to implement. Less complexity, less
> corner-cases, less bugs.
> 

I think I see what you mean. E.g. in cmd_clone we have:

  write_config(&option_config);
  git_config(git_default_config, NULL);
  ...
  git_config_set("remote.Foo.url", repo);
  ...
  remote = remote_get(option_origin); /* which does 'git_config(handle_config)' */

So if we load the config cache at 'git_config(git_default_config)', then
'remote_get' won't see "remote.Foo.url" unless we invalidate the cache first.


I still don't like that the invalidation is done in git_config_set, though, as
this is also used to write completely unrelated files. Wouldn't it be better to
have a 'git_config_refresh()' that could be used in place of (or before) current
'git_config(callback)' calls? The initial implementation could just invalidate
the config cache. If there's time and energy to spare, a more advanced version
could first check if any of the involved config files has changed.


The xstrdup() problem could be solved by interning strings (see the
attached patch for a trivial implementation). I.e. allocate each distinct
string only once (and keep it allocated).

So if there are 100 instances of "true" in the config file, the interned string
pool would contain only one instance (i.e. 5 bytes + hashmap_entry + malloc
overhead, vs. 100 * (5 bytes + malloc overhead) in case of xstrdup()). If the
config cache is cleared, the interned string in the pool would still remain
valid. If the config cache is reloaded, unmodified values would reuse the
existing strings from the pool.

Side note: interning getenv() results would fix the non-POSIX-compliant
getenv()-usage in git [1].

[1] https://groups.google.com/d/msg/msysgit/4cVWWJkN4to/DR8FGTQ09Q0J

---- 8< ----
Subject: [PATCH] hashmap: add string interning API

Interning short strings that are likely to have multiple copies may reduce
memory footprint and speed up string comparisons.

Add a strintern() API that uses a hashmap to manage the pool of interned
strings.

Signed-off-by: Karsten Blees <blees@dcon.de>
---
 Documentation/technical/api-hashmap.txt | 11 +++++++++++
 hashmap.c                               | 34 +++++++++++++++++++++++++++++++++
 hashmap.h                               |  4 ++++
 t/t0011-hashmap.sh                      | 13 +++++++++++++
 test-hashmap.c                          | 14 ++++++++++++++
 5 files changed, 76 insertions(+)

diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt
index b977ae8..db7c955 100644
--- a/Documentation/technical/api-hashmap.txt
+++ b/Documentation/technical/api-hashmap.txt
@@ -162,6 +162,17 @@ more entries.
 `hashmap_iter_first` is a combination of both (i.e. initializes the iterator
 and returns the first entry, if any).
 
+`const char *strintern(const char *string)`::
+
+	Returns the unique, interned version of the specified string, similar
+	to the `String.intern` API in Java and .NET. Interned strings remain
+	valid for the entire lifetime of the process.
++
+Can be used as `[x]strdup()` replacement, except that the interned string must
+not be modified or freed.
++
+Uses a hashmap to store the pool of interned strings.
+
 Usage example
 -------------
 
diff --git a/hashmap.c b/hashmap.c
index d1b8056..03cd8f3 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -226,3 +226,37 @@ void *hashmap_iter_next(struct hashmap_iter *iter)
 		current = iter->map->table[iter->tablepos++];
 	}
 }
+
+struct string_pool_entry {
+	struct hashmap_entry ent;
+	char key[FLEX_ARRAY];
+};
+
+static int string_pool_cmp(const struct string_pool_entry *e1,
+			   const struct string_pool_entry *e2,
+			   const char *key)
+{
+	return strcmp(e1->key, key ? key : e2->key);
+}
+
+const char *strintern(const char *string)
+{
+	static struct hashmap map;
+	struct string_pool_entry key, *e;
+	/* initialize string pool hashmap */
+	if (!map.tablesize)
+		hashmap_init(&map, (hashmap_cmp_fn) string_pool_cmp, 0);
+
+	/* lookup interned string in pool */
+	hashmap_entry_init(&key, strhash(string));
+	e = hashmap_get(&map, &key, string);
+	if (!e) {
+		/* not found: create it */
+		int len = strlen(string);
+		e = xmalloc(sizeof(struct string_pool_entry) + len + 1);
+		hashmap_entry_init(e, key.ent.hash);
+		memcpy(e->key, string, len + 1);
+		hashmap_add(&map, e);
+	}
+	return e->key;
+}
diff --git a/hashmap.h b/hashmap.h
index a816ad4..b428677 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -68,4 +68,8 @@ static inline void *hashmap_iter_first(struct hashmap *map,
 	return hashmap_iter_next(iter);
 }
 
+/* string interning */
+
+extern const char *strintern(const char *string);
+
 #endif
diff --git a/t/t0011-hashmap.sh b/t/t0011-hashmap.sh
index 391e2b6..f97c805 100755
--- a/t/t0011-hashmap.sh
+++ b/t/t0011-hashmap.sh
@@ -237,4 +237,17 @@ test_expect_success 'grow / shrink' '
 
 '
 
+test_expect_success 'string interning' '
+
+test_hashmap "intern value1
+intern Value1
+intern value2
+intern value2
+" "value1
+Value1
+value2
+value2"
+
+'
+
 test_done
diff --git a/test-hashmap.c b/test-hashmap.c
index f5183fb..116cbb5 100644
--- a/test-hashmap.c
+++ b/test-hashmap.c
@@ -239,6 +239,20 @@ int main(int argc, char *argv[])
 			/* print table sizes */
 			printf("%u %u\n", map.tablesize, map.size);
 
+		} else if (!strcmp("intern", cmd) && l1) {
+
+			/* test that strintern works */
+			const char *i1 = strintern(p1);
+			const char *i2 = strintern(p1);
+			if (strcmp(i1, p1))
+				printf("strintern(%s) returns %s\n", p1, i1);
+			else if (i1 == p1)
+				printf("strintern(%s) returns input pointer\n", p1);
+			else if (i1 != i2)
+				printf("strintern(%s) != strintern(%s)", i1, i2);
+			else
+				printf("%s\n", i1);
+
 		} else if (!strcmp("perfhashmap", cmd) && l1 && l2) {
 
 			perf_hashmap(atoi(p1), atoi(p2));
-- 
2.0.0.9649.g1eafa1f.dirty

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Matthieu Moy]

Karsten Blees <karsten.blees@gmail.com> writes:

> I still don't like that the invalidation is done in git_config_set, though, as
> this is also used to write completely unrelated files.

I don't get it. It is used to write the config files. Yes, we trigger a
complete reload instead of just changing this particular value in the
hashmap, but I do not see "unrelated files" in the picture.

> Wouldn't it be better to have a 'git_config_refresh()' that could be
> used in place of (or before) current 'git_config(callback)' calls? The
> initial implementation could just invalidate the config cache. If
> there's time and energy to spare, a more advanced version could first
> check if any of the involved config files has changed.

That would not change the "xstrdup" vs "no xstrdup" issue, right?

> The xstrdup() problem could be solved by interning strings (see the
> attached patch for a trivial implementation). I.e. allocate each distinct
> string only once (and keep it allocated).

That's an option. We need to be carefull not to modify any string
in-place, but I guess that would considerably simplify memory management
for strings.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Karsten Blees]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

Am 28.06.2014 08:01, schrieb Matthieu Moy:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> I still don't like that the invalidation is done in git_config_set, though, as
>> this is also used to write completely unrelated files.
> 
> I don't get it. It is used to write the config files. Yes, we trigger a
> complete reload instead of just changing this particular value in the
> hashmap, but I do not see "unrelated files" in the picture.
> 

git-cherry-pick, revert: writes '.git/sequencer/opts' (sequencer.c:save_opts)
git-mv <submodule>: writes '.gitmodules' (submodule.c:update_path_in_gitmodules)
  ...and the submodule's '.git/config' (submodule.c:connect_work_tree_and_git_dir)


Note that the typical configuration commands git-config/remote/branch seem
to call git_config_set* immediately before exit, so no need to invalidate
the config cache here either.

Which leaves clone, init and push (transport.c:set_upstreams, seems to be
just before exit as well).

I didn't look further after finding the example in cmd_clone, so with the
statistics above, it may well be the only instance of {git_config; 
git_config_set; git_config}, I don't know.

I've attached the reverse call hierarchy as generated by Eclipse - quite
useful for things like this.

>> Wouldn't it be better to have a 'git_config_refresh()' that could be
>> used in place of (or before) current 'git_config(callback)' calls? The
>> initial implementation could just invalidate the config cache. If
>> there's time and energy to spare, a more advanced version could first
>> check if any of the involved config files has changed.
> 
> That would not change the "xstrdup" vs "no xstrdup" issue, right?
> 

Right. (Unless it turns out invalidation isn't needed after all. But even
then using interned strings for the config would be a Good Thing IMO.)

>> The xstrdup() problem could be solved by interning strings (see the
>> attached patch for a trivial implementation). I.e. allocate each distinct
>> string only once (and keep it allocated).
> 
> That's an option. We need to be carefull not to modify any string
> in-place,

Hopefully an illegal non-const cast will be caught in the review process.



[-- Attachment #2: git-config-set-callers.txt --]
[-- Type: text/plain, Size: 6597 bytes --]

git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int) : int - /git/config.c
	cmd_config(int, const char * *, const char *) : int - /git/builtin/config.c (5 matches)
	git_config_set_in_file(const char *, const char *, const char *) : int - /git/config.c
		cmd_config(int, const char * *, const char *) : int - /git/builtin/config.c (2 matches)
		connect_work_tree_and_git_dir(const char *, const char *) : void - /git/submodule.c
			cmd_mv(int, const char * *, const char *) : int - /git/builtin/mv.c
		save_opts(replay_opts *) : void - /git/sequencer.c (8 matches)
			sequencer_pick_revisions(replay_opts *) : int - /git/sequencer.c
				cmd_cherry_pick(int, const char * *, const char *) : int - /git/builtin/revert.c
				cmd_revert(int, const char * *, const char *) : int - /git/builtin/revert.c
		update_path_in_gitmodules(const char *, const char *) : int - /git/submodule.c
			cmd_mv(int, const char * *, const char *) : int - /git/builtin/mv.c
	git_config_set_multivar(const char *, const char *, const char *, int) : int - /git/config.c
		add_branch(const char *, const char *, const char *, int, strbuf *) : int - /git/builtin/remote.c
			add_branches(remote *, const char * *, const char *) : int - /git/builtin/remote.c
				set_remote_branches(const char *, const char * *, int) : int - /git/builtin/remote.c
					set_branches(int, const char * *) : int - /git/builtin/remote.c
						cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			add(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		cmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c (2 matches)
		git_config_set(const char *, const char *) : int - /git/config.c
			add(int, const char * *) : int - /git/builtin/remote.c (3 matches)
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c (2 matches)
			create_default_files(const char *) : int - /git/builtin/init-db.c (8 matches)
				init_db(const char *, unsigned int) : int - /git/builtin/init-db.c
					cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
					cmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c
			edit_branch_description(const char *) : int - /git/builtin/branch.c
				cmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c
			init_db(const char *, unsigned int) : int - /git/builtin/init-db.c (2 matches)
				cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
				cmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c
			install_branch_config(int, const char *, const char *, const char *) : void - /git/branch.c (3 matches)
				cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
				set_upstreams(transport *, ref *, int) : void - /git/transport.c
					transport_push(transport *, int, const char * *, int, unsigned int *) : int - /git/transport.c
						push_with_options(transport *, int) : int - /git/builtin/push.c
							do_push(const char *, int) : int - /git/builtin/push.c (2 matches)
								cmd_push(int, const char * *, const char *) : int - /git/builtin/push.c
				setup_tracking(const char *, const char *, enum branch_track, int) : int - /git/branch.c
					create_branch(const char *, const char *, const char *, int, int, int, int, enum branch_track) : void - /git/branch.c
						cmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c (2 matches)
						update_refs_for_switch(const checkout_opts *, branch_info *, branch_info *) : void - /git/builtin/checkout.c
							switch_branches(const checkout_opts *, branch_info *) : int - /git/builtin/checkout.c
								checkout_branch(checkout_opts *, branch_info *) : int - /git/builtin/checkout.c
									cmd_checkout(int, const char * *, const char *) : int - /git/builtin/checkout.c
				update_head(const ref *, const ref *, const char *) : void - /git/builtin/clone.c
					cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
			mingw_mark_as_git_dir(const char *) : void - /git/compat/mingw.c
				init_db(const char *, unsigned int) : int - /git/builtin/init-db.c
					cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
					cmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c
			mv(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			rm(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			set_url(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			write_refspec_config(const char *, const ref *, const ref *, strbuf *) : void - /git/builtin/clone.c
				cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
		migrate_file(remote *) : int - /git/builtin/remote.c (3 matches)
			mv(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		mv(int, const char * *) : int - /git/builtin/remote.c (2 matches)
			cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		remove_all_fetch_refspecs(const char *, const char *) : int - /git/builtin/remote.c
			set_remote_branches(const char *, const char * *, int) : int - /git/builtin/remote.c
				set_branches(int, const char * *) : int - /git/builtin/remote.c
					cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		set_url(int, const char * *) : int - /git/builtin/remote.c (3 matches)
			cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		write_one_config(const char *, const char *, void *) : int - /git/builtin/clone.c
			write_config(string_list *) : void - /git/builtin/clone.c
				cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
		write_refspec_config(const char *, const ref *, const ref *, strbuf *) : void - /git/builtin/clone.c
			cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
	save_opts(replay_opts *) : void - /git/sequencer.c
		sequencer_pick_revisions(replay_opts *) : int - /git/sequencer.c
			cmd_cherry_pick(int, const char * *, const char *) : int - /git/builtin/revert.c
			cmd_revert(int, const char * *, const char *) : int - /git/builtin/revert.c

Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Matthieu Moy]

Karsten Blees <karsten.blees@gmail.com> writes:

> Am 28.06.2014 08:01, schrieb Matthieu Moy:
>> Karsten Blees <karsten.blees@gmail.com> writes:
>> 
>>> I still don't like that the invalidation is done in git_config_set, though, as
>>> this is also used to write completely unrelated files.
>> 
>> I don't get it. It is used to write the config files. Yes, we trigger a
>> complete reload instead of just changing this particular value in the
>> hashmap, but I do not see "unrelated files" in the picture.
>> 
>
> git-cherry-pick, revert: writes '.git/sequencer/opts' (sequencer.c:save_opts)
> git-mv <submodule>: writes '.gitmodules' (submodule.c:update_path_in_gitmodules)
>   ...and the submodule's '.git/config' (submodule.c:connect_work_tree_and_git_dir)

In the previous patch series, the cache was only about the system, user
and repo config files. So, .git/sequencer/opts, .gitmodules are totally
independant. In the latest series, there's a new notion of config_set.
.gitmodules, .git/sequencer/opts and the usual config files are in
different config sets, and their cache is invalidated independently.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string

[Jonathan Nieder]

Tanay Abhra wrote:

>  alias.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)

What commit are these patches against?  Are they a continuation
of the "git config cache & special querying api" series?

Thanks,
Jonathan

Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/24/2014 4:08 AM, Jonathan Nieder wrote:
> Tanay Abhra wrote:
>
>>   alias.c | 28 ++++++++++------------------
>>   1 file changed, 10 insertions(+), 18 deletions(-)
>
> What commit are these patches against?  Are they a continuation
> of the "git config cache & special querying api" series?
>

My fault, I should have marked them. You have inferred correctly,
they have been built on top of "git config cache & special" querying
api" series[1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/252329

Thanks,
Tanay.

Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Use git_config_get_string instead of git_config to take advantage of
> the config hash-table api which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  alias.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/alias.c b/alias.c
> index 5efc3d6..0fe32bc 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -1,25 +1,17 @@
>  #include "cache.h"
>
> -static const char *alias_key;
> -static char *alias_val;
> -
> -static int alias_lookup_cb(const char *k, const char *v, void *cb)
> -{
> -       if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
> -               if (!v)
> -                       return config_error_nonbool(k);
> -               alias_val = xstrdup(v);
> -               return 0;
> -       }
> -       return 0;
> -}
> -
>  char *alias_lookup(const char *alias)
>  {
> -       alias_key = alias;
> -       alias_val = NULL;
> -       git_config(alias_lookup_cb, NULL);
> -       return alias_val;
> +       const char *v;
> +       char *value;
> +       struct strbuf key = STRBUF_INIT;
> +       strbuf_addf(&key, "alias.%s", alias);
> +       git_config_get_string(key.buf, &v);
> +       if (!v)
> +               config_error_nonbool(key.buf);

If 'v' is NULL, you correctly report an error, but then fall through
and invoke xstrdup() with NULL, which invites undefined behavior [1].

[1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html

> +       value = xstrdup(v);
> +       strbuf_release(&key);
> +       return value;

You could release the strbuf earlier, which would allow you to 'return
xstrdup(v)' and drop the 'value' variable. Perhaps you want something
like this:

    const char *v;
    struct strbuf key = STRBUF_INIT;
    strbuf_addf(&key, "alias.%s", alias);
    git_config_get_string(key.buf, &v);
    if (v)
        config_error_nonbool(key.buf);
    strbuf_release(&key);
    return v ? xstrdup(v) : NULL;

>  }
>
>  #define SPLIT_CMDLINE_BAD_ENDING 1
> --
> 1.9.0.GIT

Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/25/2014 7:42 AM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  alias.c | 28 ++++++++++------------------
>>  1 file changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/alias.c b/alias.c
>> index 5efc3d6..0fe32bc 100644
>> --- a/alias.c
>> +++ b/alias.c
>> @@ -1,25 +1,17 @@
>>  #include "cache.h"
>>
>> -static const char *alias_key;
>> -static char *alias_val;
>> -
>> -static int alias_lookup_cb(const char *k, const char *v, void *cb)
>> -{
>> -       if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
>> -               if (!v)
>> -                       return config_error_nonbool(k);
>> -               alias_val = xstrdup(v);
>> -               return 0;
>> -       }
>> -       return 0;
>> -}
>> -
>>  char *alias_lookup(const char *alias)
>>  {
>> -       alias_key = alias;
>> -       alias_val = NULL;
>> -       git_config(alias_lookup_cb, NULL);
>> -       return alias_val;
>> +       const char *v;
>> +       char *value;
>> +       struct strbuf key = STRBUF_INIT;
>> +       strbuf_addf(&key, "alias.%s", alias);
>> +       git_config_get_string(key.buf, &v);
>> +       if (!v)
>> +               config_error_nonbool(key.buf);
> 
> If 'v' is NULL, you correctly report an error, but then fall through
> and invoke xstrdup() with NULL, which invites undefined behavior [1].
> 
> [1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html
> 
>> +       value = xstrdup(v);
>> +       strbuf_release(&key);
>> +       return value;
> 
> You could release the strbuf earlier, which would allow you to 'return
> xstrdup(v)' and drop the 'value' variable. Perhaps you want something
> like this:
> 
>     const char *v;
>     struct strbuf key = STRBUF_INIT;
>     strbuf_addf(&key, "alias.%s", alias);
>     git_config_get_string(key.buf, &v);
>     if (v)
>         config_error_nonbool(key.buf);
>     strbuf_release(&key);
>     return v ? xstrdup(v) : NULL;
> 

Done. Thanks.

>>  }
>>
>>  #define SPLIT_CMDLINE_BAD_ENDING 1
>> --
>> 1.9.0.GIT
> 

Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> the config hash-table api which provides a cleaner control flow.

api -> API

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/