Re: [PATCH 2/2] Copy resolve_ref() return value for longer use

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

2011/11/14 Junio C Hamano [off-list ref]:

quoted

quoted

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