{"thread":{"id":"39589","subject":"[PATCH] clone: check if server supports shallow clones","startedAt":"2015-06-10T18:35:20Z","lastAt":"2015-06-17T17:00:01Z","messageCount":12,"participants":["Mike Edgar","Jeff King","Michael Edgar","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"263518","messageId":"1433961320-1366-1-git-send-email-adgar@google.com","threadId":"39589","inReplyTo":null,"subject":"[PATCH] clone: check if server supports shallow clones","fromName":"Mike Edgar","fromEmail":"adgar@google.com","sentAt":"2015-06-10T18:35:20Z","receivedAt":"2015-06-10T18:35:20Z","isPatch":true,"sender":{"key":"adgar@google.com","avatar":"https://avatars.githubusercontent.com/u/65914?v=4"},"body":"When the user passes --depth to git-clone the server's capabilities are\nnot currently consulted. The client will send shallow requests even if\nthe server does not understand them, and the resulting error may be\nunhelpful to the user. This change pre-emptively checks so git-clone can\nexit with a helpful error if necessary.\n\nSigned-off-by: Mike Edgar \n---\n builtin/clone.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b878252..b4e9846 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -944,6 +944,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \trefs = transport_get_remote_refs(transport);\n \n+\tif (option_depth && !is_local && !server_supports(\"shallow\"))\n+\t\tdie(_(\"Server does not support shallow clients\"));\n+\n \tif (refs) {\n \t\tmapped_refs = wanted_peer_refs(refs, refspec);\n \t\t/*\n-- \n2.2.0.rc0.207.ga3a616c\n"},{"id":"263523","messageId":"20150610190512.GB22800@peff.net","threadId":"39589","inReplyTo":"1433961320-1366-1-git-send-email-adgar@google.com","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-10T19:05:14Z","receivedAt":"2015-06-10T19:05:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 10, 2015 at 02:35:20PM -0400, Mike Edgar wrote:\n\n> When the user passes --depth to git-clone the server's capabilities are\n> not currently consulted. The client will send shallow requests even if\n> the server does not understand them, and the resulting error may be\n> unhelpful to the user. This change pre-emptively checks so git-clone can\n> exit with a helpful error if necessary.\n\nThis sounds like a good thing to do, but...\n\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -944,6 +944,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \n> \trefs = transport_get_remote_refs(transport);\n> \n> +\tif (option_depth && !is_local && !server_supports(\"shallow\"))\n> +\t\tdie(_(\"Server does not support shallow clients\"));\n> +\n\nIt feels a little weird to be checking the option here in cmd_clone.\nThe transport layer knows we have specified a depth, so it seems like a\nmore natural place for it (or possibly even lower, in the actual\ngit-protocol code).\n\nThat being said, I think the current capabilities handling is a bit\nmessy and crosses module boundaries freely. So I would not be surprised\nif this is the most reasonable place to put it. But it does make me\nwonder whether \"git fetch --depth=...\" needs the same treatment.\n\nI see that do_fetch_pack checks server_supports(\"shallow\"). Is that\nenough to cover all fetch cases? And if it is, why does it not cover the\nmatching clone cases?\n\n-Peff\n"},{"id":"263527","messageId":"CALkbG6+Xqw+giW3msUKYx4odFv+2GAi=C2t2d=6VjuFp0xpO0w@mail.gmail.com","threadId":"39589","inReplyTo":"20150610190512.GB22800@peff.net","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Michael Edgar","fromEmail":"adgar@google.com","sentAt":"2015-06-10T20:25:45Z","receivedAt":"2015-06-10T20:25:45Z","isPatch":true,"sender":{"key":"adgar@google.com","avatar":"https://avatars.githubusercontent.com/u/65914?v=4"},"body":"> On Wed, Jun 10, 2015 at 3:05 PM, Jeff King wrote:\n> I see that do_fetch_pack checks server_supports(\"shallow\"). Is that\n> enough to cover all fetch cases? And if it is, why does it not cover the\n> matching clone cases?\n> -Peff\n\nGreat question. I determined that the do_fetch_pack logic doesn't work for\nclones because it also checks is_repository_shallow(), which looks for the\n.git/shallow file (or the alternate file).\n\nI considered changing clone to create an empty .git/shallow file or alternate,\nbut it turns out that an empty shallow file is treated as no shallow file at\nall. Since at this stage in cloning we have nothing to put in the shallow file,\nit seemed like any other fix would require more substantial changes.\n"},{"id":"263529","messageId":"20150610205607.GA25842@peff.net","threadId":"39589","inReplyTo":"CALkbG6+Xqw+giW3msUKYx4odFv+2GAi=C2t2d=6VjuFp0xpO0w@mail.gmail.com","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-10T20:56:08Z","receivedAt":"2015-06-10T20:56:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 10, 2015 at 04:25:45PM -0400, Michael Edgar wrote:\n\n> > On Wed, Jun 10, 2015 at 3:05 PM, Jeff King wrote:\n> > I see that do_fetch_pack checks server_supports(\"shallow\"). Is that\n> > enough to cover all fetch cases? And if it is, why does it not cover the\n> > matching clone cases?\n> > -Peff\n> \n> Great question. I determined that the do_fetch_pack logic doesn't work for\n> clones because it also checks is_repository_shallow(), which looks for the\n> .git/shallow file (or the alternate file).\n> \n> I considered changing clone to create an empty .git/shallow file or alternate,\n> but it turns out that an empty shallow file is treated as no shallow file at\n> all. Since at this stage in cloning we have nothing to put in the shallow file,\n> it seemed like any other fix would require more substantial changes.\n\nHmm. Can we improve the check in do_fetch_pack? That is, do we have a\nway of checking whether the _current_ fetch is going to make us shallow?\nI don't see anything obvious (like a \"depth\" flag) passed into\ndo_fetch_pack, so I guess it is relying solely on global shallow state\nthat we've already set up? Or is it the case that \"git fetch\n--depth=\" in a non-shallow repository does not properly check this flag?\n\nIf it turns out that \"fetch --depth\" is fine, and only \"clone --depth\"\nis broken, I can certainly live with the patch you provided. But I think\nwe need to clarify that a bit in the commit message.\n\n-Peff\n"},{"id":"263580","messageId":"CACsJy8CiwiWgf2CarNNN5NgN7QbRB8oxGMmxF+VX8T=ZV2M1ow@mail.gmail.com","threadId":"39589","inReplyTo":"20150610190512.GB22800@peff.net","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-11T13:02:33Z","receivedAt":"2015-06-11T13:02:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 11, 2015 at 2:05 AM, Jeff King wrote:\n> On Wed, Jun 10, 2015 at 02:35:20PM -0400, Mike Edgar wrote:\n>\n>> When the user passes --depth to git-clone the server's capabilities are\n>> not currently consulted. The client will send shallow requests even if\n>> the server does not understand them, and the resulting error may be\n>> unhelpful to the user. This change pre-emptively checks so git-clone can\n>> exit with a helpful error if necessary.\n>\n> This sounds like a good thing to do, but...\n>\n>> --- a/builtin/clone.c\n>> +++ b/builtin/clone.c\n>> @@ -944,6 +944,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>>\n>> refs = transport_get_remote_refs(transport);\n>>\n>> + if (option_depth && !is_local && !server_supports(\"shallow\"))\n>> + die(_(\"Server does not support shallow clients\"));\n>> +\n>\n> It feels a little weird to be checking the option here in cmd_clone.\n> The transport layer knows we have specified a depth, so it seems like a\n> more natural place for it (or possibly even lower, in the actual\n> git-protocol code).\n>\n> That being said, I think the current capabilities handling is a bit\n> messy and crosses module boundaries freely. So I would not be surprised\n> if this is the most reasonable place to put it. But it does make me\n> wonder whether \"git fetch --depth=...\" needs the same treatment.\n>\n> I see that do_fetch_pack checks server_supports(\"shallow\"). Is that\n> enough to cover all fetch cases? And if it is, why does it not cover the\n> matching clone cases?\n\nI think this replacement check would do\n\nif ((args->depth > 0 || is_repository_shallow()) && !server_supports(\"shallow\"))\n die(\"Server does not support shallow clients\");\n-- \nDuy\n"},{"id":"263585","messageId":"20150611143204.GA3343@peff.net","threadId":"39589","inReplyTo":"CACsJy8CiwiWgf2CarNNN5NgN7QbRB8oxGMmxF+VX8T=ZV2M1ow@mail.gmail.com","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-11T14:32:07Z","receivedAt":"2015-06-11T14:32:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 11, 2015 at 08:02:33PM +0700, Duy Nguyen wrote:\n\n> > I see that do_fetch_pack checks server_supports(\"shallow\"). Is that\n> > enough to cover all fetch cases? And if it is, why does it not cover the\n> > matching clone cases?\n> \n> I think this replacement check would do\n> \n> if ((args->depth > 0 || is_repository_shallow()) && !server_supports(\"shallow\"))\n> die(\"Server does not support shallow clients\");\n\nOh, indeed, there is the depth flag I was looking for. :)\n\nAnd from some rudimentary testing, I believe that:\n\n git init\n git fetch --depth=1 ...\n\nis currently broken in the same way as clone (we are not shallow yet, so\nit does not complain when the server does not support it). I think the\npatch above fixes both that and the clone case.\n\nOf course it's hard to add to the test suite, since we do not have a way\nof hitting a server that does not understand shallow (I simply fudged\nserver_supports() to return false on the client).\n\n-Peff\n"},{"id":"263624","messageId":"CALkbG6+vRqKwmBjK9jwSph4iUUbqPjiuNdHEeT3nvFhFJ0VpEg@mail.gmail.com","threadId":"39589","inReplyTo":"20150611143204.GA3343@peff.net","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Michael Edgar","fromEmail":"adgar@google.com","sentAt":"2015-06-11T18:18:00Z","receivedAt":"2015-06-11T18:18:00Z","isPatch":true,"sender":{"key":"adgar@google.com","avatar":"https://avatars.githubusercontent.com/u/65914?v=4"},"body":"On Thu, Jun 11, 2015 at 10:32 AM, Jeff King wrote:\n> On Thu, Jun 11, 2015 at 08:02:33PM +0700, Duy Nguyen wrote:\n>\n>> > I see that do_fetch_pack checks server_supports(\"shallow\"). Is that\n>> > enough to cover all fetch cases? And if it is, why does it not cover the\n>> > matching clone cases?\n>>\n>> I think this replacement check would do\n>>\n>> if ((args->depth > 0 || is_repository_shallow()) && !server_supports(\"shallow\"))\n>> die(\"Server does not support shallow clients\");\n>\n> Oh, indeed, there is the depth flag I was looking for. :)\n>\n> And from some rudimentary testing, I believe that:\n>\n> git init\n> git fetch --depth=1 ...\n>\n> is currently broken in the same way as clone (we are not shallow yet, so\n> it does not complain when the server does not support it). I think the\n> patch above fixes both that and the clone case.\n\nGreat point! Duy's proposed fix worked for me testing against the Git\nserver that\nled me to the original bug (code.google.com), for both clones and init/fetch.\n\nShall I send that out as a revised patch for review? (This is my first\nstab at a Git\npatch)\n\n> Of course it's hard to add to the test suite, since we do not have a way\n> of hitting a server that does not understand shallow (I simply fudged\n> server_supports() to return false on the client).\n\nI noticed this, and figured it would be more dangerous for users to include a\ntesting backdoor (eg a flag overriding the advertised capabilities). Is that an\naccurate assessment, or is there a safer way to test these sorts of interop\ncases?\n\nCheers!\n-- \nMichael Edgar | Software Engineer | adgar@google.com\n"},{"id":"263638","messageId":"xmqqzj46dlfw.fsf@gitster.dls.corp.google.com","threadId":"39589","inReplyTo":"CALkbG6+vRqKwmBjK9jwSph4iUUbqPjiuNdHEeT3nvFhFJ0VpEg@mail.gmail.com","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-11T20:44:35Z","receivedAt":"2015-06-11T20:44:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Edgar writes:\n\n> On Thu, Jun 11, 2015 at 10:32 AM, Jeff King wrote:\n>> On Thu, Jun 11, 2015 at 08:02:33PM +0700, Duy Nguyen wrote:\n>>\n>>> > I see that do_fetch_pack checks server_supports(\"shallow\"). Is that\n>>> > enough to cover all fetch cases? And if it is, why does it not cover the\n>>> > matching clone cases?\n>>>\n>>> I think this replacement check would do\n>>>\n>>> if ((args->depth > 0 || is_repository_shallow()) && !server_supports(\"shallow\"))\n>>> die(\"Server does not support shallow clients\");\n>>\n>> Oh, indeed, there is the depth flag I was looking for. :)\n>>\n>> And from some rudimentary testing, I believe that:\n>>\n>> git init\n>> git fetch --depth=1 ...\n>>\n>> is currently broken in the same way as clone (we are not shallow yet, so\n>> it does not complain when the server does not support it). I think the\n>> patch above fixes both that and the clone case.\n>\n> Shall I send that out as a revised patch for review? (This is my first\n> stab at a Git\n> patch)\n\nSurely, and thanks. Hopefully one or both of them will Ack it and\nall there is left for me to do will be to pick it up ;-)\n"},{"id":"264007","messageId":"1434541694-12748-1-git-send-email-adgar@google.com","threadId":"39589","inReplyTo":"xmqqzj46dlfw.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] fetch-pack: check for shallow if depth given","fromName":"Mike Edgar","fromEmail":"adgar@google.com","sentAt":"2015-06-17T11:48:14Z","receivedAt":"2015-06-17T11:48:14Z","isPatch":true,"sender":{"key":"adgar@google.com","avatar":"https://avatars.githubusercontent.com/u/65914?v=4"},"body":"When a repository is first fetched as a shallow clone, either by\ngit-clone or by fetching into an empty repo, the server's capabilities\nare not currently consulted. The client will send shallow requests even\nif the server does not understand them, and the resulting error may be\nunhelpful to the user. This change pre-emptively checks so we can exit\nwith a helpful error if necessary.\n\nSigned-off-by: Mike Edgar \n---\n fetch-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a912935..a136772 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -809,7 +809,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tsort_ref_list(&ref, ref_compare_name);\n \tqsort(sought, nr_sought, sizeof(*sought), cmp_ref_by_name);\n \n-\tif (is_repository_shallow() && !server_supports(\"shallow\"))\n+\tif ((args->depth > 0 || is_repository_shallow()) && !server_supports(\"shallow\"))\n \t\tdie(\"Server does not support shallow clients\");\n \tif (server_supports(\"multi_ack_detailed\")) {\n \t\tif (args->verbose)\n-- \n2.2.0.rc0.207.ga3a616c\n"},{"id":"264038","messageId":"xmqq8ubi47jo.fsf@gitster.dls.corp.google.com","threadId":"39589","inReplyTo":"20150611143204.GA3343@peff.net","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-17T16:35:23Z","receivedAt":"2015-06-17T16:35:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jun 11, 2015 at 08:02:33PM +0700, Duy Nguyen wrote:\n>\n>> > I see that do_fetch_pack checks server_supports(\"shallow\"). Is that\n>> > enough to cover all fetch cases? And if it is, why does it not cover the\n>> > matching clone cases?\n>> \n>> I think this replacement check would do\n>> \n>> if ((args->depth > 0 || is_repository_shallow()) && !server_supports(\"shallow\"))\n>> die(\"Server does not support shallow clients\");\n>\n> Oh, indeed, there is the depth flag I was looking for. :)\n>\n> And from some rudimentary testing, I believe that:\n>\n> git init\n> git fetch --depth=1 ...\n>\n> is currently broken in the same way as clone (we are not shallow yet, so\n> it does not complain when the server does not support it). I think the\n> patch above fixes both that and the clone case.\n>\n> Of course it's hard to add to the test suite, since we do not have a way\n> of hitting a server that does not understand shallow (I simply fudged\n> server_supports() to return false on the client).\n\nWe've had the \"shallow\" capability advertised since ed09aef0\n(support fetching into a shallow repository, 2006-10-30), and this\npatch itself may not be that super-important in practice. Let's not\nworry too much about a test for situations that may not likely\nmatter to us [*1*].\n\nThanks, all.\n\n\n[Footnote]\n\n*1* How behind are re-implementations of upload-pack by other\npeople, I have to wonder, though?\n"},{"id":"264044","messageId":"20150617165909.GB22689@peff.net","threadId":"39589","inReplyTo":"xmqq8ubi47jo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] clone: check if server supports shallow clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T16:59:10Z","receivedAt":"2015-06-17T16:59:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 17, 2015 at 09:35:23AM -0700, Junio C Hamano wrote:\n\n> > Of course it's hard to add to the test suite, since we do not have a way\n> > of hitting a server that does not understand shallow (I simply fudged\n> > server_supports() to return false on the client).\n> \n> We've had the \"shallow\" capability advertised since ed09aef0\n> (support fetching into a shallow repository, 2006-10-30), and this\n> patch itself may not be that super-important in practice. Let's not\n> worry too much about a test for situations that may not likely\n> matter to us [*1*].\n\nI had actually started looking at doing a generic interop testing\nsuite. It would be nice to be able to do something like:\n\n cd t/interop\n ./run v1.0.0 v2.0.0 ./t0001-clone.sh\n\nand then the test script looks something like:\n\n test_expect_success 'clone with A from B' '\n\tgit.a clone -u \"git.b upload-pack\"\n '\n\nThe \"run\" script is similar to the version in t/perf that builds\narbitrary revisions for testing, but with the twist that it points\nthe PATH to \"git.a\" and \"git.b\", which symlink into the bin-wrappers/ of\nthe built directories (and probably disallows bare \"git\" to prevent\nmistakes).\n\nBut I agree that this particular bug is not all that exciting to test.\n\n> *1* How behind are re-implementations of upload-pack by other\n> people, I have to wonder, though?\n\nJGit advertises \"shallow\". Libgit2 does not, but it also does not\nimplement upload-pack. :)\n\nI do wonder which server Mike was hitting to come across this in the\nfirst place. Maybe the Google Code dulwich-based one?\n\n-Peff\n"},{"id":"264045","messageId":"20150617170001.GC22689@peff.net","threadId":"39589","inReplyTo":"1434541694-12748-1-git-send-email-adgar@google.com","subject":"Re: [PATCH v2] fetch-pack: check for shallow if depth given","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T17:00:01Z","receivedAt":"2015-06-17T17:00:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 17, 2015 at 07:48:14AM -0400, Mike Edgar wrote:\n\n> When a repository is first fetched as a shallow clone, either by\n> git-clone or by fetching into an empty repo, the server's capabilities\n> are not currently consulted. The client will send shallow requests even\n> if the server does not understand them, and the resulting error may be\n> unhelpful to the user. This change pre-emptively checks so we can exit\n> with a helpful error if necessary.\n> \n> Signed-off-by: Mike Edgar \n\nLooks good to me.\n\n-Peff\n"}]}