Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:01:44
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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.
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.
quoted hunk ↗ jump to hunk
- 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.
quoted hunk ↗ jump to hunk
}@@ -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