{"thread":{"id":"51077","subject":"[PATCH] doc: explain why file: URLs and bundles don't mix","startedAt":"2019-05-13T00:24:42Z","lastAt":"2019-05-14T09:30:26Z","messageCount":5,"participants":["Alyssa Ross","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"375355","messageId":"20190513002355.25031-1-hi@alyssa.is","threadId":"51077","inReplyTo":null,"subject":"[PATCH] doc: explain why file: URLs and bundles don't mix","fromName":"Alyssa Ross","fromEmail":"hi@alyssa.is","sentAt":"2019-05-13T00:23:55Z","receivedAt":"2019-05-13T00:24:42Z","isPatch":true,"sender":{"key":"hi@alyssa.is","avatar":"https://avatars.githubusercontent.com/u/2768870?v=4"},"body":"Signed-off-by: Alyssa Ross \n---\n Documentation/urls.txt | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/urls.txt b/Documentation/urls.txt\nindex b05da95788..c83d9f859e 100644\n--- a/Documentation/urls.txt\n+++ b/Documentation/urls.txt\n@@ -43,14 +43,24 @@ syntaxes may be used:\n - \\file:///path/to/repo.git/\n \n ifndef::git-clone[]\n-These two syntaxes are mostly equivalent, except when cloning, when\n-the former implies --local option. See linkgit:git-clone[1] for\n-details.\n+These two syntaxes are mostly equivalent, with some exceptions:\n+\n+- When cloning, the former implies --local option. See\n+ linkgit:git-clone[1] for details.\n+\n+- The latter is implemented using linkgit:git-upload-pack[1], which\n+ expects its repository to be a directory, and therefore does not\n+ work for bundles (see linkgit:git-bundle[1]).\n endif::git-clone[]\n \n ifdef::git-clone[]\n-These two syntaxes are mostly equivalent, except the former implies\n---local option.\n+These two syntaxes are mostly equivalent, with some exceptions:\n+\n+- The former implies --local option.\n+\n+- The latter is implemented using linkgit:git-upload-pack[1], which\n+ expects its repository to be a directory, and therefore does not\n+ work for bundles (see linkgit:git-bundle[1]).\n endif::git-clone[]\n \n When Git doesn't know how to handle a certain transport protocol, it\n-- \n2.21.0\n\n"},{"id":"375368","messageId":"xmqqv9yebwzp.fsf@gitster-ct.c.googlers.com","threadId":"51077","inReplyTo":"20190513002355.25031-1-hi@alyssa.is","subject":"Re: [PATCH] doc: explain why file: URLs and bundles don't mix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-13T08:05:14Z","receivedAt":"2019-05-13T08:05:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alyssa Ross writes:\n\n> Signed-off-by: Alyssa Ross \n> ---\n> Documentation/urls.txt | 20 +++++++++++++++-----\n> 1 file changed, 15 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/urls.txt b/Documentation/urls.txt\n> index b05da95788..c83d9f859e 100644\n> --- a/Documentation/urls.txt\n> +++ b/Documentation/urls.txt\n> @@ -43,14 +43,24 @@ syntaxes may be used:\n> - \\file:///path/to/repo.git/\n> \n> ifndef::git-clone[]\n> -These two syntaxes are mostly equivalent, except when cloning, when\n> -the former implies --local option. See linkgit:git-clone[1] for\n> -details.\n> +These two syntaxes are mostly equivalent, with some exceptions:\n> +\n> +- When cloning, the former implies --local option. See\n> + linkgit:git-clone[1] for details.\n> +\n> +- The latter is implemented using linkgit:git-upload-pack[1], which\n> + expects its repository to be a directory, and therefore does not\n> + work for bundles (see linkgit:git-bundle[1]).\n\nHmm, I do not think this is quite true. \n\nIf \"git clone /path/to/repo.bndl\" implied --local, we would end up\ntrying to hardlink into /path/to/repo.bndl/objects and would fail.\n\nI think what is closer to the reaility is that we check if the\nsource is a bundle when the local filesystem path is used and try to\nclone from the bundle, before using the local filesystem path as a\ndirectory we can \"clone --local\" from. On the other hand, when the\n:// syntax is used, we do not even bother seeing if\nthe named resource is a bundle, or if --local optimization is\npossible (because we do not bother seeing if the named resource is a\nlocal filesystem entity, either).\n\nA possibly interesting tangent to think about is what would happen\nif we slightly tweak the above design. What it would require for\nthe code to take \"git clone https://site/repo.bndl\", realize that\nthe named resource is a bundle file, curl/wget it and clone from\nthat downloaded bundle? And if it is feasible to implement, would\nit even be a good idea to begin with? I do not have a ready answer\nto either of these questions myself.\n"},{"id":"375401","messageId":"20190513165140.vq225fscfvemol7k@x220.qyliss.net","threadId":"51077","inReplyTo":"xmqqv9yebwzp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] doc: explain why file: URLs and bundles don't mix","fromName":"Alyssa Ross","fromEmail":"hi@alyssa.is","sentAt":"2019-05-13T16:54:55Z","receivedAt":"2019-05-13T16:55:02Z","isPatch":true,"sender":{"key":"hi@alyssa.is","avatar":"https://avatars.githubusercontent.com/u/2768870?v=4"},"body":"> > +- When cloning, the former implies --local option. See\n> > + linkgit:git-clone[1] for details.\n> > +\n> > +- The latter is implemented using linkgit:git-upload-pack[1], which\n> > + expects its repository to be a directory, and therefore does not\n> > + work for bundles (see linkgit:git-bundle[1]).\n>\n> Hmm, I do not think this is quite true.\n>\n> If \"git clone /path/to/repo.bndl\" implied --local, we would end up\n> trying to hardlink into /path/to/repo.bndl/objects and would fail.\n>\n> I think what is closer to the reaility is that we check if the\n> source is a bundle when the local filesystem path is used and try to\n> clone from the bundle, before using the local filesystem path as a\n> directory we can \"clone --local\" from. On the other hand, when the\n> :// syntax is used, we do not even bother seeing if\n> the named resource is a bundle, or if --local optimization is\n> possible (because we do not bother seeing if the named resource is a\n> local filesystem entity, either).\n\nWould the following work better for you?\n\nWhen cloning, the former will check to see if the source is a bundle.\nIf it is, it will clone from the bundle, otherwise it will behave as if\ngiven --local. The latter performs neither of these checks, and\ntherefore does not support bundles.\n\n> A possibly interesting tangent to think about is what would happen\n> if we slightly tweak the above design. What it would require for\n> the code to take \"git clone https://site/repo.bndl\", realize that\n> the named resource is a bundle file, curl/wget it and clone from\n> that downloaded bundle? And if it is feasible to implement, would\n> it even be a good idea to begin with? I do not have a ready answer\n> to either of these questions myself.\n\nI was very surprised to find that cloning from a bundle didn't even work\nwith a file URL, especially since the documentation seemed to imply that\nit should. I assumed that file URLs not supporting bundles was an\noversight, and made some attempt to read the code with a view to fixing\nit. Once I realised that file URLs went through a completely different\ncode path to paths, I decided it was better to just document things as\nthey were.\n\nI think it would be nice if, at least for file URLs, bundles were\nsupported. I doubt supporting cloning a bundle over a network would\ninherently cause problems (although I'm by no means qualified to make\nsuch an assessment) -- if it's not clear whether it's a good idea to do\nso, it could always be implemented without being advertised.\n"},{"id":"375485","messageId":"20190514092900.GA11679@sigill.intra.peff.net","threadId":"51077","inReplyTo":"xmqqv9yebwzp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] doc: explain why file: URLs and bundles don't mix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T09:29:01Z","receivedAt":"2019-05-14T09:29:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 13, 2019 at 05:05:14PM +0900, Junio C Hamano wrote:\n\n> I think what is closer to the reaility is that we check if the\n> source is a bundle when the local filesystem path is used and try to\n> clone from the bundle, before using the local filesystem path as a\n> directory we can \"clone --local\" from. On the other hand, when the\n> :// syntax is used, we do not even bother seeing if\n> the named resource is a bundle, or if --local optimization is\n> possible (because we do not bother seeing if the named resource is a\n> local filesystem entity, either).\n\nYeah. My concern on reading Alyssa's patch is that it goes into too much\ndetail about \"why\", when this is basically just \"it's a bug that nobody\nhas yet fixed\". I think it makes sense to warn people about the\nbehavior, but we should probably be vague about the reasons and up-front\nthat there is really no good reason that it doesn't work (and it might\nin the future).\n\nOf course fixing the issue is better still. ;)\n\n> A possibly interesting tangent to think about is what would happen\n> if we slightly tweak the above design. What it would require for\n> the code to take \"git clone https://site/repo.bndl\", realize that\n> the named resource is a bundle file, curl/wget it and clone from\n> that downloaded bundle? And if it is feasible to implement, would\n> it even be a good idea to begin with? I do not have a ready answer\n> to either of these questions myself.\n\nI've been meaning to come back to this for, oh, going on 8 years now:\n\n https://public-inbox.org/git/20111110074330.GA27925@sigill.intra.peff.net/\n\n(though it was meant to work with a CDN \"try this first then top-off\"\nstrategy; with Jonathan Tan's more flexible CDN work which uses raw\npackfiles, I doubt I'd come back to this).\n\n-Peff\n"},{"id":"375486","messageId":"20190514093023.GB11679@sigill.intra.peff.net","threadId":"51077","inReplyTo":"20190513165140.vq225fscfvemol7k@x220.qyliss.net","subject":"Re: [PATCH] doc: explain why file: URLs and bundles don't mix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T09:30:23Z","receivedAt":"2019-05-14T09:30:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 13, 2019 at 04:54:55PM +0000, Alyssa Ross wrote:\n\n> > I think what is closer to the reaility is that we check if the\n> > source is a bundle when the local filesystem path is used and try to\n> > clone from the bundle, before using the local filesystem path as a\n> > directory we can \"clone --local\" from. On the other hand, when the\n> > :// syntax is used, we do not even bother seeing if\n> > the named resource is a bundle, or if --local optimization is\n> > possible (because we do not bother seeing if the named resource is a\n> > local filesystem entity, either).\n> \n> Would the following work better for you?\n> \n> When cloning, the former will check to see if the source is a bundle.\n> If it is, it will clone from the bundle, otherwise it will behave as if\n> given --local. The latter performs neither of these checks, and\n> therefore does not support bundles.\n\nFWIW, I like that much better than the original proposal. I think it\nwould also be OK to say something like: ...does not support bundles (but\nmay in the future).\n\n-Peff\n"}]}