Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
Show 42 quoted lines
> 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?
Show 6 quoted lines
>> 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.
>
Show 28 quoted lines
>> - 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.
> Show 15 quoted lines
>> }
>>
>>
>> @@ -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
> Tanay Abhra <tanayabh@gmail.com>