From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Fri, 18 Nov 2011 00:57:29 GMT
Subject: Re: [PATCH 6/8] Convert resolve_ref_unsafe+xstrdup to resolve_ref
Message-ID: <CACsJy8ATKa+=8HT8qrxgvs1gwFvT=HNxbxwSWf26N3oss6Bhfw@mail.gmail.com>
In-Reply-To: <CALkWK0=h2Q1VTt5AwbBMaZgCYNEkZG5vQGoG=SDBMqYexhJjGA@mail.gmail.com>

2011/11/17 Ramkumar Ramachandra <artagnon@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.

free(NULL) is OK if I remember correctly, so it's not really a bug.
Although I do plug a memory leak when merge_working_tree() returns
non-zero.
-- 
Duy