{"thread":{"id":"40910","subject":"Multiple fetches when unshallowing a shallow clone","startedAt":"2015-11-30T19:35:04Z","lastAt":"2015-12-07T21:42:58Z","messageCount":19,"participants":["Jason Paller-Rzepka","Stefan Beller","Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"273840","messageId":"CACs8u9STLLHr3c3O9kQKGEN52DLfJ2LatjWkeaeeLA-xP=gC5Q@mail.gmail.com","threadId":"40910","inReplyTo":null,"subject":"Multiple fetches when unshallowing a shallow clone","fromName":"Jason Paller-Rzepka","fromEmail":"jasonpr@google.com","sentAt":"2015-11-30T19:35:04Z","receivedAt":"2015-11-30T19:35:04Z","isPatch":false,"sender":{"key":"jasonpr@google.com","avatar":null},"body":"Hi all,\n\nWould anyone be willing to help me understand some shallow-clone\nbehavior? (I found a bug in Dulwich, and I'm looking for some context\nso I can determine how to fix it.)\n\nI learned that cgit sometimes performs two fetches for a `git fetch\n--unshallow`: one with depth 'infinity', and a subsequent one with\ndepth zero.\n\nCould anyone answer:\n1) What is the purpose of the second fetch?\n2) What does this depth of zero mean? Is it the same as a depth of\ninfinity? (I assume not... but, since I thought the smallest\nmeaningful depth was 1, I don't know what else it might mean.)\n\nThank you!\nJason\n"},{"id":"274021","messageId":"CAGZ79ka=RxVZ49D0wkqTRqspKb=Ce5Ay01muBt_Gk6_rDbH6KA@mail.gmail.com","threadId":"40910","inReplyTo":"CACs8u9STLLHr3c3O9kQKGEN52DLfJ2LatjWkeaeeLA-xP=gC5Q@mail.gmail.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-04T20:46:59Z","receivedAt":"2015-12-04T20:46:59Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 30, 2015 at 11:35 AM, Jason Paller-Rzepka\n wrote:\n> Hi all,\n>\n> Would anyone be willing to help me understand some shallow-clone\n> behavior? (I found a bug in Dulwich, and I'm looking for some context\n> so I can determine how to fix it.)\n>\n> I learned that cgit sometimes performs two fetches for a `git fetch\n> --unshallow`: one with depth 'infinity', and a subsequent one with\n> depth zero.\n\nIs there a condition to trigger this 'sometimes' ?\n\nI just tried reproducing via\n$ GIT_TRACE=1 git fetch --unshallow\n\nand could not see a second fetch, but only a\nfetch-pack with --depth=2147483647\n>\n> Could anyone answer:\n> 1) What is the purpose of the second fetch?\n> 2) What does this depth of zero mean? Is it the same as a depth of\n> infinity? (I assume not... but, since I thought the smallest\n> meaningful depth was 1, I don't know what else it might mean.)\n>\n> Thank you!\n> Jason\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"274025","messageId":"20151204212712.GA22493@sigill.intra.peff.net","threadId":"40910","inReplyTo":"CAGZ79ka=RxVZ49D0wkqTRqspKb=Ce5Ay01muBt_Gk6_rDbH6KA@mail.gmail.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-04T21:27:12Z","receivedAt":"2015-12-04T21:27:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 04, 2015 at 12:46:59PM -0800, Stefan Beller wrote:\n\n> On Mon, Nov 30, 2015 at 11:35 AM, Jason Paller-Rzepka\n> wrote:\n> > Hi all,\n> >\n> > Would anyone be willing to help me understand some shallow-clone\n> > behavior? (I found a bug in Dulwich, and I'm looking for some context\n> > so I can determine how to fix it.)\n> >\n> > I learned that cgit sometimes performs two fetches for a `git fetch\n> > --unshallow`: one with depth 'infinity', and a subsequent one with\n> > depth zero.\n> \n> Is there a condition to trigger this 'sometimes' ?\n> \n> I just tried reproducing via\n> $ GIT_TRACE=1 git fetch --unshallow\n> \n> and could not see a second fetch, but only a\n> fetch-pack with --depth=2147483647\n\nThis seems to reproduce consistently for me:\n\n $ git clone --depth=1 git://github.com/git/git\n Cloning into 'git'...\n remote: Counting objects: 2925, done.\n remote: Compressing objects: 100% (2602/2602), done.\n remote: Total 2925 (delta 230), reused 2329 (delta 206), pack-reused 0\n Receiving objects: 100% (2925/2925), 6.17 MiB | 10.80 MiB/s, done.\n Resolving deltas: 100% (230/230), done.\n\n $ cd git\n $ git fetch --unshallow\n remote: Counting objects: 185430, done.\n remote: Compressing objects: 100% (46933/46933), done.\n remote: Total 185430 (delta 140505), reused 181589 (delta 136694), pack-reused 0\n Receiving objects: 100% (185430/185430), 52.80 MiB | 10.84 MiB/s, done.\n Resolving deltas: 100% (140505/140505), completed with 1784 local objects.\n remote: Counting objects: 579, done.\n remote: Compressing objects: 100% (579/579), done.\n remote: Total 579 (delta 0), reused 579 (delta 0), pack-reused 0\n Receiving objects: 100% (579/579), 266.85 KiB | 0 bytes/s, done.\n [... fetch output ...]\n\nThat looks like two packs being received for the --unshallow case.\n\n-Peff\n"},{"id":"274026","messageId":"CAGZ79kZwZuKEuYqRBFBRCu_bY6PmTnC+3e=Znpf24rxZB6hEbA@mail.gmail.com","threadId":"40910","inReplyTo":"20151204212712.GA22493@sigill.intra.peff.net","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-04T21:36:47Z","receivedAt":"2015-12-04T21:36:47Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 4, 2015 at 1:27 PM, Jeff King wrote:\n>>\n>> and could not see a second fetch, but only a\n>> fetch-pack with --depth=2147483647\n>\n> This seems to reproduce consistently for me:\n>\n> $ git clone --depth=1 git://github.com/git/git\n\nI used the http protocol, so I guess that's the difference.\n"},{"id":"274027","messageId":"CACs8u9Qvqn4KDMKo+RHsQaf+dw+CGtWrOpoUJzaZAqD1rFRiuw@mail.gmail.com","threadId":"40910","inReplyTo":"20151204212712.GA22493@sigill.intra.peff.net","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Jason Paller-Rzepka","fromEmail":"jasonpr@google.com","sentAt":"2015-12-04T21:38:16Z","receivedAt":"2015-12-04T21:38:16Z","isPatch":false,"sender":{"key":"jasonpr@google.com","avatar":null},"body":"It appears that it happens when the shallow history grows to include a\ncommit that's pointed to by a previously unseen tag. For example,\nwhen I deepen a checkout of git to depth 8, I hit v2.5.2, and a second\nfetch takes place.\n\n```\n$ git clone --depth=1 http://github.com/git/git\nCloning into 'git'...\nremote: Counting objects: 2925, done.\nremote: Compressing objects: 100% (2602/2602), done.\nremote: Total 2925 (delta 230), reused 2329 (delta 206), pack-reused 0\nReceiving objects: 100% (2925/2925), 6.17 MiB | 0 bytes/s, done.\nResolving deltas: 100% (230/230), done.\nChecking connectivity... done.\n$ git -C git fetch --depth=8\nremote: Counting objects: 858, done.\nremote: Compressing objects: 100% (774/774), done.\nremote: Total 858 (delta 793), reused 138 (delta 80), pack-reused 0\nReceiving objects: 100% (858/858), 364.53 KiB | 0 bytes/s, done.\nResolving deltas: 100% (793/793), completed with 476 local objects.\nremote: Counting objects: 1, done.\nremote: Total 1 (delta 0), reused 1 (delta 0), pack-reused 0\nUnpacking objects: 100% (1/1), done.\nFrom http://github.com/git/git\n * [new tag] v2.5.2 -> v2.5.2\n$\n```\n\nBut why would fetching a tag (or set of tags) merit a depth of zero?\nDoesn't depth 1 mean \"give me the the objects, and none of their\ndescendants\"? Why use 0?\n\nThanks!\nJason\n\nOn Fri, Dec 4, 2015 at 4:27 PM, Jeff King wrote:\n> On Fri, Dec 04, 2015 at 12:46:59PM -0800, Stefan Beller wrote:\n>\n>> On Mon, Nov 30, 2015 at 11:35 AM, Jason Paller-Rzepka\n>> wrote:\n>> > Hi all,\n>> >\n>> > Would anyone be willing to help me understand some shallow-clone\n>> > behavior? (I found a bug in Dulwich, and I'm looking for some context\n>> > so I can determine how to fix it.)\n>> >\n>> > I learned that cgit sometimes performs two fetches for a `git fetch\n>> > --unshallow`: one with depth 'infinity', and a subsequent one with\n>> > depth zero.\n>>\n>> Is there a condition to trigger this 'sometimes' ?\n>>\n>> I just tried reproducing via\n>> $ GIT_TRACE=1 git fetch --unshallow\n>>\n>> and could not see a second fetch, but only a\n>> fetch-pack with --depth=2147483647\n>\n> This seems to reproduce consistently for me:\n>\n> $ git clone --depth=1 git://github.com/git/git\n> Cloning into 'git'...\n> remote: Counting objects: 2925, done.\n> remote: Compressing objects: 100% (2602/2602), done.\n> remote: Total 2925 (delta 230), reused 2329 (delta 206), pack-reused 0\n> Receiving objects: 100% (2925/2925), 6.17 MiB | 10.80 MiB/s, done.\n> Resolving deltas: 100% (230/230), done.\n>\n> $ cd git\n> $ git fetch --unshallow\n> remote: Counting objects: 185430, done.\n> remote: Compressing objects: 100% (46933/46933), done.\n> remote: Total 185430 (delta 140505), reused 181589 (delta 136694), pack-reused 0\n> Receiving objects: 100% (185430/185430), 52.80 MiB | 10.84 MiB/s, done.\n> Resolving deltas: 100% (140505/140505), completed with 1784 local objects.\n> remote: Counting objects: 579, done.\n> remote: Compressing objects: 100% (579/579), done.\n> remote: Total 579 (delta 0), reused 579 (delta 0), pack-reused 0\n> Receiving objects: 100% (579/579), 266.85 KiB | 0 bytes/s, done.\n> [... fetch output ...]\n>\n> That looks like two packs being received for the --unshallow case.\n>\n> -Peff\n"},{"id":"274030","messageId":"CAGZ79kZMNuiiH=8zu4pJXazW8MNYCu_59MBh7iX0f0ws3dfsYA@mail.gmail.com","threadId":"40910","inReplyTo":"CACs8u9Qvqn4KDMKo+RHsQaf+dw+CGtWrOpoUJzaZAqD1rFRiuw@mail.gmail.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-04T21:50:25Z","receivedAt":"2015-12-04T21:50:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I can reproduce it now. Instead of using my $random version, I just\nneeded origin/master\nto reproduce.\n\nThe second fetch is invoked via\n(as outputted via GIT_TRACE=1 git -C git fetch --depth=8)\n\n13:44:56.863841 run-command.c:343 trace: run_command:\n'fetch-pack' '--stateless-rpc' '--stdin' '--lock-pack' '--thin'\n'https://github.com/git/git/'\n\nso it seems like there is no explicit depth given, so I think the 0\ncomes from the initialization step and nobody touched it to fill with\nmeaningful values.\n"},{"id":"274031","messageId":"20151204215158.GA27987@sigill.intra.peff.net","threadId":"40910","inReplyTo":"CACs8u9Qvqn4KDMKo+RHsQaf+dw+CGtWrOpoUJzaZAqD1rFRiuw@mail.gmail.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-04T21:51:58Z","receivedAt":"2015-12-04T21:51:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 04, 2015 at 04:38:16PM -0500, Jason Paller-Rzepka wrote:\n\n> It appears that it happens when the shallow history grows to include a\n> commit that's pointed to by a previously unseen tag. For example,\n> when I deepen a checkout of git to depth 8, I hit v2.5.2, and a second\n> fetch takes place.\n\nYeah. The code is in builtin/fetch.c:backfill_tags.\n\n> But why would fetching a tag (or set of tags) merit a depth of zero?\n> Doesn't depth 1 mean \"give me the the objects, and none of their\n> descendants\"? Why use 0?\n\nThat comes from this line:\n\n transport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n\nThat line blame back to b888d61 (Make fetch a builtin, 2007-09-10),\nwhich isn't incredibly helpful.\n\nI think that comes from the original git-fetch.sh, which had:\n\n ?*)\n # do not deepen a shallow tree when following tags\n shallow_depth=\n\nWhich makes sense. I think the code at that point is not aware that we\njust \"unshallowed\" and can therefore drop the depth parameter\naltogether. But I admit I am not all that familiar with the shallow\ncode.\n\n+cc Duy, who can probably say something way more intelligent about this\noff the top of his head. :)\n\n-Peff\n"},{"id":"274032","messageId":"xmqqoae5ri5x.fsf@gitster.mtv.corp.google.com","threadId":"40910","inReplyTo":"20151204212712.GA22493@sigill.intra.peff.net","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-04T21:57:30Z","receivedAt":"2015-12-04T21:57:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This seems to reproduce consistently for me:\n>\n> $ git clone --depth=1 git://github.com/git/git\n> Cloning into 'git'...\n> remote: Counting objects: 2925, done.\n> remote: Compressing objects: 100% (2602/2602), done.\n> remote: Total 2925 (delta 230), reused 2329 (delta 206), pack-reused 0\n> Receiving objects: 100% (2925/2925), 6.17 MiB | 10.80 MiB/s, done.\n> Resolving deltas: 100% (230/230), done.\n>\n> $ cd git\n> $ git fetch --unshallow\n> remote: Counting objects: 185430, done.\n> remote: Compressing objects: 100% (46933/46933), done.\n> remote: Total 185430 (delta 140505), reused 181589 (delta 136694), pack-reused 0\n> Receiving objects: 100% (185430/185430), 52.80 MiB | 10.84 MiB/s, done.\n> Resolving deltas: 100% (140505/140505), completed with 1784 local objects.\n> remote: Counting objects: 579, done.\n> remote: Compressing objects: 100% (579/579), done.\n> remote: Total 579 (delta 0), reused 579 (delta 0), pack-reused 0\n> Receiving objects: 100% (579/579), 266.85 KiB | 0 bytes/s, done.\n> [... fetch output ...]\n>\n> That looks like two packs being received for the --unshallow case.\n\nWhat is puzzling is that I do not seem to see this \"two fetches\"\nwith the local transport. I only see \"deepen 2147483647\" in the\nprotocol log.\n\nMoreover, the only interesting lines in the output from\n\n $ git grep -B1 'deepen ' \\*.[ch]\n\nare\n\nfetch-pack.c- if (args->depth > 0)\nfetch-pack.c: packet_buf_write(&req_buf, \"deepen %d\", args->depth);\n\nso I do not see how anybody would be sending \"deepen 0\" as Jason\nsaw.\n"},{"id":"274033","messageId":"20151204221009.GA23129@sigill.intra.peff.net","threadId":"40910","inReplyTo":"xmqqoae5ri5x.fsf@gitster.mtv.corp.google.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-04T22:10:09Z","receivedAt":"2015-12-04T22:10:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 04, 2015 at 01:57:30PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > This seems to reproduce consistently for me:\n> >\n> > $ git clone --depth=1 git://github.com/git/git\n> > Cloning into 'git'...\n> > remote: Counting objects: 2925, done.\n> > remote: Compressing objects: 100% (2602/2602), done.\n> > remote: Total 2925 (delta 230), reused 2329 (delta 206), pack-reused 0\n> > Receiving objects: 100% (2925/2925), 6.17 MiB | 10.80 MiB/s, done.\n> > Resolving deltas: 100% (230/230), done.\n> >\n> > $ cd git\n> > $ git fetch --unshallow\n> > remote: Counting objects: 185430, done.\n> > remote: Compressing objects: 100% (46933/46933), done.\n> > remote: Total 185430 (delta 140505), reused 181589 (delta 136694), pack-reused 0\n> > Receiving objects: 100% (185430/185430), 52.80 MiB | 10.84 MiB/s, done.\n> > Resolving deltas: 100% (140505/140505), completed with 1784 local objects.\n> > remote: Counting objects: 579, done.\n> > remote: Compressing objects: 100% (579/579), done.\n> > remote: Total 579 (delta 0), reused 579 (delta 0), pack-reused 0\n> > Receiving objects: 100% (579/579), 266.85 KiB | 0 bytes/s, done.\n> > [... fetch output ...]\n> >\n> > That looks like two packs being received for the --unshallow case.\n> \n> What is puzzling is that I do not seem to see this \"two fetches\"\n> with the local transport. I only see \"deepen 2147483647\" in the\n> protocol log.\n\nYeah, I do not ever see \"deepen 0\" from GIT_TRACE_PACKET output. FWIW,\nhere's the output I am using to reproduce this locally:\n\n # do this once\n git clone --bare git://github.com/git/git src.git\n\n # do this for each test run\n rm -rf repo\n git clone --no-local --depth=1 src.git repo\n cd repo\n echo \"==> unshallow\" &&\n git fetch --progress --unshallow 2>&1 | grep remote\n\nAnd you can see that there are two separate sections (and I traced this\nto backfill_tags() with gdb).\n\nNote that the issue goes away if the shallow clone is done with \"--bare\"\n(I guess because we pick up tags differently in the initial clone?).\n\n-Peff\n"},{"id":"274034","messageId":"xmqqh9jxrfy5.fsf@gitster.mtv.corp.google.com","threadId":"40910","inReplyTo":"20151204215158.GA27987@sigill.intra.peff.net","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-04T22:45:22Z","receivedAt":"2015-12-04T22:45:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> But why would fetching a tag (or set of tags) merit a depth of zero?\n>> Doesn't depth 1 mean \"give me the the objects, and none of their\n>> descendants\"? Why use 0?\n>\n> That comes from this line:\n>\n> transport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n>\n> That line blame back to b888d61 (Make fetch a builtin, 2007-09-10),\n> which isn't incredibly helpful.\n\nHmm, \"0\" means \"no depth limitations\", which is exactly what we want\nin this \"unshallow\" case, I would think. The behaviour observed is\njust like a regular fetch that auto-follows tags, where it has to\nmake a second fetch if the primary fetch fails to include everything\nthat is needed for propagating the tag for whatever reason.\n\nHaving said that, IIRC, these days a depth limited clone is created\nimplicitly with --single-branch option, and I am not sure what the\nright behaviour for the auto-following of tags in such a repository.\n\n> I think that comes from the original git-fetch.sh, which had:\n>\n> ?*)\n> # do not deepen a shallow tree when following tags\n> shallow_depth=\n>\n> Which makes sense. I think the code at that point is not aware that we\n> just \"unshallowed\" and can therefore drop the depth parameter\n> altogether. But I admit I am not all that familiar with the shallow\n> code.\n>\n> +cc Duy, who can probably say something way more intelligent about this\n> off the top of his head. :)\n>\n> -Peff\n"},{"id":"274045","messageId":"CACsJy8BSNm6=E6Mo6etv_Lvdd2kpqQ7EEMBAoP-AQraYy3xU8w@mail.gmail.com","threadId":"40910","inReplyTo":"xmqqh9jxrfy5.fsf@gitster.mtv.corp.google.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-05T05:33:47Z","receivedAt":"2015-12-05T05:33:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Dec 4, 2015 at 11:45 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>>> But why would fetching a tag (or set of tags) merit a depth of zero?\n>>> Doesn't depth 1 mean \"give me the the objects, and none of their\n>>> descendants\"? Why use 0?\n>>\n>> That comes from this line:\n>>\n>> transport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n>>\n>> That line blame back to b888d61 (Make fetch a builtin, 2007-09-10),\n>> which isn't incredibly helpful.\n>\n> Hmm, \"0\" means \"no depth limitations\", which is exactly what we want\n> in this \"unshallow\" case, I would think. The behaviour observed is\n\nNo depth 0 means \"do not change depth\", which is why Jeff saw no\n'deepen' lines (and those lines should be rejected any way). It's\nequivalent of doing \"git fetch\" without --depth.\n\n> just like a regular fetch that auto-follows tags, where it has to\n> make a second fetch if the primary fetch fails to include everything\n> that is needed for propagating the tag for whatever reason.\n>\n> Having said that, IIRC, these days a depth limited clone is created\n> implicitly with --single-branch option, and I am not sure what the\n> right behaviour for the auto-following of tags in such a repository.\n\nI suppose followtags feature has been around long enough that we can\nsimply trust that and skip the second fetch? But it's not that easy\nfor subsequent fetches after the initial fetch in git-clone, because\nwe no longer know if --single-branch was used (of if there is any new\nbranch fetched since).\n-- \nDuy\n"},{"id":"274060","messageId":"xmqqmvtons4j.fsf@gitster.mtv.corp.google.com","threadId":"40910","inReplyTo":"CACsJy8BSNm6=E6Mo6etv_Lvdd2kpqQ7EEMBAoP-AQraYy3xU8w@mail.gmail.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-06T04:00:28Z","receivedAt":"2015-12-06T04:00:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Dec 4, 2015 at 11:45 PM, Junio C Hamano wrote:\n> ...\n>> just like a regular fetch that auto-follows tags, where it has to\n>> make a second fetch if the primary fetch fails to include everything\n>> that is needed for propagating the tag for whatever reason.\n>>\n>> Having said that, IIRC, these days a depth limited clone is created\n>> implicitly with --single-branch option, and I am not sure what the\n>> right behaviour for the auto-following of tags in such a repository.\n>\n> I suppose followtags feature has been around long enough that we can\n> simply trust that and skip the second fetch?\n\nHmmmm, I wonder why the code needs the backfill fetch while talking\nto a server that has the include-tag capability, then?\n"},{"id":"274061","messageId":"20151206063718.GA549@sigill.intra.peff.net","threadId":"40910","inReplyTo":"xmqqmvtons4j.fsf@gitster.mtv.corp.google.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-06T06:37:19Z","receivedAt":"2015-12-06T06:37:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 05, 2015 at 08:00:28PM -0800, Junio C Hamano wrote:\n\n> > I suppose followtags feature has been around long enough that we can\n> > simply trust that and skip the second fetch?\n> \n> Hmmmm, I wonder why the code needs the backfill fetch while talking\n> to a server that has the include-tag capability, then?\n\nThe logic in find_non_local_tags makes no sense to me.\n\nAfter we do the first fetch, we call this function to find out if there\nare any tags we need to pick up. We iterate through the list of refs\ngiven to us by the remote (including peeled lines), and do:\n\n if (ends_with(ref->name, \"^{}\")) {\n if (item && !has_sha1_file(ref->old_sha1) &&\n !will_fetch(head, ref->old_sha1) &&\n !has_sha1_file(item->util) &&\n !will_fetch(head, item->util))\n item->util = NULL;\n ...\n }\n\nwhere \"ref\" is the peeled line (i.e., the pointed-to commit), and \"item\"\nis the preceding line (i.e., the tag itself) with util set to its sha1.\nAny such item whose util survives as non-NULL is fetched in the backfill\nstep.\n\nSo I'd think the logic for backfilling a given tag should be:\n\n 1. We have the peeled object, and...\n\n 2. We don't currently have the tag pointing to it, and...\n\n 3. We are not already going to fetch it.\n\nYou could write that as:\n\n if (has_sha1_file(ref->old_sha1) &&\n !has_sha1_file(item->util) &&\n !will_fetch(head, item->util))\n\nOf course the conditional in the code is \"should we skip the backfill\",\nthe negation. So using De Morgan's laws, we'd write:\n\n if (!has_sha1_file(ref->old_sha1) ||\n has_sha1_file(item->util) ||\n will_fetch(head, item->util))\n\nWhich is quite a bit different than what is there. I'm not sure at all\nwhat the \"will_fetch(head, ref->old_sha1)\" is doing. In fact, for the\nbackfill step, it looks like we feed an empty \"head\" list, so the\n!will_fetch() is always true.\n\nAnd indeed, replacing the logic with what I wrote does make the backfill\ngo away in my test case. But it's so far from what is there that I feel\nlike I must be missing something.\n\n-Peff\n"},{"id":"274062","messageId":"20151206070144.GA17902@sigill.intra.peff.net","threadId":"40910","inReplyTo":"20151206063718.GA549@sigill.intra.peff.net","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-06T07:01:44Z","receivedAt":"2015-12-06T07:01:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 06, 2015 at 01:37:18AM -0500, Jeff King wrote:\n\n> And indeed, replacing the logic with what I wrote does make the backfill\n> go away in my test case. But it's so far from what is there that I feel\n> like I must be missing something.\n\nI think one thing I was missing is that we need to just grab the\n_object_, but we need to realize that the ref needs updating[1]. So we\ncannot skip backfill of any tag that we do not already have, even if we\nalready have the tag object.\n\nWhich made me wonder why this:\n\n git init parent &&\n git -C parent commit --allow-empty -m one &&\n git clone parent child &&\n git -C parent commit --allow-empty -m two &&\n git -C parent tag -m mytag foo &&\n git -C parent commit --allow-empty -m three &&\n git -C child fetch\n\ndoes not appear to need to backfill to pick up refs/tags/foo. But it\ndoes. It's just that it hits the quickfetch() code path and does not\nhave to ask the other side for a pack. And that explains why it does hit\nin the --shallow case: we explicitly disable quickfetch in such cases.\n\nFor the unshallow case, of course we could use it (but only for the\nsecond, backfill fetch). Something like this seems to work for me:\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ed84963..b33b90f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -881,6 +881,8 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n \n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n+\tif (unshallow)\n+\t\tdepth = NULL;\n \tfetch_refs(transport, ref_map);\n \n \tif (gsecondary) {\n\nBut I admit I am not at all confident that it doesn't cause other\nproblems, or that it covers all cases. Even in a shallow repo, we should\nbe able to quickfetch individual tags, shouldn't we? I wonder if we\ncould just always set \"depth = NULL\" here.\n\n-Peff\n\n[1] I'm still puzzled why find_non_local_tags uses has_sha1_file() on\n the tag object at all, then.\n"},{"id":"274066","messageId":"CACsJy8DA2Xg9bRmudsRgwy9k=BCawTRU=7bm+rEXv2KoZgoXJw@mail.gmail.com","threadId":"40910","inReplyTo":"20151206070144.GA17902@sigill.intra.peff.net","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-06T10:46:31Z","receivedAt":"2015-12-06T10:46:31Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Dec 6, 2015 at 8:01 AM, Jeff King wrote:\n> On Sun, Dec 06, 2015 at 01:37:18AM -0500, Jeff King wrote:\n>\n>> And indeed, replacing the logic with what I wrote does make the backfill\n>> go away in my test case. But it's so far from what is there that I feel\n>> like I must be missing something.\n>\n> I think one thing I was missing is that we need to just grab the\n> _object_, but we need to realize that the ref needs updating[1]. So we\n> cannot skip backfill of any tag that we do not already have, even if we\n> already have the tag object.\n\nIt's probably worth adding a few comment lines about this. I did\nsearch back commit history but did not get this.\n\n> Which made me wonder why this:\n>\n> git init parent &&\n> git -C parent commit --allow-empty -m one &&\n> git clone parent child &&\n> git -C parent commit --allow-empty -m two &&\n> git -C parent tag -m mytag foo &&\n> git -C parent commit --allow-empty -m three &&\n> git -C child fetch\n>\n> does not appear to need to backfill to pick up refs/tags/foo. But it\n> does. It's just that it hits the quickfetch() code path and does not\n> have to ask the other side for a pack. And that explains why it does hit\n> in the --shallow case: we explicitly disable quickfetch in such cases.\n>\n> For the unshallow case, of course we could use it (but only for the\n> second, backfill fetch). Something like this seems to work for me:\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index ed84963..b33b90f 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -881,6 +881,8 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n>\n> transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n> transport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n> + if (unshallow)\n> + depth = NULL;\n> fetch_refs(transport, ref_map);\n>\n> if (gsecondary) {\n>\n> But I admit I am not at all confident that it doesn't cause other\n> problems, or that it covers all cases. Even in a shallow repo, we should\n> be able to quickfetch individual tags, shouldn't we?\n\nYes. depth is only non-NULL when you pass --depth (or --unshallow).\nquickfetch should happen when you fetch without those options.\n\n> I wonder if we could just always set \"depth = NULL\" here.\n\n--unshallow is essentially --depth=, so I don't see why\n--unshallow should be singled out here. We probably want to restore\ndepth back (or pass a flag to explicitly ignore the \"depth\" exception\nin quickfetch). For multiple fetches, we spawn new commands so depth\nbeing NULL does not harm. Just in case somebody tries to fetch a\ncouple more times in the same process in future.\n-- \nDuy\n"},{"id":"274130","messageId":"CACs8u9TvUKiPN9Brgym_zDea+_-MWWP9r4wZDa_qib6QxJ6YcA@mail.gmail.com","threadId":"40910","inReplyTo":"CACsJy8DA2Xg9bRmudsRgwy9k=BCawTRU=7bm+rEXv2KoZgoXJw@mail.gmail.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Jason Paller-Rzepka","fromEmail":"jasonpr@google.com","sentAt":"2015-12-07T19:57:55Z","receivedAt":"2015-12-07T19:57:55Z","isPatch":false,"sender":{"key":"jasonpr@google.com","avatar":null},"body":"I should point out that I never saw a \"deepen 0\" line. Rather, I saw\ngit send \"option depth 0\" to the remote helper.\nDuy, you mentioned that depth=0 means \"do not change depth\". I assume\nthat means the server should use exactly the shallows that the client\nsent, and it does not need to traverse the tree or modify the shallow\nor unshallow sets at all. Right?\nDuy, you also mentioned that \"those lines should be rejected any way\".\nYou just mean that a \"deepen 0\" line should be rejected, right? And\nthat's because the right way to tell git-upload-pack not to change the\ndepth is to omit the \"deepen\" line after the \"shallow\" lines, so\nthere's never a need to send \"deepen 0\"?\n\nThanks!\n\nOn Sun, Dec 6, 2015 at 5:46 AM, Duy Nguyen wrote:\n> On Sun, Dec 6, 2015 at 8:01 AM, Jeff King wrote:\n>> On Sun, Dec 06, 2015 at 01:37:18AM -0500, Jeff King wrote:\n>>\n>>> And indeed, replacing the logic with what I wrote does make the backfill\n>>> go away in my test case. But it's so far from what is there that I feel\n>>> like I must be missing something.\n>>\n>> I think one thing I was missing is that we need to just grab the\n>> _object_, but we need to realize that the ref needs updating[1]. So we\n>> cannot skip backfill of any tag that we do not already have, even if we\n>> already have the tag object.\n>\n> It's probably worth adding a few comment lines about this. I did\n> search back commit history but did not get this.\n>\n>> Which made me wonder why this:\n>>\n>> git init parent &&\n>> git -C parent commit --allow-empty -m one &&\n>> git clone parent child &&\n>> git -C parent commit --allow-empty -m two &&\n>> git -C parent tag -m mytag foo &&\n>> git -C parent commit --allow-empty -m three &&\n>> git -C child fetch\n>>\n>> does not appear to need to backfill to pick up refs/tags/foo. But it\n>> does. It's just that it hits the quickfetch() code path and does not\n>> have to ask the other side for a pack. And that explains why it does hit\n>> in the --shallow case: we explicitly disable quickfetch in such cases.\n>>\n>> For the unshallow case, of course we could use it (but only for the\n>> second, backfill fetch). Something like this seems to work for me:\n>>\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index ed84963..b33b90f 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -881,6 +881,8 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n>>\n>> transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n>> transport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n>> + if (unshallow)\n>> + depth = NULL;\n>> fetch_refs(transport, ref_map);\n>>\n>> if (gsecondary) {\n>>\n>> But I admit I am not at all confident that it doesn't cause other\n>> problems, or that it covers all cases. Even in a shallow repo, we should\n>> be able to quickfetch individual tags, shouldn't we?\n>\n> Yes. depth is only non-NULL when you pass --depth (or --unshallow).\n> quickfetch should happen when you fetch without those options.\n>\n>> I wonder if we could just always set \"depth = NULL\" here.\n>\n> --unshallow is essentially --depth=, so I don't see why\n> --unshallow should be singled out here. We probably want to restore\n> depth back (or pass a flag to explicitly ignore the \"depth\" exception\n> in quickfetch). For multiple fetches, we spawn new commands so depth\n> being NULL does not harm. Just in case somebody tries to fetch a\n> couple more times in the same process in future.\n> --\n> Duy\n"},{"id":"274144","messageId":"CACsJy8CQx-jL0aWW4mpGopNQtdJV7+8YCy2SzVL=VJSbBKAwMg@mail.gmail.com","threadId":"40910","inReplyTo":"CACs8u9RzUVWw2Ld1K7JeO7Eci114JEiML8bbGy96m4pZZk=FnA@mail.gmail.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-07T21:21:59Z","receivedAt":"2015-12-07T21:21:59Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 7, 2015 at 8:57 PM, Jason Paller-Rzepka wrote:\n> Duy, you mentioned that depth=0 means \"do not change depth\". I assume that\n> means the server should use exactly the shallows that the client sent, and\n> it does not need to traverse the tree or modify the shallow or unshallow\n> sets at all. Right?\n\nCorrect. The server might send new shallow lines anyway though, if the\nserver repo is also shallow and the new fetched ref needs to be cut.\nBut I don't know if Dulwich supports that yet.\n\n> Duy, you also mentioned that \"those lines should be rejected any way\". You\n> just mean that a \"deepen 0\" line should be rejected, right? And that's\n> because the right way to tell git-upload-pack not to change the depth is to\n> omit the \"deepen\" line after the \"shallow\" lines, so there's never a need to\n> send \"deepen 0\"?\n\nAlso correct. I didn't check the code when I wrote that. But I have\nchecked and upload-pack does reject \"deepen 0\"\n\nif (starts_with(line, \"deepen \")) {\n char *end;\n depth = strtol(line + 7, &end, 0);\n if (end == line + 7 || depth <= 0)\n die(\"Invalid deepen: %s\", line);\n-- \nDuy\n"},{"id":"274145","messageId":"xmqqa8pmlzjc.fsf@gitster.mtv.corp.google.com","threadId":"40910","inReplyTo":"20151206070144.GA17902@sigill.intra.peff.net","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-07T21:27:51Z","receivedAt":"2015-12-07T21:27:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think one thing I was missing is that we need to just grab the\n> _object_, but we need to realize that the ref needs updating[1]. So we\n> cannot skip backfill of any tag that we do not already have, even if we\n> already have the tag object.\n> ...\n> [1] I'm still puzzled why find_non_local_tags uses has_sha1_file() on\n> the tag object at all, then.\n\nThe designed semantics of auto-following tags (not necessarily as\nimplemented or documented, i.e. there may be implementation or\ndocumentation bugs), I think, is to arrive at the same state as\ndoing a fetch (or a push) without the auto-following and then doing\na separate fetch (or a push) of tags that point at the objects that\nare reachable from the tips of refs after finishing the first\n(i.e. without auto-follow) fetch (or a push). In a scenario where\nwe already have a commit reachable from existing remote-tracking\nbranch and the current transfer (be it a fetch or a push, with or\nwithout auto-follow) does not update any remote-tracking branch\n(because the source side did not have any changes), if the source\nside added a tag that refers to that commit that the receiving end\nlacks, that tag needs to be transferred and then stored.\n\nSo has_sha1_file() is not the right test---if anything, it needs to\nbe checking if the object being checked is reachable from a tip of\nsome ref.\n\nBut of course, that test is rather expensive, so perhaps the\nimplementation cheated and uses has_sha1_file() instead? The only\ncase it would misidentify would be after an aborted fetch (or push)\nleft unconnected island of objects and some of these objects that\nare not reachable are pointed at by tags the receiving end does not\nhave.\n\n\n\n\n\n \n"},{"id":"274147","messageId":"20151207214257.GA32082@sigill.intra.peff.net","threadId":"40910","inReplyTo":"xmqqa8pmlzjc.fsf@gitster.mtv.corp.google.com","subject":"Re: Multiple fetches when unshallowing a shallow clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-07T21:42:58Z","receivedAt":"2015-12-07T21:42:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 07, 2015 at 01:27:51PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think one thing I was missing is that we need to just grab the\n> > _object_, but we need to realize that the ref needs updating[1]. So we\n> > cannot skip backfill of any tag that we do not already have, even if we\n> > already have the tag object.\n> > ...\n> > [1] I'm still puzzled why find_non_local_tags uses has_sha1_file() on\n> > the tag object at all, then.\n> \n> The designed semantics of auto-following tags (not necessarily as\n> implemented or documented, i.e. there may be implementation or\n> documentation bugs), I think, is to arrive at the same state as\n> doing a fetch (or a push) without the auto-following and then doing\n> a separate fetch (or a push) of tags that point at the objects that\n> are reachable from the tips of refs after finishing the first\n> (i.e. without auto-follow) fetch (or a push). In a scenario where\n> we already have a commit reachable from existing remote-tracking\n> branch and the current transfer (be it a fetch or a push, with or\n> without auto-follow) does not update any remote-tracking branch\n> (because the source side did not have any changes), if the source\n> side added a tag that refers to that commit that the receiving end\n> lacks, that tag needs to be transferred and then stored.\n> \n> So has_sha1_file() is not the right test---if anything, it needs to\n> be checking if the object being checked is reachable from a tip of\n> some ref.\n> \n> But of course, that test is rather expensive, so perhaps the\n> implementation cheated and uses has_sha1_file() instead? The only\n> case it would misidentify would be after an aborted fetch (or push)\n> left unconnected island of objects and some of these objects that\n> are not reachable are pointed at by tags the receiving end does not\n> have.\n\nI may have confused myself. There are actually two has_sha1_file() calls\nin find_non_local_tags.\n\nI agree it is the only sensible test for \"do we have the commit this tag\npeels to, and if so, we want to grab the tag\". Reachability is too\nexpensive to compute.\n\nBut for the other one (\"do we have the tag object itself\"), I initially\nclaimed \"if we have the tag object already, we do not have to do the\nbackfill fetch\". Which is not quite true. We have to update the ref even\nif we have the tag object. But then, what if we have the tag object for\nother reasons (e.g., because another tag points at it?).\n\nE.g. in this sequence:\n\n git -C parent commit --allow-empty -m base\n git -C parent tag -m mytag foo\n git clone parent child\n git -C parent update-ref refs/tags/bar foo\n git -C child fetch\n\nwe must backfill refs/tags/bar during the fetch, even though we already\nhave the object. I don't see any point in checking has_sha1_file() for\nthe tagged object at all. If we don't have it, we obviously must fetch.\nAnd if we do have it, we must fetch the ref, even if that results in no\nobjects transferred.\n\nIt's entirely possible I'm just confused, and AFAICT nobody has noticed\nany breakage here, so please don't feel you need to spend a lot of time\nhumoring me. I'm just writing up my confusion for posterity. :)\n\n-Peff\n"}]}