From: Jeff King <peff@peff.net>
Date: Mon, 14 Nov 2011 11:24:40 GMT
Subject: Re: [PATCH 2/2] Copy resolve_ref() return value for longer use
Message-ID: <20111114112440.GD10847@sigill.intra.peff.net>
In-Reply-To: <CACsJy8BnqoPVJiM6mbq7p3gKtLh-KGUuTshcukGokC3istTxMQ@mail.gmail.com>

On Mon, Nov 14, 2011 at 10:32:11AM +0700, Nguyen Thai Ngoc Duy wrote:

> 2011/11/14 Junio C Hamano <gitster@pobox.com>:
> >> diff --git a/builtin/branch.c b/builtin/branch.c
> >> index 0fe9c4d..5b6d839 100644
> >> --- a/builtin/branch.c
> >> +++ b/builtin/branch.c
> >> @@ -115,8 +115,10 @@ static int branch_merged(int kind, const char *name,
> >>                   branch->merge[0] &&
> >>                   branch->merge[0]->dst &&
> >>                   (reference_name =
> >> -                  resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
> >> +                  resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {
> >> +                     reference_name = xstrdup(reference_name);
> >>                       reference_rev = lookup_commit_reference(sha1);
> >> +             }
> >>       }
> >>       if (!reference_rev)
> >>               reference_rev = head_rev;
> >> @@ -141,6 +143,7 @@ static int branch_merged(int kind, const char *name,
> >>                               "         '%s', even though it is merged to HEAD."),
> >>                               name, reference_name);
> >>       }
> >> +     free((char*)reference_name);
> >>       return merged;
> >>  }
> >
> > Now reference_name stores the result of xstrdup(), it does not have reason
> > to be of type "const char *". It is preferable to lose the cast here, I
> > think. The same comment applies to the remainder of the patch.
> 
> But resolve_ref() returns "const char *", we need to type cast at
> least once, either at resolve_ref() assignment or at free(), until we
> change resolve_ref(). Or should we change resolve_ref() to return
> "char *" now?

Your problem is that you are using the same variable for two different
things: storing the pointer to non-owned memory returned from
resolve_ref, and then storing the owned memory that comes from xstrdup.
Those two things have different types, since we use "const" on non-owned
memory. Thus you end up casting.

So your code isn't wrong, but I do think it would be more obviously
correct to a reader if it used two variables and dropped the cast.

-Peff