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

Tanay Abhra [off-list ref] writes:

On 6/30/2014 7:04 PM, Karsten Blees wrote:

quoted

Am 29.06.2014 13:01, schrieb Eric Sunshine:

quoted

On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra [off-list ref] wrote:

quoted

On 6/25/2014 1:24 PM, Eric Sunshine wrote:

quoted

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra [off-list ref] wrote:

quoted

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