RFC v2alias.c: replace git_config with git_config_get_string

> 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

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

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

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

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

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

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

> +	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);

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

> 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");

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

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

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

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

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

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

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

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

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

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

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

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

> 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

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

> 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

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

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

>>  /* 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.
> 

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

> 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).

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

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

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

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

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

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

> 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);

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

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