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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html