Re: [PATCH 08/21] remote: let guess_remote_head() optionally return all matches
From: Jay Soffian <hidden>
Date: 2016-06-15 22:46:17
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Thu, Feb 26, 2009 at 9:40 AM, Jeff King [off-list ref] wrote:
On Thu, Feb 26, 2009 at 09:37:29AM -0500, Jeff King wrote:quoted
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.