{"thread":{"id":"41639","subject":"[BUG?] fetch into shallow sends a large number of objects","startedAt":"2016-03-07T22:15:40Z","lastAt":"2016-03-11T18:16:35Z","messageCount":16,"participants":["Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"280337","messageId":"20160307221539.GA24034@sigill.intra.peff.net","threadId":"41639","inReplyTo":null,"subject":"[BUG?] fetch into shallow sends a large number of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-07T22:15:40Z","receivedAt":"2016-03-07T22:15:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I came across an interesting shallow-fetch case, and I suspect git is\ndoing something very sub-optimal.\n\nYou can reproduce with:\n\n\tgit clone --bare git://github.com/CocoaPods/Specs\n\tcd Specs.git\n\n\ttime git pack-objects --revs --thin --stdout \\\n\t --delta-base-offset --include-tag <<\\EOF | wc -c\n\td7a6d9295d718c6015be496880f1a293bdd89185\n\t--not\n\t067f265bb512c95b22b83ccd121b9facbddcf6b1\n\tEOF\n\n\ttime git pack-objects --revs --thin --stdout --shallow \\\n\t --delta-base-offset --include-tag <<\\EOF | wc -c\n\t--shallow ecd7ea6033fe8a05d5c21f3a54355fded6942659\n\td7a6d9295d718c6015be496880f1a293bdd89185\n\t--not\n\t067f265bb512c95b22b83ccd121b9facbddcf6b1\n\tEOF\n\nThe first is a non-shallow clone; it takes a few hundred milliseconds to\ngenerate the pack, and the result is a few hundred kilobytes. The second\nis a shallow clone (logged from a real-world request); it sends 200\ntimes as many objects, totaling 270MB, and takes almost a minute of CPU.\n\nI'm trying to figure out why that is, and whether git can do better.\n\nI think what's happening here is that the history looks like this:\n\n F ... ecd7ea6 ... 067f265 ... M ... d7a6d929\n \\ /\n X ....................... Y\n\nThat is, we are asking for 067f265 to d7a6d929, but that includes some\nmerge M which _crosses_ our grafted shallow-point ecd7ea6. That pulls in\nessentially all of the history for the entire (missing only the commits\nfrom the fork point F up to ecd7ea6).\n\nSo I _think_ that pack-objects is doing the best it can with the\ninformation it was given. But presumably in a shallow repo the user\nwould prefer to have a segmented history rather than pull in all of\nthose old commits.\n\nI don't know how the client invoked git, but we can guess what happened\nand simulate with:\n\n git tag shallow ecd7ea6033fe8a05d5c21f3a54355fded6942659\n git tag old 067f265bb512c95b22b83ccd121b9facbddcf6b1\n git tag new d7a6d9295d718c6015be496880f1a293bdd89185\n\n git clone --no-local --bare --branch=shallow --depth=1 . clone.git\n cd clone.git\n git fetch origin old:refs/tags/old\n git fetch origin new:refs/tags/new\n\nOf the two follow-up fetches in the clone, the first is reasonably fast\n(it just grabs a few new commits on top of the shallow base), but the\nsecond is expensive (it grabs the merge which pulls in the whole\nhistory). If we add \"--depth=1\" to each of those fetches, everything\nremains fast.\n\nIs this user error to call \"git fetch\" without \"--depth\" in the\nsubsequent cases? Or should git remember that we are in a shallow repo,\nand presume that the user by default wants to keep things shallow?\n\n-Peff\n"},{"id":"280343","messageId":"xmqqio0xn93q.fsf@gitster.mtv.corp.google.com","threadId":"41639","inReplyTo":"20160307221539.GA24034@sigill.intra.peff.net","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-07T23:47:53Z","receivedAt":"2016-03-07T23:47:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't know how the client invoked git, but we can guess what happened\n> and simulate with:\n>\n> git tag shallow ecd7ea6033fe8a05d5c21f3a54355fded6942659\n> git tag old 067f265bb512c95b22b83ccd121b9facbddcf6b1\n> git tag new d7a6d9295d718c6015be496880f1a293bdd89185\n>\n> git clone --no-local --bare --branch=shallow --depth=1 . clone.git\n> cd clone.git\n> git fetch origin old:refs/tags/old\n> git fetch origin new:refs/tags/new\n>\n> Of the two follow-up fetches in the clone, the first is reasonably fast\n> (it just grabs a few new commits on top of the shallow base), but the\n> second is expensive (it grabs the merge which pulls in the whole\n> history). If we add \"--depth=1\" to each of those fetches, everything\n> remains fast.\n>\n> Is this user error to call \"git fetch\" without \"--depth\" in the\n> subsequent cases? Or should git remember that we are in a shallow repo,\n> and presume that the user by default wants to keep things shallow?\n\nHmph, you shouldn't, and I somehow thought that you do not, have to\nexplicitly say things like \"--deepen\" to break the original\nshallowness, but your example illustrates that the logic to do so is\nnot well thought out. A new side branch will prevent you from\nhitting an already-known shallow cut-off and traverse down to the\nroot.\n\nGiving a random \"depth\" in subsequent fetch would however not work\nvery well, I suspect, as that is very prone to make the part of the\nhistory the user originally obtained, and presumably used to build\nher own history, into an island that is unconnected to the updated\ntip of the history. \n\nI also do not offhand think of a good way to use the topology or\ntimestamp to figure out the best \"depth\" to truncate the side branch\nat. The server side may be able to figure out that things before 'F'\nin your picture is not relevant for a client that has the shallow\ncut-off at 067f265, but the side branch can be forked arbitrarily\nlong in the past, or it may not even share the ancient part of the\nhistory and has its own root commit.\n"},{"id":"280344","messageId":"CACsJy8C-Wfh16jroWACEFybtnhwYX91GvgGwk8XxJRft4ZURSw@mail.gmail.com","threadId":"41639","inReplyTo":"xmqqio0xn93q.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-08T00:53:35Z","receivedAt":"2016-03-08T00:53:35Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 8, 2016 at 6:47 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I don't know how the client invoked git, but we can guess what happened\n>> and simulate with:\n>>\n>> git tag shallow ecd7ea6033fe8a05d5c21f3a54355fded6942659\n>> git tag old 067f265bb512c95b22b83ccd121b9facbddcf6b1\n>> git tag new d7a6d9295d718c6015be496880f1a293bdd89185\n>>\n>> git clone --no-local --bare --branch=shallow --depth=1 . clone.git\n>> cd clone.git\n>> git fetch origin old:refs/tags/old\n>> git fetch origin new:refs/tags/new\n>>\n>> Of the two follow-up fetches in the clone, the first is reasonably fast\n>> (it just grabs a few new commits on top of the shallow base), but the\n>> second is expensive (it grabs the merge which pulls in the whole\n>> history). If we add \"--depth=1\" to each of those fetches, everything\n>> remains fast.\n>>\n>> Is this user error to call \"git fetch\" without \"--depth\" in the\n>> subsequent cases? Or should git remember that we are in a shallow repo,\n>> and presume that the user by default wants to keep things shallow?\n>\n> Hmph, you shouldn't, and I somehow thought that you do not, have to\n> explicitly say things like \"--deepen\" to break the original\n> shallowness, but your example illustrates that the logic to do so is\n> not well thought out. A new side branch will prevent you from\n> hitting an already-known shallow cut-off and traverse down to the\n> root.\n\nYep. It \"works\" by design.\n\n> Giving a random \"depth\" in subsequent fetch would however not work\n> very well, I suspect, as that is very prone to make the part of the\n> history the user originally obtained, and presumably used to build\n> her own history, into an island that is unconnected to the updated\n> tip of the history.\n\nThe new --deepen, --shallow-since and --shallow-exclude should be\nbetter in this aspect and we can send them all the time without\naffecting original cut points. Well, deepen can't be used here because\nit needs shallow cut points as anchor in the first place.\n\n> I also do not offhand think of a good way to use the topology or\n> timestamp to figure out the best \"depth\" to truncate the side branch\n> at. The server side may be able to figure out that things before 'F'\n> in your picture is not relevant for a client that has the shallow\n> cut-off at 067f265, but the side branch can be forked arbitrarily\n> long in the past, or it may not even share the ancient part of the\n> history and has its own root commit.\n\nIf a shallow point can reach root without seeing another shallow\npoint, we can mark all reachable commits from it shallow. If it sees\nanother shallow point, maybe we can mark at the merge point of them..\nWe can also send \"here is --depth=10, but only apply it on new refs\".\nThat should mitigate the problem a bit. But I'm not sure if I can\nsolve it completely.\n-- \nDuy\n"},{"id":"280361","messageId":"20160308121444.GA18535@sigill.intra.peff.net","threadId":"41639","inReplyTo":"xmqqio0xn93q.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-08T12:14:44Z","receivedAt":"2016-03-08T12:14:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 07, 2016 at 03:47:53PM -0800, Junio C Hamano wrote:\n\n> > Is this user error to call \"git fetch\" without \"--depth\" in the\n> > subsequent cases? Or should git remember that we are in a shallow repo,\n> > and presume that the user by default wants to keep things shallow?\n> \n> Hmph, you shouldn't, and I somehow thought that you do not, have to\n> explicitly say things like \"--deepen\" to break the original\n> shallowness, but your example illustrates that the logic to do so is\n> not well thought out. A new side branch will prevent you from\n> hitting an already-known shallow cut-off and traverse down to the\n> root.\n> \n> Giving a random \"depth\" in subsequent fetch would however not work\n> very well, I suspect, as that is very prone to make the part of the\n> history the user originally obtained, and presumably used to build\n> her own history, into an island that is unconnected to the updated\n> tip of the history. \n\nYeah, if we just recorded the original \"--depth=1\", each fetch would\nmake its own little island. Eventually your \"shallow\" file would bloat.\nI'm not sure what that would do for the performance of subsequent\noperations.\n\n> I also do not offhand think of a good way to use the topology or\n> timestamp to figure out the best \"depth\" to truncate the side branch\n> at. The server side may be able to figure out that things before 'F'\n> in your picture is not relevant for a client that has the shallow\n> cut-off at 067f265, but the side branch can be forked arbitrarily\n> long in the past, or it may not even share the ancient part of the\n> history and has its own root commit.\n\nRight. I think conceptually that we want a graph cut, across all\nbranches. But you can't compute that cut at the time of the original\nshallow clone, as we might not even have the side branches yet. So it\nhas to be computed at the time of the subsequent fetch.\n\nYou could make an arbitrary cut across all branches based on the\ntimestamp of the --shallow bottom. That would create new shallow bottoms\non any parallel lines of development that are brought in, from \"around\nthe same time\". But I'm uncomfortable relying on timestamps, and I\nsuspect there are some pretty bad corner cases with skew.\n\nWhat if we instead marked the parent of a shallow commit UNINTERESTING?\nSomething like:\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex abed871..fc7a469 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2780,6 +2780,9 @@ static void get_object_list(int ac, const char **av)\n \t\t\t\tunsigned char sha1[20];\n \t\t\t\tif (get_sha1_hex(line + 10, sha1))\n \t\t\t\t\tdie(\"not an SHA-1 '%s'\", line + 10);\n+\t\t\t\thandle_revision_arg(xstrfmt(\"^%s^\", line + 10),\n+\t\t\t\t\t\t &revs, flags,\n+\t\t\t\t\t\t REVARG_CANNOT_BE_FILENAME);\n \t\t\t\tregister_shallow(sha1);\n \t\t\t\tuse_bitmap_index = 0;\n \t\t\t\tcontinue;\n\nIn this case it means we pull in all of that side branch, down to the\nfork point. If it went back to ancient history (or even had its own root\ncommit), then so be it; that's what the user asked for. But I think\nthis would cover the common case of people shallow-cloning and fetching\na normal workflow.\n\nThere are two problems, though:\n\n 1. We use UNINTERESTING for two things in pack-objects: to avoid\n sending the commits, but also to assume that the user has the\n objects, for making thin-pack deltas.\n\n We want only the former, but not the latter (if you apply my patch\n and try a fetch, you'll see that the client is missing bases from\n the resulting pack).\n\n 2. We've effectively re-shallowed the client at the fork point. We\n have to tell them that somehow, or they're going to complain that\n we didn't send them all of the objects.\n\nSo I think the solution to both is that we need to do a _separate_\ntraversal with all of the positive tips we're going to send, and the\nparents of any shallow commits the client has, to find their fork points\n(i.e., merge bases). And then we add those fork points to the shallow\nlist (grafting out their parents), and communicate them to the client to\nadd to its shallow setup.\n\n-Peff\n"},{"id":"280362","messageId":"20160308122126.GB18535@sigill.intra.peff.net","threadId":"41639","inReplyTo":"CACsJy8C-Wfh16jroWACEFybtnhwYX91GvgGwk8XxJRft4ZURSw@mail.gmail.com","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-08T12:21:26Z","receivedAt":"2016-03-08T12:21:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 08, 2016 at 07:53:35AM +0700, Duy Nguyen wrote:\n\n> > I also do not offhand think of a good way to use the topology or\n> > timestamp to figure out the best \"depth\" to truncate the side branch\n> > at. The server side may be able to figure out that things before 'F'\n> > in your picture is not relevant for a client that has the shallow\n> > cut-off at 067f265, but the side branch can be forked arbitrarily\n> > long in the past, or it may not even share the ancient part of the\n> > history and has its own root commit.\n> \n> If a shallow point can reach root without seeing another shallow\n> point, we can mark all reachable commits from it shallow. If it sees\n> another shallow point, maybe we can mark at the merge point of them..\n\nHmph, I read your email before sending my other response, but somehow I\ndidn't quite understand what you were saying. Now after having written\nmy long-winded other one, I think I just re-invented the same thing you\nare proposing here. ;)\n\n> We can also send \"here is --depth=10, but only apply it on new refs\".\n> That should mitigate the problem a bit. But I'm not sure if I can\n> solve it completely.\n\nI think \"new refs\" isn't something we can rely on. For example, in this\ncase the old history may have been merged in and the ref deleted before\nthe fetcher shows up.\n\n-Peff\n"},{"id":"280365","messageId":"CACsJy8Dk_g1O98UsDaeVS3VXmE2Mn5aR+w1OiFir+QwyJYLVZQ@mail.gmail.com","threadId":"41639","inReplyTo":"20160308121444.GA18535@sigill.intra.peff.net","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-08T12:33:43Z","receivedAt":"2016-03-08T12:33:43Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 8, 2016 at 7:14 PM, Jeff King wrote:\n> ...\n>\n> So I think the solution to both is that we need to do a _separate_\n> traversal with all of the positive tips we're going to send, and the\n> parents of any shallow commits the client has, to find their fork points\n> (i.e., merge bases). And then we add those fork points to the shallow\n> list (grafting out their parents), and communicate them to the client to\n> add to its shallow setup.\n\nGood news. We have the mechanism in place, I think.\nget_shallow_commits_by_rev_list() (from 'pu') will produce the right\nshallow points for sending back to the client if you pass \"--not\n\" to it. It's meant to be used for\n--shallow-exclude and --shallow-since, but if neither is given (nor\n--depth) I guess we can run it with current shallow points. I wonder\nif we can detect some common cases and avoid commit traversing this\nway though.\n-- \nDuy\n"},{"id":"280366","messageId":"20160308132524.GA22866@sigill.intra.peff.net","threadId":"41639","inReplyTo":"CACsJy8Dk_g1O98UsDaeVS3VXmE2Mn5aR+w1OiFir+QwyJYLVZQ@mail.gmail.com","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-08T13:25:24Z","receivedAt":"2016-03-08T13:25:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 08, 2016 at 07:33:43PM +0700, Duy Nguyen wrote:\n\n> On Tue, Mar 8, 2016 at 7:14 PM, Jeff King wrote:\n> > ...\n> >\n> > So I think the solution to both is that we need to do a _separate_\n> > traversal with all of the positive tips we're going to send, and the\n> > parents of any shallow commits the client has, to find their fork points\n> > (i.e., merge bases). And then we add those fork points to the shallow\n> > list (grafting out their parents), and communicate them to the client to\n> > add to its shallow setup.\n> \n> Good news. We have the mechanism in place, I think.\n> get_shallow_commits_by_rev_list() (from 'pu') will produce the right\n> shallow points for sending back to the client if you pass \"--not\n> \" to it. It's meant to be used for\n> --shallow-exclude and --shallow-since, but if neither is given (nor\n> --depth) I guess we can run it with current shallow points. I wonder\n> if we can detect some common cases and avoid commit traversing this\n> way though.\n\nI tried that, but I couldn't quite get it to work. I don't think we need\nany special rev-list, though; we can just find the boundary points of\nthat traversal and mark them as new shallows.\n\nI think this patch does roughly the right thing:\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4859535..da76f70 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -833,12 +833,41 @@ static void receive_needs(void)\n \t\tdeepen_by_rev_list(av.argc, av.argv, &shallows);\n \t\targv_array_clear(&av);\n \t}\n-\telse\n-\t\tif (shallows.nr > 0) {\n-\t\t\tint i;\n-\t\t\tfor (i = 0; i < shallows.nr; i++)\n-\t\t\t\tregister_shallow(shallows.objects[i].item->oid.hash);\n+\telse if (shallows.nr > 0) {\n+\t\tstruct rev_info revs;\n+\t\tstruct argv_array av = ARGV_ARRAY_INIT;\n+\t\tstruct commit *c;\n+\t\tint i;\n+\n+\t\targv_array_push(&av, \"rev-list\");\n+\t\targv_array_push(&av, \"--boundary\");\n+\t\tfor (i = 0; i < want_obj.nr; i++) {\n+\t\t\tstruct object *o = want_obj.objects[i].item;\n+\t\t\targv_array_push(&av, oid_to_hex(&o->oid));\n \t\t}\n+\t\tfor (i = 0; i < shallows.nr; i++) {\n+\t\t\tstruct object *o = shallows.objects[i].item;\n+\t\t\targv_array_pushf(&av, \"^%s\", oid_to_hex(&o->oid));\n+\t\t}\n+\n+\t\tinit_revisions(&revs, NULL);\n+\t\tsetup_revisions(av.argc, av.argv, &revs, NULL);\n+\t\tif (prepare_revision_walk(&revs))\n+\t\t\tdie(\"revision walk setup failed\");\n+\n+\t\twhile ((c = get_revision(&revs))) {\n+\t\t\tif (!(c->object.flags & BOUNDARY))\n+\t\t\t\tcontinue;\n+\t\t\tregister_shallow(c->object.oid.hash);\n+\t\t\tpacket_write(1, \"shallow %s\",\n+\t\t\t\t oid_to_hex(&c->object.oid));\n+\t\t}\n+\t\tpacket_flush(1);\n+\t\targv_array_clear(&av);\n+\n+\t\tfor (i = 0; i < shallows.nr; i++)\n+\t\t\tregister_shallow(shallows.objects[i].item->oid.hash);\n+\t}\n \n \tshallow_nr += shallows.nr;\n \tfree(shallows.objects);\n\nThough I think perhaps we should also be adding those BOUNDARY commits\nto the \"shallows\" object array? This works because the \"--shallow\" we\npass to pack-objects comes by reading the commit-graft list manipulated\nby register_shallow(), so I'm not sure if it matters.\n\n_But_, the client is not prepared to handle this. We send \"shallow\"\nlines that it is not expecting, since it did not ask for any depth. So I\nthink this logic would have to kick in only when the client tells us to\ndo so.\n\nI hacked around it with:\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex e8ae6d1..988c808 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -830,6 +830,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \n \tsort_ref_list(&ref, ref_compare_name);\n \tqsort(sought, nr_sought, sizeof(*sought), cmp_ref_by_name);\n+\targs->deepen = 1;\n \n \tif ((args->depth > 0 || is_repository_shallow()) && !server_supports(\"shallow\"))\n \t\tdie(\"Server does not support shallow clients\");\n\nand confirmed that the resulting \"git fetch origin new\" from my earlier\nexample does a sane thing.\n\nSo what next? I think there's some protocol work here, and I think the\noverall design of that needs to be considered alongside the other\n\"deepen\" options your topic in pu adds (and of which I'm largely\nignorant). Does this sufficiently interest you to pick up and roll into\nyour other shallow work?\n\n-Peff\n"},{"id":"280368","messageId":"20160308133041.GA9465@sigill.intra.peff.net","threadId":"41639","inReplyTo":"20160308132524.GA22866@sigill.intra.peff.net","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-08T13:30:41Z","receivedAt":"2016-03-08T13:30:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 08, 2016 at 08:25:24AM -0500, Jeff King wrote:\n\n> > Good news. We have the mechanism in place, I think.\n> > get_shallow_commits_by_rev_list() (from 'pu') will produce the right\n> > shallow points for sending back to the client if you pass \"--not\n> > \" to it. It's meant to be used for\n> > --shallow-exclude and --shallow-since, but if neither is given (nor\n> > --depth) I guess we can run it with current shallow points. I wonder\n> > if we can detect some common cases and avoid commit traversing this\n> > way though.\n> \n> I tried that, but I couldn't quite get it to work. I don't think we need\n> any special rev-list, though; we can just find the boundary points of\n> that traversal and mark them as new shallows.\n\nBy the way, I found a bug during my initial attempts with\nget_shallow_commits_by_rev_list(). One of the loops uses \"p\" to traverse\na linked list, and then re-uses \"p\" again for another list traversal\ninside the body of the loop. When the inner loop finishes, \"p\" is\nleft as NULL, and then the outer loop tries to access \"p->next\", which\nsegfaults.\n\nI _think_ this is just a mistaken re-use of the variable, and can be\nfixed with a new iteration variable for the inner loop, like:\n\ndiff --git a/shallow.c b/shallow.c\nindex 6ceb3f8..d600947 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -188,13 +188,14 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,\n \t */\n \tfor (p = not_shallow_list; p; p = p->next) {\n \t\tstruct commit *c = p->item;\n+\t\tstruct commit_list *parent;\n \n \t\tif (parse_commit(c))\n \t\t\tdie(\"unable to parse commit %s\",\n \t\t\t oid_to_hex(&c->object.oid));\n \n-\t\tfor (p = c->parents; p; p = p->next)\n-\t\t\tif (!(p->item->object.flags & not_shallow_flag)) {\n+\t\tfor (parent = c->parents; parent; parent = parent->next)\n+\t\t\tif (!(parent->item->object.flags & not_shallow_flag)) {\n \t\t\t\tc->object.flags |= shallow_flag;\n \t\t\t\tcommit_list_insert(c, &result);\n \t\t\t\tbreak;\n\nAs I said, I didn't end up using this function either way, but you\nprobably want the fix above for the rest of your series. :)\n\n-Peff\n"},{"id":"280391","messageId":"CACsJy8BFK64rzJwQxB8M3=uA_g9s6N70vjNyLOnroZc28soiAQ@mail.gmail.com","threadId":"41639","inReplyTo":"20160308133041.GA9465@sigill.intra.peff.net","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-08T23:02:10Z","receivedAt":"2016-03-08T23:02:10Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 8, 2016 at 8:30 PM, Jeff King wrote:\n> On Tue, Mar 08, 2016 at 08:25:24AM -0500, Jeff King wrote:\n>\n>> > Good news. We have the mechanism in place, I think.\n>> > get_shallow_commits_by_rev_list() (from 'pu') will produce the right\n>> > shallow points for sending back to the client if you pass \"--not\n>> > \" to it. It's meant to be used for\n>> > --shallow-exclude and --shallow-since, but if neither is given (nor\n>> > --depth) I guess we can run it with current shallow points. I wonder\n>> > if we can detect some common cases and avoid commit traversing this\n>> > way though.\n>>\n>> I tried that, but I couldn't quite get it to work. I don't think we need\n>> any special rev-list, though; we can just find the boundary points of\n>> that traversal and mark them as new shallows.\n>\n> By the way, I found a bug during my initial attempts with\n> get_shallow_commits_by_rev_list().\n\nHehe. Thanks. Will check why tests missed it and reroll with Ramsay's\nfix once -rc period is over.\n-- \nDuy\n"},{"id":"280552","messageId":"20160310122020.GA24007@lanh","threadId":"41639","inReplyTo":"20160308132524.GA22866@sigill.intra.peff.net","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-10T12:20:20Z","receivedAt":"2016-03-10T12:20:20Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 08, 2016 at 08:25:24AM -0500, Jeff King wrote:\n> I think this patch does roughly the right thing:\n> \n> diff --git a/upload-pack.c b/upload-pack.c\n> index 4859535..da76f70 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -833,12 +833,41 @@ static void receive_needs(void)\n> \t\tdeepen_by_rev_list(av.argc, av.argv, &shallows);\n> \t\targv_array_clear(&av);\n> \t}\n> -\telse\n> -\t\tif (shallows.nr > 0) {\n> -\t\t\tint i;\n> -\t\t\tfor (i = 0; i < shallows.nr; i++)\n> -\t\t\t\tregister_shallow(shallows.objects[i].item->oid.hash);\n> +\telse if (shallows.nr > 0) {\n> +\t\tstruct rev_info revs;\n> +\t\tstruct argv_array av = ARGV_ARRAY_INIT;\n> +\t\tstruct commit *c;\n> +\t\tint i;\n> +\n> +\t\targv_array_push(&av, \"rev-list\");\n> +\t\targv_array_push(&av, \"--boundary\");\n\nNice. I didn't know about --boundary. But will it work correctly in\nthis case?\n\n --- B ---- C ---- F\n / /\n --- D ---- E ---- G\n\nC and D will be current shallow cut points. People \"want\" F and G.\n\"rev-list --boundary F G ^C ^D\" would mark E as boundary/shallow too,\ncorrect? If so the history from G will be one depth short on a normal\nfetch.\n\n> +\t\tfor (i = 0; i < want_obj.nr; i++) {\n> +\t\t\tstruct object *o = want_obj.objects[i].item;\n> +\t\t\targv_array_push(&av, oid_to_hex(&o->oid));\n> \t\t}\n> +\t\tfor (i = 0; i < shallows.nr; i++) {\n> +\t\t\tstruct object *o = shallows.objects[i].item;\n> +\t\t\targv_array_pushf(&av, \"^%s\", oid_to_hex(&o->oid));\n> +\t\t}\n> +\n> +\t\tinit_revisions(&revs, NULL);\n> +\t\tsetup_revisions(av.argc, av.argv, &revs, NULL);\n> +\t\tif (prepare_revision_walk(&revs))\n> +\t\t\tdie(\"revision walk setup failed\");\n> +\n> +\t\twhile ((c = get_revision(&revs))) {\n> +\t\t\tif (!(c->object.flags & BOUNDARY))\n> +\t\t\t\tcontinue;\n> +\t\t\tregister_shallow(c->object.oid.hash);\n> +\t\t\tpacket_write(1, \"shallow %s\",\n> +\t\t\t\t oid_to_hex(&c->object.oid));\n> +\t\t}\n\n> ...\n> _But_, the client is not prepared to handle this. We send \"shallow\"\n> lines that it is not expecting, since it did not ask for any depth. So I\n> think this logic would have to kick in only when the client tells us to\n> do so.\n\nUrgh.. not good. Perhaps a new extension to let the server know the\nclient can handle spontaneous \"deepen\" commands and only activate new\nmode when the extension is present?\n\n> So what next? I think there's some protocol work here, and I think the\n> overall design of that needs to be considered alongside the other\n> \"deepen\" options your topic in pu adds (and of which I'm largely\n> ignorant). Does this sufficiently interest you to pick up and roll into\n> your other shallow work?\n\nI can pick it up if you are busy with other stuff. But I'm also having\na couple other topics at the moment, so it may not progress very fast.\n--\nDuy\n"},{"id":"280573","messageId":"20160310211052.GC30595@sigill.intra.peff.net","threadId":"41639","inReplyTo":"20160310122020.GA24007@lanh","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-10T21:10:53Z","receivedAt":"2016-03-10T21:10:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 10, 2016 at 07:20:20PM +0700, Duy Nguyen wrote:\n\n> > +\telse if (shallows.nr > 0) {\n> > +\t\tstruct rev_info revs;\n> > +\t\tstruct argv_array av = ARGV_ARRAY_INIT;\n> > +\t\tstruct commit *c;\n> > +\t\tint i;\n> > +\n> > +\t\targv_array_push(&av, \"rev-list\");\n> > +\t\targv_array_push(&av, \"--boundary\");\n> \n> Nice. I didn't know about --boundary. But will it work correctly in\n> this case?\n> \n> --- B ---- C ---- F\n> / /\n> --- D ---- E ---- G\n> \n> C and D will be current shallow cut points. People \"want\" F and G.\n> \"rev-list --boundary F G ^C ^D\" would mark E as boundary/shallow too,\n> correct? If so the history from G will be one depth short on a normal\n> fetch.\n\nIMHO, that is the right thing. They asked for \"C\" as a shallow cut-off\npoint, so anything that is a parent of \"C\" should be omitted as shallow,\ntoo. It has nothing to do with the numeric depth, which was just the\nstarting point for generating the shallow cutoffs.\n\nThat's just my mental model, though. I admit I don't actually use\nshallow clones myself, and maybe people would expect something else.\n\n> > _But_, the client is not prepared to handle this. We send \"shallow\"\n> > lines that it is not expecting, since it did not ask for any depth. So I\n> > think this logic would have to kick in only when the client tells us to\n> > do so.\n> \n> Urgh.. not good. Perhaps a new extension to let the server know the\n> client can handle spontaneous \"deepen\" commands and only activate new\n> mode when the extension is present?\n\nYeah, we definitely need an extension. I'm not sure if the extension\nshould be \"I know about spontaneous shallow/deepen responses; it's OK to\nsend them to me\" or \"I want you to include the shallow points I send as\nboundary cutoffs for further shallow-ing of newly fetched history\".\n\nThey amount to the same thing when implementing _this_ feature, but the\nlatter leaves us room in the future for a client to say \"sure, I\nunderstand your spontaneous responses, but I explicitly _don't_ want you\nto do the boundary computation\". I don't know if that is useful or not,\nbut it might not hurt to have later on (and by adding it now, it \"just\nworks\" later on with older servers/clients).\n\n> > So what next? I think there's some protocol work here, and I think the\n> > overall design of that needs to be considered alongside the other\n> > \"deepen\" options your topic in pu adds (and of which I'm largely\n> > ignorant). Does this sufficiently interest you to pick up and roll into\n> > your other shallow work?\n> \n> I can pick it up if you are busy with other stuff. But I'm also having\n> a couple other topics at the moment, so it may not progress very fast.\n\nThanks. I don't think it is too urgent; it has been that way for a\nwhile. I certainly have plenty of other things to work on, but mostly I\njust feel a bit out of my depth on the shallow stuff. I haven't given it\nany real thought, and you obviously have.\n\n-Peff\n"},{"id":"280576","messageId":"xmqqbn6mdnyn.fsf@gitster.mtv.corp.google.com","threadId":"41639","inReplyTo":"20160310211052.GC30595@sigill.intra.peff.net","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-10T21:26:08Z","receivedAt":"2016-03-10T21:26:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 10, 2016 at 07:20:20PM +0700, Duy Nguyen wrote:\n>\n>> > +\telse if (shallows.nr > 0) {\n>> > +\t\tstruct rev_info revs;\n>> > +\t\tstruct argv_array av = ARGV_ARRAY_INIT;\n>> > +\t\tstruct commit *c;\n>> > +\t\tint i;\n>> > +\n>> > +\t\targv_array_push(&av, \"rev-list\");\n>> > +\t\targv_array_push(&av, \"--boundary\");\n>> \n>> Nice. I didn't know about --boundary. But will it work correctly in\n>> this case?\n>> \n>> --- B ---- C ---- F\n>> / /\n>> --- D ---- E ---- G\n>> \n>> C and D will be current shallow cut points. People \"want\" F and G.\n>> \"rev-list --boundary F G ^C ^D\" would mark E as boundary/shallow too,\n>> correct? If so the history from G will be one depth short on a normal\n>> fetch.\n>\n> IMHO, that is the right thing. They asked for \"C\" as a shallow cut-off\n> point, so anything that is a parent of \"C\" should be omitted as shallow,\n> too. It has nothing to do with the numeric depth, which was just the\n> starting point for generating the shallow cutoffs.\n\nI think that is the right mental model. The statement that \"C and D\nare current cut points\" does not make much sense. As you cannot\nrewrite parents of commits after the fact, you cannot construct a\ncase like \"when the shallow clone originally was made, two histories\nwere forked long time before B and D, and the cloner ended up with C\nand D as the cutoff point, but now that we have the ancestry linkage\nbetween B and D (and C and E), we need to make E a new cutoff\". The\noriginal \"shallow\" implementation does not store \"starting point +\nnumber of depth\" and instead translates that to the cut-off point\nfor this exact reason.\n\n> Yeah, we definitely need an extension. I'm not sure if the extension\n> should be \"I know about spontaneous shallow/deepen responses; it's OK to\n> send them to me\" or \"I want you to include the shallow points I send as\n> boundary cutoffs for further shallow-ing of newly fetched history\".\n>\n> They amount to the same thing when implementing _this_ feature, but the\n> latter leaves us room in the future for a client to say \"sure, I\n> understand your spontaneous responses, but I explicitly _don't_ want you\n> to do the boundary computation\". I don't know if that is useful or not,\n> but it might not hurt to have later on (and by adding it now, it \"just\n> works\" later on with older servers/clients).\n\nI am not sure what distinction you are worried about. An updated\nclient that is capable of saying \"you may give shallow/deepen\nresponses to me\" can optionally be told not to say it to the server,\nand that is equivalent to saying \"I don't want you to send them\", no?\n"},{"id":"280579","messageId":"20160310214042.GB32608@sigill.intra.peff.net","threadId":"41639","inReplyTo":"xmqqbn6mdnyn.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-10T21:40:42Z","receivedAt":"2016-03-10T21:40:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 10, 2016 at 01:26:08PM -0800, Junio C Hamano wrote:\n\n> > IMHO, that is the right thing. They asked for \"C\" as a shallow cut-off\n> > point, so anything that is a parent of \"C\" should be omitted as shallow,\n> > too. It has nothing to do with the numeric depth, which was just the\n> > starting point for generating the shallow cutoffs.\n> \n> I think that is the right mental model. The statement that \"C and D\n> are current cut points\" does not make much sense. As you cannot\n> rewrite parents of commits after the fact, you cannot construct a\n> case like \"when the shallow clone originally was made, two histories\n> were forked long time before B and D, and the cloner ended up with C\n> and D as the cutoff point, but now that we have the ancestry linkage\n> between B and D (and C and E), we need to make E a new cutoff\". The\n> original \"shallow\" implementation does not store \"starting point +\n> number of depth\" and instead translates that to the cut-off point\n> for this exact reason.\n\nOK, good. Now there are at least two of us who view it that way. :)\n\n> > Yeah, we definitely need an extension. I'm not sure if the extension\n> > should be \"I know about spontaneous shallow/deepen responses; it's OK to\n> > send them to me\" or \"I want you to include the shallow points I send as\n> > boundary cutoffs for further shallow-ing of newly fetched history\".\n> >\n> > They amount to the same thing when implementing _this_ feature, but the\n> > latter leaves us room in the future for a client to say \"sure, I\n> > understand your spontaneous responses, but I explicitly _don't_ want you\n> > to do the boundary computation\". I don't know if that is useful or not,\n> > but it might not hurt to have later on (and by adding it now, it \"just\n> > works\" later on with older servers/clients).\n> \n> I am not sure what distinction you are worried about. An updated\n> client that is capable of saying \"you may give shallow/deepen\n> responses to me\" can optionally be told not to say it to the server,\n> and that is equivalent to saying \"I don't want you to send them\", no?\n\nMostly, I wondered if we would need to send spontaneous shallow lines\nfor any other cases, and the client would not be able to say \"I\nunderstand them and want them in case A, but not in case B\".\n\nI do not have any case A in mind; it was just a general sense of \"let's\nmake feature flags as specific as possible to avoid painting ourselves\ninto a corner\". I'm OK with implementing it either way.\n\n-Peff\n"},{"id":"280599","messageId":"CACsJy8BRhuSOs96fonjBJ0ok6JZJ3CwLkDPCP_9QQdROZUVh8w@mail.gmail.com","threadId":"41639","inReplyTo":"20160310214042.GB32608@sigill.intra.peff.net","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-11T00:47:34Z","receivedAt":"2016-03-11T00:47:34Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 11, 2016 at 4:40 AM, Jeff King wrote:\n> On Thu, Mar 10, 2016 at 01:26:08PM -0800, Junio C Hamano wrote:\n>\n>> > IMHO, that is the right thing. They asked for \"C\" as a shallow cut-off\n>> > point, so anything that is a parent of \"C\" should be omitted as shallow,\n>> > too. It has nothing to do with the numeric depth, which was just the\n>> > starting point for generating the shallow cutoffs.\n>>\n>> I think that is the right mental model. The statement that \"C and D\n>> are current cut points\" does not make much sense. As you cannot\n>> rewrite parents of commits after the fact, you cannot construct a\n>> case like \"when the shallow clone originally was made, two histories\n>> were forked long time before B and D, and the cloner ended up with C\n>> and D as the cutoff point, but now that we have the ancestry linkage\n>> between B and D (and C and E), we need to make E a new cutoff\". The\n>> original \"shallow\" implementation does not store \"starting point +\n>> number of depth\" and instead translates that to the cut-off point\n>> for this exact reason.\n\nWell, assume again that F and G are ref heads, and their respective\ndistance to C and D are the same (like the below graph), then \"fetch\n--deptch=\" can mark C and D as shallow cut points because\n--depth traverses from refs until the distance is met, it does not do\ntotal exclusion ^C like rev-list.\n\n --- B ---- C ---- H ---- F\n / /\n --- D ---- E ---- G\n\n\n> OK, good. Now there are at least two of us who view it that way. :)\n\nBut going that direction just gives me more headache. If you two are\nok with this and nobody else complains, I'm ok too :-D I guess it's a\ncorner case anyway that's probably hard to occur in practice.\n\n>> > So what next? I think there's some protocol work here, and I think the\n>> > overall design of that needs to be considered alongside the other\n>> > \"deepen\" options your topic in pu adds (and of which I'm largely\n>> > ignorant). Does this sufficiently interest you to pick up and roll into\n>> > your other shallow work?\n>>\n>> I can pick it up if you are busy with other stuff. But I'm also having\n>> a couple other topics at the moment, so it may not progress very fast.\n>\n> Thanks. I don't think it is too urgent; it has been that way for a\n> while. I certainly have plenty of other things to work on, but mostly I\n> just feel a bit out of my depth on the shallow stuff. I haven't given it\n> any real thought, and you obviously have.\n\nMore people understanding this code is always better though. But don't\nworry I'll take care of this.\n\n>> > Yeah, we definitely need an extension. I'm not sure if the extension\n>> > should be \"I know about spontaneous shallow/deepen responses; it's OK to\n>> > send them to me\" or \"I want you to include the shallow points I send as\n>> > boundary cutoffs for further shallow-ing of newly fetched history\".\n>> >\n>> > They amount to the same thing when implementing _this_ feature, but the\n>> > latter leaves us room in the future for a client to say \"sure, I\n>> > understand your spontaneous responses, but I explicitly _don't_ want you\n>> > to do the boundary computation\". I don't know if that is useful or not,\n>> > but it might not hurt to have later on (and by adding it now, it \"just\n>> > works\" later on with older servers/clients).\n>>\n>> I am not sure what distinction you are worried about. An updated\n>> client that is capable of saying \"you may give shallow/deepen\n>> responses to me\" can optionally be told not to say it to the server,\n>> and that is equivalent to saying \"I don't want you to send them\", no?\n>\n> Mostly, I wondered if we would need to send spontaneous shallow lines\n> for any other cases, and the client would not be able to say \"I\n> understand them and want them in case A, but not in case B\".\n>\n> I do not have any case A in mind; it was just a general sense of \"let's\n> make feature flags as specific as possible to avoid painting ourselves\n> into a corner\". I'm OK with implementing it either way.\n\nAnother note to myself is, we do not want these spontaneous cutoff\nrequests to cut client's history even shorter (possibly by accident\nbecause of buggy servers). As long as they are about sealing commits\nthat lead to non-existing objects, i think it's ok to make it a\ngeneral feature.\n-- \nDuy\n"},{"id":"280623","messageId":"xmqqfuvxarcc.fsf@gitster.mtv.corp.google.com","threadId":"41639","inReplyTo":"CACsJy8BRhuSOs96fonjBJ0ok6JZJ3CwLkDPCP_9QQdROZUVh8w@mail.gmail.com","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-11T16:53:39Z","receivedAt":"2016-03-11T16:53:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Well, assume again that F and G are ref heads, and their respective\n> distance to C and D are the same (like the below graph), then \"fetch\n> --deptch=\" can mark C and D as shallow cut points because\n> --depth traverses from refs until the distance is met, it does not do\n> total exclusion ^C like rev-list.\n>\n> --- B ---- C ---- H ---- F\n> / /\n> --- D ---- E ---- G\n\nHmph, so we do not realize that D is reachable from C because we do\nnot even look at the parents of the latter. Is there a remification\nthat breaks the proposed mental view coming from this?\n\nLet me think aloud. Later when we fetch again (without asking to\nchange the depth but without refusing an unsolicited depth change),\na fetch of history that lead to descendants of F or G will still not\nrealize that C and D are related, because the traversal down to C\nwould stop there without noticing that E is reachable from C. That\nwon't break. If the tips of the history have been rewound and now\npoints at the tips of a history that is forked before B and D (let's\nsay the new tip to replace G is Z), would that be a problem? The\nside that does have the full history would notice that the fork\npoint X is an ancestors of cutoff C and D, and can tell that the\nside-branch proper, Y and Z, are interesting but X is where their\ntruncated history must stop.\n\n --- B ---- C ---- H ---- F\n / /\n -- X --- D ---- E ---- G\n \\\n Y --- Z\n\nWhile doing so the side that originally had C and D as the cutoff\nwould be told that now X is also a cutoff. In general, because the\nside that has the full history and gets fetched would not have to\nknow about _all_ the refs and objects the fetching side has, so if\nit didn't learn about the shallow side still having G, it wouldn't\nbe able to say that D is no longer an interesting cutoff, but would\nthat be a problem? The side with full history should also be able\nto notice that E can also be added as a cutoff. Would that be a\ngood thing to do? If so, perhaps we should have done that when the\noriginal history was shallowing cloned with depth=2 in your picture,\nperhaps? After all, even though the shallow side may not know C and\nD are related, the side that supplied the truncated history knows,\nso after depth traversal finds C and D, it perhaps can postprocess\nit to realize C and E is the set that should be sent as the cutoff?\n"},{"id":"280627","messageId":"20160311181635.GA31299@sigill.intra.peff.net","threadId":"41639","inReplyTo":"CACsJy8BRhuSOs96fonjBJ0ok6JZJ3CwLkDPCP_9QQdROZUVh8w@mail.gmail.com","subject":"Re: [BUG?] fetch into shallow sends a large number of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-11T18:16:35Z","receivedAt":"2016-03-11T18:16:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 11, 2016 at 07:47:34AM +0700, Duy Nguyen wrote:\n\n> Well, assume again that F and G are ref heads, and their respective\n> distance to C and D are the same (like the below graph), then \"fetch\n> --deptch=\" can mark C and D as shallow cut points because\n> --depth traverses from refs until the distance is met, it does not do\n> total exclusion ^C like rev-list.\n> \n> --- B ---- C ---- H ---- F\n> / /\n> --- D ---- E ---- G\n\nRight, so I think we would only apply the \"use existing cutoffs as\nbounds\" thing when we were not otherwise given a --depth. Because it can\ndefinitely cause us to override the depth (and there's no need to; the\npoint is to avoid linking in all of history, and --depth already solves\nthat). So we probably do want the client to ask \"I'm not giving you a\ndepth, but please use my existing shallows as cutoffs\".\n\nI think a more interesting case here is when we have C as a cutoff, and\nsomebody fetches \"E\" directly. They are part of the truncated history.\nSo we should exclude them from a fetch of \"G\", but if the user asked for\nthem directly, that probably needs to override the existing shallow\ncutoff.\n\nWe probably want to compute the --boundary of \"E ^C\", but then omit from\nthat any items that are directly in want_obj. IOW, it is OK to truncate\nat depth=1 due to an existing cutoff, but not at depth=0. :)\n\nThat does mean we would then fetch all of the history leading up to E,\nbut I think that's OK. If the user didn't specify --depth and they are\nfetching from behind the shallow-cutoff point, we don't have any real\nway of knowing how much they wanted (though I guess it would also be\nsensible to just apply depth=1 in such a case).\n\n-Peff\n"}]}