From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Thu, 17 Nov 2011 10:22:53 GMT
Subject: Re: [PATCH 6/8] Convert resolve_ref_unsafe+xstrdup to resolve_ref
Message-ID: <CALkWK0=h2Q1VTt5AwbBMaZgCYNEkZG5vQGoG=SDBMqYexhJjGA@mail.gmail.com>
In-Reply-To: <1321522335-24193-7-git-send-email-pclouds@gmail.com>

Hi Nguyễn,

Nguyễn Thái Ngọc Duy wrote:
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2b8e73b..6efb1cf 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -696,15 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>  {
>        int ret = 0;
>        struct branch_info old;
> +       char *path;
>        unsigned char rev[20];
>        int flag;
>        memset(&old, 0, sizeof(old));
> -       old.path = xstrdup(resolve_ref_unsafe("HEAD", rev, 0, &flag));
> +       old.path = path = resolve_ref("HEAD", rev, 0, &flag);
>        old.commit = lookup_commit_reference_gently(rev, 1);
> -       if (!(flag & REF_ISSYMREF)) {
> -               free((char *)old.path);
> +       if (!(flag & REF_ISSYMREF))
>                old.path = NULL;
> -       }
>
>        if (old.path && !prefixcmp(old.path, "refs/heads/"))
>                old.name = old.path + strlen("refs/heads/");

This caught my eye immediately: you introduced a new "path" variable.
Let's scroll ahead and see why.

> @@ -718,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>        }
>
>        ret = merge_working_tree(opts, &old, new);
> -       if (ret)
> +       if (ret) {
> +               free(path);
>                return ret;
> +       }
>
>        if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
>                orphaned_commit_warning(old.commit);
> @@ -727,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>        update_refs_for_switch(opts, &old, new);
>
>        ret = post_checkout_hook(old.commit, new->commit, 1);
> -       free((char *)old.path);
> +       free(path);
>        return ret || opts->writeout_error;
>  }

Before application of your patch, if !(flag & REF_ISSYMREF) then
old.path is set to NULL and this free() would've read free(NULL).  Was
this codepath ever reached, and did you fix a bug by introducing the
new "path" variable, or was it never reached but you introduced the
new variable for clarity anyway?  Either case is worth mentioning in
the commit message, I think.

> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index b954ca8..dc19f3c 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -628,11 +628,8 @@ static void populate_value(struct refinfo *ref)
>
>        if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
>                unsigned char unused1[20];
> -               const char *symref;
> -               symref = resolve_ref_unsafe(ref->refname, unused1, 1, NULL);
> -               if (symref)
> -                       ref->symref = xstrdup(symref);
> -               else
> +               ref->symref = resolve_ref(ref->refname, unused1, 1, NULL);
> +               if (!ref->symref)
>                        ref->symref = "";
>        }
> [...]

This bit along with the others follow a common pattern: one temporary
variable was used to capture the value of resolve_ref_unsafe(), before
using xstrdup() on it to perform the actual assignment.  You changed
the resolve_ref_unsafe() to resolve_ref() and got rid of that extra
variable.

Thanks.

-- Ram