{"thread":{"id":"10724","subject":"[PATCH 3/3] git-fetch: avoid local fetching from alternate (again)","startedAt":"2007-11-08T08:00:58Z","lastAt":"2007-11-08T22:27:02Z","messageCount":5,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"58816","messageId":"20071108080058.GC16690@spearce.org","threadId":"10724","inReplyTo":null,"subject":"[PATCH 3/3] git-fetch: avoid local fetching from alternate (again)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-08T08:00:58Z","receivedAt":"2007-11-08T08:00:58Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Back in e3c6f240fd9c5bdeb33f2d47adc859f37935e2df Junio taught\ngit-fetch to avoid copying objects when we are fetching from\na repository that is already registered as an alternate object\ndatabase. In such a case there is no reason to copy any objects\nas we can already obtain them through the alternate.\n\nHowever we need to ensure the objects are all reachable, so we\nrun `git rev-list --objects $theirs --not --all` to verify this.\nIf any object is missing or unreadable then we need to fetch/copy\nthe objects from the remote. When a missing object is detected\nthe git-rev-list process will exit with a non-zero exit status,\nmaking this condition quite easy to detect.\n\nAlthough git-fetch is currently a builtin (and so is rev-list)\nwe cannot invoke the traverse_objects() API at this point in the\ntransport code. The object walker within traverse_objects() calls\ndie() as soon as it finds an object it cannot read. If that happens\nwe want to resume the fetch process by calling do_fetch_pack().\nTo get aroaund this we spawn git-rev-list into a background process\nto prevent a die() from killing the foreground fetch process,\nthus allowing the fetch process to resume into do_fetch_pack()\nif copying is necessary.\n\nWe aren't interested in the output of rev-list (a list of SHA-1\nobject names that are reachable) or its errors (a \"spurious\" error\nabout an object not being found as we need to copy it) so we redirect\nboth stdout and stderr to /dev/null.\n\nWe run this git-rev-list based check before any fetch as we may\nalready have the necessary objects local from a prior fetch. If we\ndon't then its very likely the first $theirs object listed on the\ncommand line won't exist locally and git-rev-list will die very\nquickly, allowing us to start the network transfer. This test even\non remote URLs may save bandwidth if someone runs `git pull origin`,\nsees a merge conflict, resets out, then redoes the same pull just\na short time later. If the remote hasn't changed between the two\npulls and the local repository hasn't had git-gc run in it then\nthere is probably no need to perform network transfer as all of\nthe objects are local.\n\nDocumentation for the new fetch_local_nocopy function was suggested\nand written by Junio, based on his original comment in git-fetch.sh.\n\nSigned-off-by: Shawn O. Pearce \n---\n transport.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 65 insertions(+), 3 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex f4577b7..d37864e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -615,17 +615,76 @@ static struct ref *get_refs_via_connect(struct transport *transport)\n \treturn refs;\n }\n \n+/*\n+ * We would want to bypass the object transfer altogether if\n+ * everything we are going to fetch already exists and connected\n+ * locally.\n+ *\n+ * The refs we are going to fetch are in to_fetch (nr_heads in\n+ * total). If running\n+ *\n+ * $ git-rev-list --objects to_fetch[0] to_fetch[1] ... --not --all\n+ *\n+ * does not error out, that means everything reachable from the\n+ * refs we are going to fetch exists and is connected to some of\n+ * our existing refs.\n+ */\n+static int fetch_local_nocopy(struct transport *transport,\n+\t\t\t int nr_heads, struct ref **to_fetch)\n+{\n+\tstruct git_transport_data *data = transport->data;\n+\tstruct child_process revlist;\n+\tchar **argv;\n+\tint i, j, err;\n+\n+\t/*\n+\t * If we are deepening a shallow clone we already have these\n+\t * objects reachable. Running rev-list here will return with\n+\t * a good (0) exit status and we'll bypass the fetch that we\n+\t * really need to perform. Claiming failure now will ensure\n+\t * we perform the network exchange to deepen our history.\n+\t */\n+\tif (data->depth)\n+\t\treturn -1;\n+\n+\ti = 0;\n+\targv = xmalloc(sizeof(*argv) * (nr_heads + 6));\n+\targv[i++] = xstrdup(\"rev-list\");\n+\targv[i++] = xstrdup(\"--no-output\");\n+\targv[i++] = xstrdup(\"--objects\");\n+\tfor (j = 0; j < nr_heads; j++)\n+\t\targv[i++] = xstrdup(sha1_to_hex(to_fetch[j]->old_sha1));\n+\targv[i++] = xstrdup(\"--not\");\n+\targv[i++] = xstrdup(\"--all\");\n+\targv[i++] = NULL;\n+\n+\tmemset(&revlist, 0, sizeof(revlist));\n+\trevlist.argv = (const char**)argv;\n+\trevlist.git_cmd = 1;\n+\trevlist.no_stdin = 1;\n+\trevlist.no_stdout = 1;\n+\trevlist.no_stderr = 1;\n+\terr = run_command(&revlist);\n+\n+\tfor (i = 0; argv[i]; i++)\n+\t\tfree(argv[i]);\n+\tfree(argv);\n+\treturn err;\n+}\n+\n static int fetch_refs_via_pack(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct git_transport_data *data = transport->data;\n-\tchar **heads = xmalloc(nr_heads * sizeof(*heads));\n-\tchar **origh = xmalloc(nr_heads * sizeof(*origh));\n+\tchar **heads, **origh;\n \tstruct ref *refs;\n-\tchar *dest = xstrdup(transport->url);\n+\tchar *dest;\n \tstruct fetch_pack_args args;\n \tint i;\n \n+\tif (!fetch_local_nocopy(transport, nr_heads, to_fetch))\n+\t\treturn 0;\n+\n \tmemset(&args, 0, sizeof(args));\n \targs.uploadpack = data->uploadpack;\n \targs.keep_pack = data->keep;\n@@ -634,6 +693,9 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.verbose = transport->verbose > 0;\n \targs.depth = data->depth;\n \n+\theads = xmalloc(nr_heads * sizeof(*heads));\n+\torigh = xmalloc(nr_heads * sizeof(*origh));\n+\tdest = xstrdup(transport->url);\n \tfor (i = 0; i < nr_heads; i++)\n \t\torigh[i] = heads[i] = xstrdup(to_fetch[i]->name);\n \trefs = fetch_pack(&args, dest, nr_heads, heads, &transport->pack_lockfile);\n-- \n1.5.3.5.1590.gfadfad\n"},{"id":"58852","messageId":"20071108100039.GM14735@spearce.org","threadId":"10724","inReplyTo":"20071108080058.GC16690@spearce.org","subject":"Re: [PATCH 3/3] git-fetch: avoid local fetching from alternate (again)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-08T10:00:40Z","receivedAt":"2007-11-08T10:00:40Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" wrote:\n> Back in e3c6f240fd9c5bdeb33f2d47adc859f37935e2df Junio taught\n> git-fetch to avoid copying objects when we are fetching from\n> a repository that is already registered as an alternate object\n> database. In such a case there is no reason to copy any objects\n> as we can already obtain them through the alternate.\n\nI haven't figured it out yet but this patch seriously breaks\nt5515-fetch-merge-logic. For some reason the tag tag-three-file is\nnot being included in .git/FETCH_HEAD as a not-for-merge ref, but all\nof the test vectors are expecting it to be present. Prior to this\npatch it was included and I don't think the test vectors are wrong.\n\nIf I run git-fetch from outside the test library it does the right\nthing and fetches this annotated tag pointing to a blob just fine.\nBut during the test vector it never even mentions that tag as part\nof the status output, nor does it include it into .git/FETCH_HEAD.\nIts almost like the tag ain't there.\n\nI'm starting to suspect heap corruption again in builtin-fetch.\nThis patch alters the malloc() calls we are doing and may be shifting\nsomething around just enough in memory to cause a data overwrite or\nsomething and that's why this tag just drops out of the linked list?\nBut then why does that happen in the test suite but not outside.\nMaybe because the test suite is setting environment variables that\nI'm not and the impact of those combined with these additional\nmallocs is what is breaking it? *sigh*\n\n-- \nShawn.\n"},{"id":"58853","messageId":"7vhcjx2gq5.fsf@gitster.siamese.dyndns.org","threadId":"10724","inReplyTo":"20071108100039.GM14735@spearce.org","subject":"Re: [PATCH 3/3] git-fetch: avoid local fetching from alternate (again)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-08T10:07:14Z","receivedAt":"2007-11-08T10:07:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> I'm starting to suspect heap corruption again in builtin-fetch.\n> This patch alters the malloc() calls we are doing and may be shifting\n> something around just enough in memory to cause a data overwrite or\n> something and that's why this tag just drops out of the linked list?\n> But then why does that happen in the test suite but not outside.\n> Maybe because the test suite is setting environment variables that\n> I'm not and the impact of those combined with these additional\n> mallocs is what is breaking it? *sigh*\n\nThanks for working hard on this one.\n\nIt is starting to look like today was \"let's kill other people's\nbugs\" day. I'd go to bed before I completely miss sleep, which\nI often end up doing when thinking too long about bugs.\n"},{"id":"58859","messageId":"20071108112254.GN14735@spearce.org","threadId":"10724","inReplyTo":"7vhcjx2gq5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] git-fetch: avoid local fetching from alternate (again)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-08T11:22:54Z","receivedAt":"2007-11-08T11:22:54Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > I'm starting to suspect heap corruption again in builtin-fetch.\n> > This patch alters the malloc() calls we are doing and may be shifting\n> > something around just enough in memory to cause a data overwrite or\n> > something and that's why this tag just drops out of the linked list?\n> > But then why does that happen in the test suite but not outside.\n> > Maybe because the test suite is setting environment variables that\n> > I'm not and the impact of those combined with these additional\n> > mallocs is what is breaking it? *sigh*\n\nFound it. This ain't pretty. Remember, what's failing in the test\nsuite is we aren't getting \"tag-three-file\" automatically followed\nduring fetch. This is an annotated tag referring to a blob.\n\nHere's the *WAY WRONG FIX THAT SHOULD NOT EVER BE APPLIED*:\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 847db73..a935b5a 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -389,7 +389,7 @@ static struct ref *find_non_local_tags(struct transport *transport,\n \n \t\tif (!path_list_has_path(&existing_refs, ref_name) &&\n \t\t !path_list_has_path(&new_refs, ref_name) &&\n-\t\t lookup_object(ref->old_sha1)) {\n+\t\t has_sha1_file(ref->old_sha1)) {\n \t\t\tpath_list_insert(ref_name, &new_refs);\n \n \t\t\trm = alloc_ref(strlen(ref_name) + 1);\n\n(I know Junio knows why this patch shouldn't be applied.\n Its exactly why quickfetch calls rev-list --objects...)\n\nThe problem here is my quickfetch patch is bypassing the internal\ncall to fetch-pack. Which means we never do object handshaking,\nand thus never allocate struct object* for the things we had been\ntalking about (because we never talked about them!). Or in the\ncase of commits, their trees and parents too. Thus the call above\nto lookup_object() for a blob fails, as we never tried to parse it\nbefore in fetch-pack.\n\nNow I'm really not sure why we have blob objects allocated into\nmemory for lookup_object() during fetch-pack, but apparently we do.\nAt least during this test vector. It doesn't make sense if we are\nonly talking about commits (and their parents). I'd expect the\nroot trees to have objects (when the commit buffer was parsed for\ngraph traversal) but not blobs.\n\nMaybe some smart individual will come up with the right solution to\nkeep automatic tag following enabled (safely!) while also allowing\nus to use quickfetch. There's got to be a solution that doesn't\nimplicitly rely upon the handshaking we just happened to do with\nthe remote peer...\n\nMe, I'm off to catch a few hours of sleep.\n\n-- \nShawn.\n"},{"id":"58947","messageId":"20071108222702.GO14735@spearce.org","threadId":"10724","inReplyTo":"20071108112254.GN14735@spearce.org","subject":"Re: [PATCH 3/3] git-fetch: avoid local fetching from alternate (again)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-08T22:27:02Z","receivedAt":"2007-11-08T22:27:02Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" wrote:\n> Junio C Hamano wrote:\n> > \"Shawn O. Pearce\" writes:\n> > \n> > > I'm starting to suspect heap corruption again in builtin-fetch.\n> > > This patch alters the malloc() calls we are doing and may be shifting\n> > > something around just enough in memory to cause a data overwrite or\n> > > something and that's why this tag just drops out of the linked list?\n> > > But then why does that happen in the test suite but not outside.\n> > > Maybe because the test suite is setting environment variables that\n> > > I'm not and the impact of those combined with these additional\n> > > mallocs is what is breaking it? *sigh*\n> \n> Found it. This ain't pretty. Remember, what's failing in the test\n> suite is we aren't getting \"tag-three-file\" automatically followed\n> during fetch. This is an annotated tag referring to a blob.\n\nHere's one possible way of fixing this. I think what we want is to\ninclude \"--not --all\" when we run the object listing immediately\nafter the fetch so we mark *only* those objects we just fetched\nand ignore the ones we already had reachable through existing refs.\nThis makes the walking cost proportional to the size of the fetch\nand not the size of the object database.\n\nUnfortunately when you insert \"--not\" in front of \"--all\" in the args\narray below the test vectors all fail again. Apparently we already\nhave the blob that \"tag-three-file\" refers to so its not something\nwe walk over during this process and the tag isn't followed.\nThat happens because the test repository we are fetching into\nalready has a number of objects through its refs/remotes/origin/*\nnamespace created by git-clone and the blob is already considered\nto be reachable.\n\nI'm not formatting this as a real patch because I'm not yet sure this\nis the right way to fix the automatic tag following code. Or the\nright way to abuse the revision walking machinary within git-fetch\nto implement such a feature. Comments would be most appreciated. :)\n\n\n--8>--\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 847db73..77f1901 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -8,6 +8,9 @@\n #include \"path-list.h\"\n #include \"remote.h\"\n #include \"transport.h\"\n+#include \"diff.h\"\n+#include \"list-objects.h\"\n+#include \"revision.h\"\n \n static const char fetch_usage[] = \"git-fetch [-a | --append] [--upload-pack ] [-f | --force] [--no-tags] [-t | --tags] [-k | --keep] [-u | --update-head-ok] [--depth ] [-v | --verbose] [ ...]\";\n \n@@ -335,11 +338,43 @@ static void store_updated_refs(const char *url, struct ref *ref_map)\n \tfclose(fp);\n }\n \n+static void mark_just_fetched_commit(struct commit *commit)\n+{\n+}\n+\n+static void mark_just_fetched_object(struct object_array_entry *p)\n+{\n+\tif (p->item->type == OBJ_BLOB && !has_sha1_file(p->item->sha1))\n+\t\tdie(\"missing blob object '%s'\", sha1_to_hex(p->item->sha1));\n+}\n+\n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n {\n \tint ret = transport_fetch_refs(transport, ref_map);\n-\tif (!ret)\n+\tif (!ret) {\n+\t\tconst char *args[] = { \"rev-list\", \"--objects\", \"--all\", NULL};\n+\t\tstruct rev_info revs;\n+\t\tstruct ref *ref;\n+\n+\t\tinit_revisions(&revs, NULL);\n+\t\tsetup_revisions(ARRAY_SIZE(args) - 1, args, &revs, NULL);\n+\t\tsave_commit_buffer = 0;\n+\t\ttrack_object_refs = 0;\n+\n+\t\tfor (ref = ref_map; ref; ref = ref->next) {\n+\t\t\tstruct object *o = parse_object(ref->old_sha1);\n+\t\t\tif (o)\n+\t\t\t\tadd_pending_object(&revs, o, \"(just-fetched)\");\n+\t\t}\n+\n+\t\tprepare_revision_walk(&revs);\n+\t\tmark_edges_uninteresting(revs.commits, &revs, NULL);\n+\t\ttraverse_commit_list(&revs,\n+\t\t\tmark_just_fetched_commit,\n+\t\t\tmark_just_fetched_object);\n+\n \t\tstore_updated_refs(transport->url, ref_map);\n+\t}\n \ttransport_unlock_pack(transport);\n \treturn ret;\n }\n@@ -365,6 +400,7 @@ static struct ref *find_non_local_tags(struct transport *transport,\n \tstruct ref *ref_map = NULL;\n \tstruct ref **tail = &ref_map;\n \tconst struct ref *ref;\n+\tstruct object *obj;\n \n \tfor_each_ref(add_existing, &existing_refs);\n \tfor (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {\n@@ -389,7 +425,8 @@ static struct ref *find_non_local_tags(struct transport *transport,\n \n \t\tif (!path_list_has_path(&existing_refs, ref_name) &&\n \t\t !path_list_has_path(&new_refs, ref_name) &&\n-\t\t lookup_object(ref->old_sha1)) {\n+\t\t (obj = lookup_object(ref->old_sha1)) &&\n+\t\t obj->flags & SEEN) {\n \t\t\tpath_list_insert(ref_name, &new_refs);\n \n \t\t\trm = alloc_ref(strlen(ref_name) + 1);\ndiff --git a/list-objects.c b/list-objects.c\nindex e5c88c2..78bf04c 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -170,4 +170,5 @@ void traverse_commit_list(struct rev_info *revs,\n \t}\n \tfor (i = 0; i < objects.nr; i++)\n \t\tshow_object(&objects.objects[i]);\n+\tfree(objects.objects);\n }\n\n-- \nShawn.\n"}]}