{"thread":{"id":"19500","subject":"Segfault in \"git remote show \"","startedAt":"2009-05-25T16:10:12Z","lastAt":"2009-05-31T14:26:48Z","messageCount":9,"participants":["Erik Faye-Lund","Clemens Buchacher","Jay Soffian","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"114678","messageId":"40aa078e0905250910v268faa66p29a0183db26cb0df@mail.gmail.com","threadId":"19500","inReplyTo":null,"subject":"Segfault in \"git remote show \"","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-05-25T16:10:12Z","receivedAt":"2009-05-25T16:10:12Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"I've been messing around a bit, trying to set up a gitosis repo to\nmirror one of my projects. Now, I added the remote to my project, but\nwhen running \"git remote show \", I'm getting a segfault\nin builtin-remote.c at line 303 (\"while (ref->next)\"), because ref\nitself is NULL. The offending function itself (get_push_ref_states) is\ncalled with NULL as the remote_refs parameters, leading to a\nNULL-pointer dereferencing.\n\nHere's the backtrace from the crash:\n\n(gdb) bt\n#0 get_push_ref_states (remote_refs=0x0, states=0x27fcdc)\n at builtin-remote.c:303\n#1 0x00444301 in get_remote_ref_states (name=0x342bcb \"origin_new\",\n states=0x27fcdc, query=7) at builtin-remote.c:803\n#2 0x004449ae in show (argc=1, argv=0x342d0c) at builtin-remote.c:1004\n#3 0x0044593b in cmd_remote (argc=2, argv=0x342d0c, prefix=0x0)\n at builtin-remote.c:1323\n#4 0x00401bc7 in run_builtin (p=0x4e8338, argc=3, argv=0x342d0c) at git.c:247\n#5 0x00401dab in handle_internal_command (argc=3, argv=0x342d0c) at git.c:392\n#6 0x00401ea7 in run_argv (argcp=0x27ff30, argv=0x27ff34) at git.c:438\n#7 0x00402036 in mingw_main (argc=3, argv=0x342d0c) at git.c:509\n#8 0x00401f12 in main (argc=4, argv=0x342d08) at git.c:456\n\nIt appears that the reason for the crash is that get_remote_heads()\n(called from transport_get_remote_refs() through\nget_refs_via_connect()) returns NULL in list.\nHere's my log from stepping through get_remote_heads:\n\nBreakpoint 3, get_remote_heads (in=7, list=0x27fc28, nr_match=0, match=0x0,\n flags=0, extra_have=0x27a127c) at connect.c:59\n59 *list = NULL;\n(gdb) n\n67 len = packet_read_line(in, buffer, sizeof(buffer));\n(gdb) n\n68 if (!len)\n(gdb) n\n101 return list;\n(gdb) n\n102 }\n(gdb) p list\n$13 = (struct ref **) 0x27fc28\n(gdb) p list\n$14 = (struct ref **) 0x27fc28\n(gdb) n\nget_refs_via_connect (transport=0x27a1218, for_push=0) at transport.c:640\n640 return refs;\n(gdb) p refs\n$15 = (struct ref *) 0x0\n\nIt looks to me like we're failing to read anything from the network\nhere and don't handle the error correctly, but I don't understand the\ncode well enough to tell for sure.\n\nMy best guess would be that a fix could be something like this, but\nI'm not really 100% sure. Doing so makes \"git remote show\nremote->url[0] : NULL);\n remote_refs = transport_get_remote_refs(transport);\n transport_disconnect(transport);\n+ if (!remote_refs) return 0;\n\n states->queried = 1;\n if (query & GET_REF_STATES)\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"114686","messageId":"20090525190121.GA3979@localhost","threadId":"19500","inReplyTo":"40aa078e0905250910v268faa66p29a0183db26cb0df@mail.gmail.com","subject":"Re: Segfault in \"git remote show \"","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-25T19:01:21Z","receivedAt":"2009-05-25T19:01:21Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Subject: [PATCH] fix segfault showing an empty remote\n\nIn case of an empty list, the search for its tail caused a\nNULL-pointer dereference.\n\nReported-by: Erik Faye-Lund \nSigned-off-by: Clemens Buchacher \n---\n\nOn Mon, May 25, 2009 at 06:10:12PM +0200, Erik Faye-Lund wrote:\n> I've been messing around a bit, trying to set up a gitosis repo to\n> mirror one of my projects. Now, I added the remote to my project, but\n> when running \"git remote show \", I'm getting a segfault\n> in builtin-remote.c at line 303 (\"while (ref->next)\"), because ref\n> itself is NULL.\n\nThis should fix it.\n\nI simply copied this from other uses of match_refs. I wonder if this calls\nfor a find_link_ref_tail() function, but I didn't know where to put it.\n\nClemens\n\n builtin-remote.c | 8 ++++----\n t/t5505-remote.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 71abf68..fda9a54 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -299,11 +299,11 @@ static int get_push_ref_states(const struct ref *remote_refs,\n \t\treturn 0;\n \n \tlocal_refs = get_local_heads();\n-\tref = push_map = copy_ref_list(remote_refs);\n-\twhile (ref->next)\n-\t\tref = ref->next;\n-\tpush_tail = &ref->next;\n+\tpush_map = copy_ref_list(remote_refs);\n \n+\tpush_tail = &push_map;\n+\twhile (*push_tail)\n+\t\tpush_tail = &((*push_tail)->next);\n \tmatch_refs(local_refs, push_map, &push_tail, remote->push_refspec_nr,\n \t\t remote->push_refspec, MATCH_REFS_NONE);\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 5ec668d..e70246b 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -494,5 +494,15 @@ test_expect_success 'remote prune to cause a dangling symref' '\n \tgrep \"dangling symref\" err\n '\n \n+test_expect_success 'show empty remote' '\n+\n+\ttest_create_repo empty &&\n+\tgit clone empty empty-clone &&\n+\t(\n+\t\tcd empty-clone &&\n+\t\tgit remote show origin\n+\t)\n+'\n+\n test_done\n \n-- \n1.6.3.1.147.g637c3\n"},{"id":"114738","messageId":"76718490905260727v7261391uab169167978e4522@mail.gmail.com","threadId":"19500","inReplyTo":"20090525190121.GA3979@localhost","subject":"Re: Segfault in \"git remote show \"","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-05-26T14:27:15Z","receivedAt":"2009-05-26T14:27:15Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, May 25, 2009 at 3:01 PM, Clemens Buchacher wrote:\n> Subject: [PATCH] fix segfault showing an empty remote\n>\n> In case of an empty list, the search for its tail caused a\n> NULL-pointer dereference.\n>\n> Reported-by: Erik Faye-Lund \n> Signed-off-by: Clemens Buchacher \n\nAcked-by: Jay Soffian\n\n> ---\n>\n> On Mon, May 25, 2009 at 06:10:12PM +0200, Erik Faye-Lund wrote:\n>> I've been messing around a bit, trying to set up a gitosis repo to\n>> mirror one of my projects. Now, I added the remote to my project, but\n>> when running \"git remote show \", I'm getting a segfault\n>> in builtin-remote.c at line 303 (\"while (ref->next)\"), because ref\n>> itself is NULL.\n>\n> This should fix it.\n\nThanks.\n\n> I simply copied this from other uses of match_refs. I wonder if this calls\n> for a find_link_ref_tail() function, but I didn't know where to put it.\n\nSince the primary use case is in combination with match_refs(),\nremote.[ch] perhaps.\n\nj.\n"},{"id":"114743","messageId":"40aa078e0905261154o7f8901e4k8526c787e0e9d3ac@mail.gmail.com","threadId":"19500","inReplyTo":"20090525190121.GA3979@localhost","subject":"Re: Segfault in \"git remote show \"","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-05-26T18:54:16Z","receivedAt":"2009-05-26T18:54:16Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, May 25, 2009 at 9:01 PM, Clemens Buchacher wrote:\n> Subject: [PATCH] fix segfault showing an empty remote\n>\n> In case of an empty list, the search for its tail caused a\n> NULL-pointer dereference.\n>\n> Reported-by: Erik Faye-Lund \n> Signed-off-by: Clemens Buchacher \n\nTested-by: Erik Faye-Lund \n\n> ---\n>\n> On Mon, May 25, 2009 at 06:10:12PM +0200, Erik Faye-Lund wrote:\n>> I've been messing around a bit, trying to set up a gitosis repo to\n>> mirror one of my projects. Now, I added the remote to my project, but\n>> when running \"git remote show \", I'm getting a segfault\n>> in builtin-remote.c at line 303 (\"while (ref->next)\"), because ref\n>> itself is NULL.\n>\n> This should fix it.\n\nThanks a lot for looking into and fixing this. I have verified that\nthe patch indeed fixes the issue for me.\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"114834","messageId":"1243455224-3463-1-git-send-email-drizzd@aon.at","threadId":"19500","inReplyTo":"76718490905260727v7261391uab169167978e4522@mail.gmail.com","subject":"[PATCH 1/2] fix segfault showing an empty remote","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-27T20:13:43Z","receivedAt":"2009-05-27T20:13:43Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"In case of an empty list, the search for its tail caused a\nNULL-pointer dereference.\n\nSigned-off-by: Clemens Buchacher \nReported-by: Erik Faye-Lund \nAcked-by: Jay Soffian \n---\n builtin-remote.c | 8 ++++----\n t/t5505-remote.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 71abf68..fda9a54 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -299,11 +299,11 @@ static int get_push_ref_states(const struct ref *remote_refs,\n \t\treturn 0;\n \n \tlocal_refs = get_local_heads();\n-\tref = push_map = copy_ref_list(remote_refs);\n-\twhile (ref->next)\n-\t\tref = ref->next;\n-\tpush_tail = &ref->next;\n+\tpush_map = copy_ref_list(remote_refs);\n \n+\tpush_tail = &push_map;\n+\twhile (*push_tail)\n+\t\tpush_tail = &((*push_tail)->next);\n \tmatch_refs(local_refs, push_map, &push_tail, remote->push_refspec_nr,\n \t\t remote->push_refspec, MATCH_REFS_NONE);\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 5ec668d..e70246b 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -494,5 +494,15 @@ test_expect_success 'remote prune to cause a dangling symref' '\n \tgrep \"dangling symref\" err\n '\n \n+test_expect_success 'show empty remote' '\n+\n+\ttest_create_repo empty &&\n+\tgit clone empty empty-clone &&\n+\t(\n+\t\tcd empty-clone &&\n+\t\tgit remote show origin\n+\t)\n+'\n+\n test_done\n \n-- \n1.6.3.1.147.g637c3\n"},{"id":"114835","messageId":"1243455224-3463-2-git-send-email-drizzd@aon.at","threadId":"19500","inReplyTo":"1243455224-3463-1-git-send-email-drizzd@aon.at","subject":"[PATCH 2/2] match_refs: search ref list tail internally","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-27T20:13:44Z","receivedAt":"2009-05-27T20:13:44Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Avoid code duplication by moving list tail search to match_refs().\n\nThis does not change the semantics, not even for http-push. The NULL\ntest for remote_tail was redundant.\n\nSigned-off-by: Clemens Buchacher \n---\n\nOn Tue, May 26, 2009 at 10:27:15AM -0400, Jay Soffian wrote:\n> On Mon, May 25, 2009 at 3:01 PM, Clemens Buchacher wrote:\n> > I simply copied this from other uses of match_refs. I wonder if this calls\n> > for a find_link_ref_tail() function, but I didn't know where to put it.\n>-\n> Since the primary use case is in combination with match_refs(),\n> remote.[ch] perhaps.\n\nFair enough. Here it goes.\n\n builtin-remote.c | 7 ++-----\n builtin-send-pack.c | 9 ++-------\n http-push.c | 4 +---\n remote.c | 17 +++++++++++++----\n remote.h | 2 +-\n transport.c | 6 +-----\n 6 files changed, 20 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex fda9a54..9248e0a 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -294,17 +294,14 @@ static int get_push_ref_states(const struct ref *remote_refs,\n \tstruct ref_states *states)\n {\n \tstruct remote *remote = states->remote;\n-\tstruct ref *ref, *local_refs, *push_map, **push_tail;\n+\tstruct ref *ref, *local_refs, *push_map;\n \tif (remote->mirror)\n \t\treturn 0;\n \n \tlocal_refs = get_local_heads();\n \tpush_map = copy_ref_list(remote_refs);\n \n-\tpush_tail = &push_map;\n-\twhile (*push_tail)\n-\t\tpush_tail = &((*push_tail)->next);\n-\tmatch_refs(local_refs, push_map, &push_tail, remote->push_refspec_nr,\n+\tmatch_refs(local_refs, &push_map, remote->push_refspec_nr,\n \t\t remote->push_refspec, MATCH_REFS_NONE);\n \n \tstates->push.strdup_strings = 1;\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex be3b092..c375a3d 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -473,7 +473,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint fd[2];\n \tstruct child_process *conn;\n \tstruct extra_have_objects extra_have;\n-\tstruct ref *remote_refs, **remote_tail, *local_refs;\n+\tstruct ref *remote_refs, *local_refs;\n \tint ret;\n \tint send_all = 0;\n \tconst char *receivepack = \"git-receive-pack\";\n@@ -567,13 +567,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\tflags |= MATCH_REFS_MIRROR;\n \n \t/* match them up */\n-\tremote_tail = &remote_refs;\n-\twhile (*remote_tail)\n-\t\tremote_tail = &((*remote_tail)->next);\n-\tif (match_refs(local_refs, remote_refs, &remote_tail,\n-\t\t nr_refspecs, refspecs, flags)) {\n+\tif (match_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))\n \t\treturn -1;\n-\t}\n \n \tret = send_pack(&args, fd, conn, remote_refs, &extra_have);\n \ndiff --git a/http-push.c b/http-push.c\nindex dac2c6e..02f1ff5 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -2310,9 +2310,7 @@ int main(int argc, char **argv)\n \t}\n \n \t/* match them up */\n-\tif (!remote_tail)\n-\t\tremote_tail = &remote_refs;\n-\tif (match_refs(local_refs, remote_refs, &remote_tail,\n+\tif (match_refs(local_refs, &remote_refs,\n \t\t nr_refspec, (const char **) refspec, push_all)) {\n \t\trc = -1;\n \t\tgoto cleanup;\ndiff --git a/remote.c b/remote.c\nindex 2c3e905..f3c4b99 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1085,12 +1085,20 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,\n \t\treturn NULL;\n }\n \n+static struct ref** tail_ref(struct ref** head)\n+{\n+\tstruct ref **tail = head;\n+\twhile (*tail)\n+\t\ttail = &((*tail)->next);\n+\treturn tail;\n+}\n+\n /*\n * Note. This is used only by \"push\"; refspec matching rules for\n * push and fetch are subtly different, so do not try to reuse it\n * without thinking.\n */\n-int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n+int match_refs(struct ref *src, struct ref **dst,\n \t int nr_refspec, const char **refspec, int flags)\n {\n \tstruct refspec *rs;\n@@ -1098,13 +1106,14 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \tint send_mirror = flags & MATCH_REFS_MIRROR;\n \tint errs;\n \tstatic const char *default_refspec[] = { \":\", 0 };\n+\tstruct ref **dst_tail = tail_ref(dst);\n \n \tif (!nr_refspec) {\n \t\tnr_refspec = 1;\n \t\trefspec = default_refspec;\n \t}\n \trs = parse_push_refspec(nr_refspec, (const char **) refspec);\n-\terrs = match_explicit_refs(src, dst, dst_tail, rs, nr_refspec);\n+\terrs = match_explicit_refs(src, *dst, &dst_tail, rs, nr_refspec);\n \n \t/* pick the remainder */\n \tfor ( ; src; src = src->next) {\n@@ -1134,7 +1143,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t\t\t\t\t\t dst_side, &dst_name))\n \t\t\t\tdie(\"Didn't think it matches any more\");\n \t\t}\n-\t\tdst_peer = find_ref_by_name(dst, dst_name);\n+\t\tdst_peer = find_ref_by_name(*dst, dst_name);\n \t\tif (dst_peer) {\n \t\t\tif (dst_peer->peer_ref)\n \t\t\t\t/* We're already sending something to this ref. */\n@@ -1150,7 +1159,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t\t\t\tgoto free_name;\n \n \t\t\t/* Create a new one and link it */\n-\t\t\tdst_peer = make_linked_ref(dst_name, dst_tail);\n+\t\t\tdst_peer = make_linked_ref(dst_name, &dst_tail);\n \t\t\thashcpy(dst_peer->new_sha1, src->new_sha1);\n \t\t}\n \t\tdst_peer->peer_ref = copy_ref(src);\ndiff --git a/remote.h b/remote.h\nindex 99706a8..257a555 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -85,7 +85,7 @@ void ref_remove_duplicates(struct ref *ref_map);\n int valid_fetch_refspec(const char *refspec);\n struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n \n-int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n+int match_refs(struct ref *src, struct ref **dst,\n \t int nr_refspec, const char **refspec, int all);\n \n /*\ndiff --git a/transport.c b/transport.c\nindex 17891d5..d8a2392 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1003,7 +1003,6 @@ int transport_push(struct transport *transport,\n \tif (transport->push_refs) {\n \t\tstruct ref *remote_refs =\n \t\t\ttransport->get_refs_list(transport, 1);\n-\t\tstruct ref **remote_tail;\n \t\tstruct ref *local_refs = get_local_heads();\n \t\tint match_flags = MATCH_REFS_NONE;\n \t\tint verbose = flags & TRANSPORT_PUSH_VERBOSE;\n@@ -1014,10 +1013,7 @@ int transport_push(struct transport *transport,\n \t\tif (flags & TRANSPORT_PUSH_MIRROR)\n \t\t\tmatch_flags |= MATCH_REFS_MIRROR;\n \n-\t\tremote_tail = &remote_refs;\n-\t\twhile (*remote_tail)\n-\t\t\tremote_tail = &((*remote_tail)->next);\n-\t\tif (match_refs(local_refs, remote_refs, &remote_tail,\n+\t\tif (match_refs(local_refs, &remote_refs,\n \t\t\t refspec_nr, refspec, match_flags)) {\n \t\t\treturn -1;\n \t\t}\n-- \n1.6.3.1.147.g637c3\n"},{"id":"114891","messageId":"7vtz35hfk7.fsf@alter.siamese.dyndns.org","threadId":"19500","inReplyTo":"1243455224-3463-2-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 2/2] match_refs: search ref list tail internally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-28T07:06:32Z","receivedAt":"2009-05-28T07:06:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> Avoid code duplication by moving list tail search to match_refs().\n>\n> This does not change the semantics, not even for http-push. The NULL\n> test for remote_tail was redundant.\n\nThe existing program (and the result after the patch) open-codes too much\ninside the huge main() function, and it is extremely painful to follow\nwhat is going on.\n\nBut it is not that \"the NULL test was redundant\" that I'd be worried\nabout. In that unreadable main() function:\n\n - get_dav_remote_heads() is called, which asks the other end what the\n remote refs are; the result is queued in the linked list whose head is\n remote_refs and tail is remote_tail;\n\n - the resulting remote_tail is given to match_refs() to further grow the\n linked list. The function will queue new \"struct ref\" instances\n (e.g. by making a call to match_explicit_refs()) at the end of the\n list.\n\n The caller used to pass \"here is the end of the list\" variable to it,\n but now you compute (perhaps redundantly) the end of the ref list by\n tangling from dst to its tail, yourself, and match_refs() links new\n elements at the tail of the list correctly.\n\n But what happens to the calling program's remote_tail variable after\n match_refs() returns? The code used to guarantee that it always point\n at the real end of the list, but that guarantee is now gone.\n\nIt so happens that http-push.c never looked at remote_tail to do further\nprocessing on the list after match_refs() returned, and that is why your\npatch does not break http-push.c. Any third-party patch to http-push.c\nthat relied on the old guarantee will textually merge cleanly but will\nsubtly break with this change.\n\nOther parts of this patch removes the local \"remote_tail\" variables, and\nit is very clear that they do not have this problem; any third-parth patch\nwill break if they used remote_tail after match_refs() returned, so this\nchange is a safe one for them.\n\nI wonder what interaction this change will have with the http-push\nclean-up Ray Chuan has been working on...\n"},{"id":"114907","messageId":"20090528092624.GA8787@localhost","threadId":"19500","inReplyTo":"7vtz35hfk7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] match_refs: search ref list tail internally","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-28T09:26:24Z","receivedAt":"2009-05-28T09:26:24Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Thu, May 28, 2009 at 12:06:32AM -0700, Junio C Hamano wrote:\n> It so happens that http-push.c never looked at remote_tail to do further\n> processing on the list after match_refs() returned, and that is why your\n> patch does not break http-push.c.\n> \n> Any third-party patch to http-push.c\n> that relied on the old guarantee will textually merge cleanly but will\n> subtly break with this change.\n> \n> Other parts of this patch removes the local \"remote_tail\" variables, and\n> it is very clear that they do not have this problem; any third-parth patch\n> will break if they used remote_tail after match_refs() returned, so this\n> change is a safe one for them.\n\nYes, I tried to get rid of remote_tail entirely, but I could not come up\nwith a solution immediately. We could insert to the front of the list\ninstead. I'll try that.\n\n> I wonder what interaction this change will have with the http-push\n> clean-up Ray Chuan has been working on...\n\nFor me, t5540 currently segfaults on pu. I'm looking into it.\n"},{"id":"115134","messageId":"20090531142648.GA25946@localhost","threadId":"19500","inReplyTo":"7vtz35hfk7.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] match_refs: search ref list tail internally","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-31T14:26:48Z","receivedAt":"2009-05-31T14:26:48Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Avoid code duplication by moving list tail search to match_refs().\n\nThis does not change the semantics, except for http-push, which now inserts\nto the front of the ref list in order to get rid of the global remote_tail.\n\nSigned-off-by: Clemens Buchacher \n---\n\nOn Thu, May 28, 2009 at 12:06:32AM -0700, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> > Avoid code duplication by moving list tail search to match_refs().\n> >\n> > This does not change the semantics, not even for http-push. The NULL\n> > test for remote_tail was redundant.\n[...]\n> Other parts of this patch removes the local \"remote_tail\" variables, and\n> it is very clear that they do not have this problem; any third-parth patch\n> will break if they used remote_tail after match_refs() returned, so this\n> change is a safe one for them.\n\nAlright, so here's the version which removes remote_tail.\n\n> I wonder what interaction this change will have with the http-push\n> clean-up Ray Chuan has been working on...\n\nI did test on rctay's http-fix-push-fetching branch in\ngit://github.com/rctay/git.git with the most recent fixes.\n\nI also squashed in your style fixes from the version currently in pu.\nThanks.\n\nClemens\n\n builtin-remote.c | 7 ++-----\n builtin-send-pack.c | 9 ++-------\n http-push.c | 11 ++++-------\n remote.c | 17 +++++++++++++----\n remote.h | 2 +-\n transport.c | 6 +-----\n 6 files changed, 23 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex fda9a54..9248e0a 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -294,17 +294,14 @@ static int get_push_ref_states(const struct ref *remote_refs,\n \tstruct ref_states *states)\n {\n \tstruct remote *remote = states->remote;\n-\tstruct ref *ref, *local_refs, *push_map, **push_tail;\n+\tstruct ref *ref, *local_refs, *push_map;\n \tif (remote->mirror)\n \t\treturn 0;\n \n \tlocal_refs = get_local_heads();\n \tpush_map = copy_ref_list(remote_refs);\n \n-\tpush_tail = &push_map;\n-\twhile (*push_tail)\n-\t\tpush_tail = &((*push_tail)->next);\n-\tmatch_refs(local_refs, push_map, &push_tail, remote->push_refspec_nr,\n+\tmatch_refs(local_refs, &push_map, remote->push_refspec_nr,\n \t\t remote->push_refspec, MATCH_REFS_NONE);\n \n \tstates->push.strdup_strings = 1;\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex be3b092..c375a3d 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -473,7 +473,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint fd[2];\n \tstruct child_process *conn;\n \tstruct extra_have_objects extra_have;\n-\tstruct ref *remote_refs, **remote_tail, *local_refs;\n+\tstruct ref *remote_refs, *local_refs;\n \tint ret;\n \tint send_all = 0;\n \tconst char *receivepack = \"git-receive-pack\";\n@@ -567,13 +567,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\tflags |= MATCH_REFS_MIRROR;\n \n \t/* match them up */\n-\tremote_tail = &remote_refs;\n-\twhile (*remote_tail)\n-\t\tremote_tail = &((*remote_tail)->next);\n-\tif (match_refs(local_refs, remote_refs, &remote_tail,\n-\t\t nr_refspecs, refspecs, flags)) {\n+\tif (match_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))\n \t\treturn -1;\n-\t}\n \n \tret = send_pack(&args, fd, conn, remote_refs, &extra_have);\n \ndiff --git a/http-push.c b/http-push.c\nindex 10f64e3..c44cedd 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1850,7 +1850,7 @@ static int update_remote(unsigned char *sha1, struct remote_lock *lock)\n \treturn 1;\n }\n \n-static struct ref *remote_refs, **remote_tail;\n+static struct ref *remote_refs;\n \n static void one_remote_ref(char *refname)\n {\n@@ -1880,13 +1880,12 @@ static void one_remote_ref(char *refname)\n \t\t}\n \t}\n \n-\t*remote_tail = ref;\n-\tremote_tail = &ref->next;\n+\tref->next = remote_refs;\n+\tremote_refs = ref;\n }\n \n static void get_dav_remote_heads(void)\n {\n-\tremote_tail = &remote_refs;\n \tremote_ls(\"refs/\", (PROCESS_FILES | PROCESS_DIRS | RECURSIVE), process_ls_ref, NULL);\n }\n \n@@ -2341,9 +2340,7 @@ int main(int argc, char **argv)\n \t}\n \n \t/* match them up */\n-\tif (!remote_tail)\n-\t\tremote_tail = &remote_refs;\n-\tif (match_refs(local_refs, remote_refs, &remote_tail,\n+\tif (match_refs(local_refs, &remote_refs,\n \t\t nr_refspec, (const char **) refspec, push_all)) {\n \t\trc = -1;\n \t\tgoto cleanup;\ndiff --git a/remote.c b/remote.c\nindex 2c3e905..08a5964 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1085,12 +1085,20 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,\n \t\treturn NULL;\n }\n \n+static struct ref **tail_ref(struct ref **head)\n+{\n+\tstruct ref **tail = head;\n+\twhile (*tail)\n+\t\ttail = &((*tail)->next);\n+\treturn tail;\n+}\n+\n /*\n * Note. This is used only by \"push\"; refspec matching rules for\n * push and fetch are subtly different, so do not try to reuse it\n * without thinking.\n */\n-int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n+int match_refs(struct ref *src, struct ref **dst,\n \t int nr_refspec, const char **refspec, int flags)\n {\n \tstruct refspec *rs;\n@@ -1098,13 +1106,14 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \tint send_mirror = flags & MATCH_REFS_MIRROR;\n \tint errs;\n \tstatic const char *default_refspec[] = { \":\", 0 };\n+\tstruct ref **dst_tail = tail_ref(dst);\n \n \tif (!nr_refspec) {\n \t\tnr_refspec = 1;\n \t\trefspec = default_refspec;\n \t}\n \trs = parse_push_refspec(nr_refspec, (const char **) refspec);\n-\terrs = match_explicit_refs(src, dst, dst_tail, rs, nr_refspec);\n+\terrs = match_explicit_refs(src, *dst, &dst_tail, rs, nr_refspec);\n \n \t/* pick the remainder */\n \tfor ( ; src; src = src->next) {\n@@ -1134,7 +1143,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t\t\t\t\t\t dst_side, &dst_name))\n \t\t\t\tdie(\"Didn't think it matches any more\");\n \t\t}\n-\t\tdst_peer = find_ref_by_name(dst, dst_name);\n+\t\tdst_peer = find_ref_by_name(*dst, dst_name);\n \t\tif (dst_peer) {\n \t\t\tif (dst_peer->peer_ref)\n \t\t\t\t/* We're already sending something to this ref. */\n@@ -1150,7 +1159,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t\t\t\tgoto free_name;\n \n \t\t\t/* Create a new one and link it */\n-\t\t\tdst_peer = make_linked_ref(dst_name, dst_tail);\n+\t\t\tdst_peer = make_linked_ref(dst_name, &dst_tail);\n \t\t\thashcpy(dst_peer->new_sha1, src->new_sha1);\n \t\t}\n \t\tdst_peer->peer_ref = copy_ref(src);\ndiff --git a/remote.h b/remote.h\nindex 99706a8..257a555 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -85,7 +85,7 @@ void ref_remove_duplicates(struct ref *ref_map);\n int valid_fetch_refspec(const char *refspec);\n struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n \n-int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n+int match_refs(struct ref *src, struct ref **dst,\n \t int nr_refspec, const char **refspec, int all);\n \n /*\ndiff --git a/transport.c b/transport.c\nindex 17891d5..d8a2392 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1003,7 +1003,6 @@ int transport_push(struct transport *transport,\n \tif (transport->push_refs) {\n \t\tstruct ref *remote_refs =\n \t\t\ttransport->get_refs_list(transport, 1);\n-\t\tstruct ref **remote_tail;\n \t\tstruct ref *local_refs = get_local_heads();\n \t\tint match_flags = MATCH_REFS_NONE;\n \t\tint verbose = flags & TRANSPORT_PUSH_VERBOSE;\n@@ -1014,10 +1013,7 @@ int transport_push(struct transport *transport,\n \t\tif (flags & TRANSPORT_PUSH_MIRROR)\n \t\t\tmatch_flags |= MATCH_REFS_MIRROR;\n \n-\t\tremote_tail = &remote_refs;\n-\t\twhile (*remote_tail)\n-\t\t\tremote_tail = &((*remote_tail)->next);\n-\t\tif (match_refs(local_refs, remote_refs, &remote_tail,\n+\t\tif (match_refs(local_refs, &remote_refs,\n \t\t\t refspec_nr, refspec, match_flags)) {\n \t\t\treturn -1;\n \t\t}\n-- \n1.6.3.1.147.g637c3\n"}]}