On Mon, Nov 14, 2011 at 10:32:11AM +0700, Nguyen Thai Ngoc Duy wrote:
> 2011/11/14 Junio C Hamano <gitster@xxxxxxxxx>:
> >> 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
--
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