# [PATCH 07a/21] remote: make copy_ref() perform a deep copy 5 messages from 2009-02-27 to 2009-02-28. Participants: Jay Soffian, Jeff King. Thread: https://gitlist.dev/t/18048 ## Jay Soffian, 2009-02-27 19:10 Subject: [PATCH 0/3] git remote: set-head and new show output (UPDATED) Message-ID: URL: https://gitlist.dev/e/cover.1235759631.git.jaysoffian%40gmail.com In-Reply-To: <20090227114308.GA20776@coredump.intra.peff.net> ``` Jeff, Junio, I didn't want to resend the whole series (gmane 111394), so what I did was create a new patch which comes after patch 7 in that series. Call it patch 7a. The rest of that series is then identical (modulo line number offsets in patches touching remote.c) except for patches 8 and 19. Patches 7a, and the new 8 and 19 follow this email. You should be able to do this: $ git checkout js/remote-improvements $ git format-patch master $ git reset --hard $(git merge-base master HEAD) Copy new "[PATCH 07a/21] remote: make copy_ref() perform a deep copy" to 0007a-remote-make-copy_ref-perform-a-deep-copy.patch Copy new "[PATCH 08/21] remote: let guess_remote_head() optionally return all matches" to 0008-remote-let-guess_remote_head-optionally-return-al.patch Copy new "[PATCH 19/21] remote: make guess_remote_head() use exact HEAD lookup if it is available" to 0019-remote-make-guess_remote_head-use-exact-HEAD-look.patch $ git am -3 *.patch For those playing along at home, js/remote-improvements is currently commits 661763a..5e4c90a. Which should give you a new js/remote-improvements that looks like this: 1 test scripts: refactor start_httpd helper 2 add basic http clone/fetch tests 3 refactor find_ref_by_name() to accept const list 4 move duplicated get_local_heads() to remote.c 5 move duplicated ref_newer() to remote.c 6 move locate_head() to remote.c 7 remote: simplify guess_remote_head() 8 remote: make copy_ref() perform a deep copy 9 remote: let guess_remote_head() optionally return all matches 10 remote: make match_refs() copy src ref before assigning to peer_ref 11 remote: make match_refs() not short-circuit 12 string-list: new for_each_string_list() function 13 builtin-remote: refactor duplicated cleanup code 14 builtin-remote: remove unused code in get_ref_states 15 builtin-remote: rename variables and eliminate redundant function call 16 builtin-remote: make get_remote_ref_states() always populate states.tracked 17 builtin-remote: fix two inconsistencies in the output of "show " 18 builtin-remote: teach show to display remote HEAD 19 builtin-remote: add set-head subcommand 20 remote: make guess_remote_head() use exact HEAD lookup if it is available 21 builtin-remote: new show output style 22 builtin-remote: new show output style for push refspecs Here's the new diff stat: Documentation/git-remote.txt | 28 ++- Makefile | 1 + builtin-clone.c | 41 +--- builtin-remote.c | 561 ++++++++++++++++++++++++++------ builtin-send-pack.c | 79 +----- cache.h | 2 +- contrib/completion/git-completion.bash | 2 +- http-push.c | 72 +---- refs.c | 4 +- remote.c | 145 ++++++++- remote.h | 12 + string-list.c | 10 + string-list.h | 5 + t/lib-httpd.sh | 9 +- t/t5505-remote.sh | 114 +++++-- t/t5540-http-push.sh | 9 +- t/t5550-http-fetch.sh | 57 ++++ 17 files changed, 821 insertions(+), 330 deletions(-) create mode 100755 t/t5550-http-fetch.sh And here's the inter-diff: diff --git a/remote.c b/remote.c index 1c09cf0..9b8522d 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; + size_t len; + if (!ref) + return NULL; + len = strlen(ref->name); + cpy = xmalloc(sizeof(struct ref) + len + 1); + memcpy(cpy, ref, sizeof(struct ref) + len + 1); + cpy->next = NULL; + cpy->symref = ref->symref ? xstrdup(ref->symref) : NULL; + cpy->remote_status = ref->remote_status ? xstrdup(ref->remote_status) : NULL; + cpy->peer_ref = copy_ref(ref->peer_ref); + return cpy; } struct ref *copy_ref_list(const struct ref *ref) @@ -801,6 +809,7 @@ static void free_ref(struct ref *ref) { if (!ref) return; + free_ref(ref->peer_ref); free(ref->remote_status); free(ref->symref); 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; ``` ## Jay Soffian, 2009-02-27 19:10 Subject: [PATCH 07a/21] remote: make copy_ref() perform a deep copy Message-ID: <45e13cde37f6da8e6c379b5deea81b00148b4355.1235759631.git.jaysoffian@gmail.com> URL: https://gitlist.dev/e/45e13cde37f6da8e6c379b5deea81b00148b4355.1235759631.git.jaysoffian%40gmail.com In-Reply-To: ``` To ensure that copied refs can always be freed w/o causing a double-free, make copy_ref() perform a deep copy. Also have copy_ref() return NULL if asked to copy NULL to simplify things for the caller. Background: currently copy_ref() performs a shallow copy. This is fine for current callers who never free the result and/or only copy refs which contain NULL pointers. But copy_ref() is about to gain a new caller (guess_remote_head()) which copies refs where peer_ref is not NULL and the caller of guess_remote_head() will want to free the result. Signed-off-by: Jay Soffian --- This is a new patch, which should come after 07/21 in the original series - http://article.gmane.org/gmane.comp.version-control.git/111394 remote.c | 18 +++++++++++++----- 1 files changed, 13 insertions(+), 5 deletions(-) diff --git a/remote.c b/remote.c index aed760e..22203ea 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; + size_t len; + if (!ref) + return NULL; + len = strlen(ref->name); + cpy = xmalloc(sizeof(struct ref) + len + 1); + memcpy(cpy, ref, sizeof(struct ref) + len + 1); + cpy->next = NULL; + cpy->symref = ref->symref ? xstrdup(ref->symref) : NULL; + cpy->remote_status = ref->remote_status ? xstrdup(ref->remote_status) : NULL; + cpy->peer_ref = copy_ref(ref->peer_ref); + return cpy; } struct ref *copy_ref_list(const struct ref *ref) @@ -801,6 +809,7 @@ static void free_ref(struct ref *ref) { if (!ref) return; + free_ref(ref->peer_ref); free(ref->remote_status); free(ref->symref); 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; } -- 1.6.2.rc1.309.g5f417 ``` ## Jay Soffian, 2009-02-27 19:10 Subject: [PATCH 08/21] remote: let guess_remote_head() optionally return all matches Message-ID: <9d146489b82f3f8108903dcb9acbe569a4b690a1.1235759631.git.jaysoffian@gmail.com> URL: https://gitlist.dev/e/9d146489b82f3f8108903dcb9acbe569a4b690a1.1235759631.git.jaysoffian%40gmail.com In-Reply-To: <45e13cde37f6da8e6c379b5deea81b00148b4355.1235759631.git.jaysoffian@gmail.com> ``` Determining HEAD is ambiguous since it is done by comparing SHA1s. In the case of multiple matches we return refs/heads/master if it matches, else we return the first match we encounter. builtin-remote needs all matches returned to it, so add a flag for it to request such. To be simple and consistent, the return value is now a copy (including peer_ref) of the matching refs. Originally contributed by Jeff King along with the prior commit as a single patch. Signed-off-by: Jay Soffian --- This patch replaces 08/21 in the original series series - http://article.gmane.org/gmane.comp.version-control.git/111394 builtin-clone.c | 2 +- remote.c | 29 +++++++++++++++++++---------- remote.h | 14 ++++++++------ 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/builtin-clone.c b/builtin-clone.c index f9ce4fb..3146ca8 100644 --- a/builtin-clone.c +++ b/builtin-clone.c @@ -510,7 +510,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) mapped_refs = write_remote_refs(refs, &refspec, reflog_msg.buf); remote_head = find_ref_by_name(refs, "HEAD"); - head_points_at = guess_remote_head(remote_head, mapped_refs); + head_points_at = guess_remote_head(remote_head, mapped_refs, 0); } else { warning("You appear to have cloned an empty repository."); diff --git a/remote.c b/remote.c index 22203ea..304e967 100644 --- a/remote.c +++ b/remote.c @@ -1460,24 +1460,33 @@ struct ref *get_local_heads(void) return local_refs; } -const struct ref *guess_remote_head(const struct ref *head, - const struct ref *refs) +struct ref *guess_remote_head(const struct ref *head, + const struct ref *refs, + int all) { const struct ref *r; + struct ref *list = NULL; + struct ref **tail = &list; if (!head) return NULL; /* If refs/heads/master could be right, it is. */ - r = find_ref_by_name(refs, "refs/heads/master"); - if (r && !hashcmp(r->old_sha1, head->old_sha1)) - return r; + if (!all) { + r = find_ref_by_name(refs, "refs/heads/master"); + if (r && !hashcmp(r->old_sha1, head->old_sha1)) + 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)) - return r; + for (r = refs; r; r = r->next) { + if (r != head && !hashcmp(r->old_sha1, head->old_sha1)) { + *tail = copy_ref(r); + tail = &((*tail)->next); + if (!all) + break; + } + } - /* Nothing is the same */ - return NULL; + return list; } diff --git a/remote.h b/remote.h index db49ce0..de3d21b 100644 --- a/remote.h +++ b/remote.h @@ -139,12 +139,14 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs); int format_tracking_info(struct branch *branch, struct strbuf *sb); struct ref *get_local_heads(void); - /* - * Look for a ref in refs whose SHA1 matches head, first checking if - * refs/heads/master matches. Return NULL if nothing matches or if head - * is NULL. + * Find refs from a list which are likely to be pointed to by the given HEAD + * ref. If 'all' is false, returns the most likely ref; otherwise, returns a + * list of all candidate refs. If no match is found (or 'head' is NULL), + * returns NULL. All returns are newly allocated and should be freed. */ -const struct ref *guess_remote_head(const struct ref *head, - const struct ref *refs); +struct ref *guess_remote_head(const struct ref *head, + const struct ref *refs, + int all); + #endif -- 1.6.2.rc1.309.g5f417 ``` ## Jay Soffian, 2009-02-27 19:10 Subject: [PATCH 19/21] remote: make guess_remote_head() use exact HEAD lookup if it is available Message-ID: <710d1a0d6059e6dee31732c2762aed7e909daad7.1235759631.git.jaysoffian@gmail.com> URL: https://gitlist.dev/e/710d1a0d6059e6dee31732c2762aed7e909daad7.1235759631.git.jaysoffian%40gmail.com In-Reply-To: <9d146489b82f3f8108903dcb9acbe569a4b690a1.1235759631.git.jaysoffian@gmail.com> ``` From: Jeff King Our usual method for determining the ref pointed to by HEAD is to compare HEAD's sha1 to the sha1 of all refs, trying to find a unique match. However, some transports actually get to look at HEAD directly; we should make use of that information when it is available. Currently, only http remotes support this feature. Signed-off-by: Jeff King Signed-off-by: Jay Soffian --- This patch replaces 19/21 in the original series series - http://article.gmane.org/gmane.comp.version-control.git/111394 remote.c | 8 ++++++++ t/t5550-http-fetch.sh | 11 +++++++++++ 2 files changed, 19 insertions(+), 0 deletions(-) diff --git a/remote.c b/remote.c index 2123005..9b8522d 100644 --- a/remote.c +++ b/remote.c @@ -1476,6 +1476,14 @@ struct ref *guess_remote_head(const struct ref *head, if (!head) return NULL; + /* + * Some transports support directly peeking at + * where HEAD points; if that is the case, then + * we don't have to guess. + */ + 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"); diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index b6e6ec9..05b1b62 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -42,5 +42,16 @@ test_expect_success 'fetch changes via http' ' test_cmp file clone/file ' +test_expect_success 'http remote detects correct HEAD' ' + git push public master:other && + (cd clone && + git remote set-head origin -d && + git remote set-head origin -a && + git symbolic-ref refs/remotes/origin/HEAD > output && + echo refs/remotes/origin/master > expect && + test_cmp expect output + ) +' + stop_httpd test_done -- 1.6.2.rc1.309.g5f417 ``` ## Jeff King, 2009-02-28 06:33 Subject: Re: [PATCH 0/3] git remote: set-head and new show output (UPDATED) Message-ID: <20090228063322.GA6775@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090228063322.GA6775%40coredump.intra.peff.net In-Reply-To: ``` On Fri, Feb 27, 2009 at 02:10:03PM -0500, Jay Soffian wrote: > I didn't want to resend the whole series (gmane 111394), so what I did was > create a new patch which comes after patch 7 in that series. Call it patch 7a. > The rest of that series is then identical (modulo line number offsets in > patches touching remote.c) except for patches 8 and 19. Patches 7a, and the > new 8 and 19 follow this email. These look good to me. -Peff ```