On Thu, Feb 26, 2009 at 9:40 AM, Jeff King <peff@xxxxxxxx> 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.
--
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
