Re: [PATCH 08/21] remote: let guess_remote_head() optionally return all matches

[Jay Soffian <jaysoffian@gmail.com>]

On Thu, Feb 26, 2009 at 9:40 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 26, 2009 at 09:37:29AM -0500, Jeff King wrote:
>
>> Hmm. This should probably be:
>>
>>   dst->peer_ref = src->peer_ref ? copy_ref(src->peer_ref) : NULL;
>>
>> (or copy_ref should return NULL when given NULL). I also wonder if the
>> copied ref's peer_ref should be explicitly NULL'd.

Well, if you wanted to be consistent about things (and I apologize if gmail
mangles the lines), I'd probably do something like:

diff --git a/remote.c b/remote.c
index 1c09cf0..9f1bf5e 100644
--- a/remote.c
+++ b/remote.c
@@ -779,10 +779,18 @@ struct ref *alloc_ref(const char *name)

 static struct ref *copy_ref(const struct ref *ref)
 {
-	struct ref *ret = xmalloc(sizeof(struct ref) + strlen(ref->name) + 1);
-	memcpy(ret, ref, sizeof(struct ref) + strlen(ref->name) + 1);
-	ret->next = NULL;
-	return ret;
+	struct ref *cpy;
+	if (!ref)
+		return NULL;
+	cpy = xmalloc(sizeof(struct ref) + strlen(ref->name) + 1);
+	memcpy(cpy, ref, sizeof(struct ref) + strlen(ref->name) + 1);
+	cpy->next = cpy->peer_ref = NULL;
+	if (ref->peer_ref) {
+		ref = ref->peer_ref;
+		cpy->peer_ref = xmalloc(sizeof(struct ref) + strlen(ref->name) + 1);
+		memcpy(cpy->peer_ref, ref, sizeof(struct ref) + strlen(ref->name) + 1);
+	}
+	return cpy;
 }

 struct ref *copy_ref_list(const struct ref *ref)
@@ -803,6 +811,7 @@ static void free_ref(struct ref *ref)
 		return;
 	free(ref->remote_status);
 	free(ref->symref);
+	free(ref->peer_ref);
 	free(ref);
 }

@@ -811,7 +820,6 @@ void free_refs(struct ref *ref)
 	struct ref *next;
 	while (ref) {
 		next = ref->next;
-		free(ref->peer_ref);
 		free_ref(ref);
 		ref = next;
 	}
@@ -1457,13 +1465,6 @@ struct ref *get_local_heads(void)
 	return local_refs;
 }

-struct ref *copy_ref_with_peer(const struct ref *src)
-{
-	struct ref *dst = copy_ref(src);
-	dst->peer_ref = copy_ref(src->peer_ref);
-	return dst;
-}
-
 struct ref *guess_remote_head(const struct ref *head,
 			      const struct ref *refs,
 			      int all)
@@ -1480,22 +1481,20 @@ struct ref *guess_remote_head(const struct ref *head,
 	 * where HEAD points; if that is the case, then
 	 * we don't have to guess.
 	 */
-	if (head->symref) {
-		r = find_ref_by_name(refs, head->symref);
-		return r ? copy_ref_with_peer(r) : NULL;
-	}
+	if (head->symref)
+		return copy_ref(find_ref_by_name(refs, head->symref));

 	/* If refs/heads/master could be right, it is. */
 	if (!all) {
 		r = find_ref_by_name(refs, "refs/heads/master");
 		if (r && !hashcmp(r->old_sha1, head->old_sha1))
-			return copy_ref_with_peer(r);
+			return copy_ref(r);
 	}

 	/* Look for another ref that points there */
 	for (r = refs; r; r = r->next) {
 		if (r != head && !hashcmp(r->old_sha1, head->old_sha1)) {
-			*tail = copy_ref_with_peer(r);
+			*tail = copy_ref(r);
 			tail = &((*tail)->next);
 			if (!all)
 				break;


Then peer_ref is consistently a copy, so we can free it consistently, we don't
need two separate copy functions, and copy_ref returns NULL upon receiving
NULL like most of the other foo_ref functions.

> BTW, all of my "probably" and "I wonder" here are because I think the
> "peer ref" pointer is a little vague as a concept. E.g., I think in most
> cases src->peer_ref->peer_ref != src.
>
> Rather than having ref structs with "next" and "peer" pointers, I think
> a more natural data structure would be a list (or array) of "ref pairs".

Actually, we don't need most of the fields in the peer_ref, so we could
probably just embed the extra fields that we need in a peer_struct inside the
ref struct. I can add this to my git todo list.

j.