{"thread":{"id":"26531","subject":"git clone NonExistentLocation","startedAt":"2011-02-17T09:01:27Z","lastAt":"2011-02-18T10:16:38Z","messageCount":10,"participants":["Stefan Naewe","Michael J Gruber","Sverre Rabbelier","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"161394","messageId":"4D5CE3E7.5030101@atlas-elektronik.com","threadId":"26531","inReplyTo":null,"subject":"git clone NonExistentLocation","fromName":"Stefan Naewe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2011-02-17T09:01:27Z","receivedAt":"2011-02-17T09:01:27Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"Hi.\n\nIf I do:\n\n $ uname -a\nLinux as100897 2.6.26-2-686 #1 SMP Thu Nov 25 01:53:57 UTC 2010 i686 GNU/Linux\n $ git version\ngit version 1.7.4.1\n $ ls -l NonExistentLocation\nls: cannot access NonExistentLocation: No such file or directory\n $ git clone NonExistentLocation\nCloning into NonExistentLocation...\nwarning: You appear to have cloned an empty repository.\n $\n\nI get a new (empty) git repository in 'NonExistentLocation':\n\n $ tree -a NonExistentLocation\nNonExistentLocation\n`-- .git\n    |-- HEAD\n    |-- branches\n    |-- config\n    |-- description\n    |-- hooks\n    |   |-- applypatch-msg.sample\n    |   |-- commit-msg.sample\n    |   |-- post-commit.sample\n    |   |-- post-receive.sample\n    |   |-- post-update.sample\n    |   |-- pre-applypatch.sample\n    |   |-- pre-commit.sample\n    |   |-- pre-rebase.sample\n    |   |-- prepare-commit-msg.sample\n    |   `-- update.sample\n    |-- info\n    |   `-- exclude\n    |-- objects\n    |   |-- info\n    |   `-- pack\n    `-- refs\n        |-- heads\n        `-- tags\n\n10 directories, 14 files\n\nIs this the intended behaviour ?\n\nThanks,\n  Stefan\n-- \n----------------------------------------------------------------\n/dev/random says: An ounce of application is worth a ton of abstraction.\n"},{"id":"161415","messageId":"4D5D1715.5020707@drmicha.warpmail.net","threadId":"26531","inReplyTo":"4D5CE3E7.5030101@atlas-elektronik.com","subject":"Re: git clone NonExistentLocation","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-02-17T12:39:49Z","receivedAt":"2011-02-17T12:39:49Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Stefan Naewe venit, vidit, dixit 17.02.2011 10:01:\n> Hi.\n> \n> If I do:\n> \n>  $ uname -a\n> Linux as100897 2.6.26-2-686 #1 SMP Thu Nov 25 01:53:57 UTC 2010 i686 GNU/Linux\n>  $ git version\n> git version 1.7.4.1\n>  $ ls -l NonExistentLocation\n> ls: cannot access NonExistentLocation: No such file or directory\n>  $ git clone NonExistentLocation\n> Cloning into NonExistentLocation...\n> warning: You appear to have cloned an empty repository.\n>  $\n> \n> I get a new (empty) git repository in 'NonExistentLocation':\n> \n>  $ tree -a NonExistentLocation\n> NonExistentLocation\n> `-- .git\n>     |-- HEAD\n>     |-- branches\n>     |-- config\n>     |-- description\n>     |-- hooks\n>     |   |-- applypatch-msg.sample\n>     |   |-- commit-msg.sample\n>     |   |-- post-commit.sample\n>     |   |-- post-receive.sample\n>     |   |-- post-update.sample\n>     |   |-- pre-applypatch.sample\n>     |   |-- pre-commit.sample\n>     |   |-- pre-rebase.sample\n>     |   |-- prepare-commit-msg.sample\n>     |   `-- update.sample\n>     |-- info\n>     |   `-- exclude\n>     |-- objects\n>     |   |-- info\n>     |   `-- pack\n>     `-- refs\n>         |-- heads\n>         `-- tags\n> \n> 10 directories, 14 files\n> \n> Is this the intended behaviour ?\n> \n> Thanks,\n>   Stefan\n\nIt is useful, and it even gives you a warning that it still might not be\nwhat you intended. Would be funny if it were accidental. Indeed, a git\n\"log -S\" on that warning reveals that it was introduced intentionally in\n\n86ac751 (Allow cloning an empty repository, 2009-01-23)\n\nMichael\n"},{"id":"161417","messageId":"4D5D1A04.4090107@atlas-elektronik.com","threadId":"26531","inReplyTo":"4D5D1715.5020707@drmicha.warpmail.net","subject":"Re: git clone NonExistentLocation","fromName":"Stefan Naewe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2011-02-17T12:52:20Z","receivedAt":"2011-02-17T12:52:20Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"On 2/17/2011 1:39 PM, Michael J Gruber wrote:\n> \n> It is useful, and it even gives you a warning that it still might not be\n> what you intended. Would be funny if it were accidental. Indeed, a git\n> \"log -S\" on that warning reveals that it was introduced intentionally in\n> \n> 86ac751 (Allow cloning an empty repository, 2009-01-23)\n\nOK. But that's about 'cloning an empty repository'.\n'NonExistentLocation' is not empty initially - it simply does\nnot exist.\n\nContrast that to 'git clone http://url.does.not.exist'. You don't\nget an empty repository in 'url.does.not.exist' after running that.\n\nRegards,\n  Stefan\n-- \n----------------------------------------------------------------\n/dev/random says: ==/==/==/==Police tagline==/==/==Do not cross ==/==/==/==\n"},{"id":"161418","messageId":"4D5D1BCB.3010003@drmicha.warpmail.net","threadId":"26531","inReplyTo":"4D5D1A04.4090107@atlas-elektronik.com","subject":"Re: git clone NonExistentLocation","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-02-17T12:59:55Z","receivedAt":"2011-02-17T12:59:55Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Stefan Naewe venit, vidit, dixit 17.02.2011 13:52:\n> On 2/17/2011 1:39 PM, Michael J Gruber wrote:\n>>\n>> It is useful, and it even gives you a warning that it still might not be\n>> what you intended. Would be funny if it were accidental. Indeed, a git\n>> \"log -S\" on that warning reveals that it was introduced intentionally in\n>>\n>> 86ac751 (Allow cloning an empty repository, 2009-01-23)\n> \n> OK. But that's about 'cloning an empty repository'.\n> 'NonExistentLocation' is not empty initially - it simply does\n> not exist.\n> \n> Contrast that to 'git clone http://url.does.not.exist'. You don't\n> get an empty repository in 'url.does.not.exist' after running that.\n\nOK, the transport layer errors out in that case.\n\nRereading Sverre's commit message, I'm still not sure whether this case\nwas intended or not. The test case covers existing empty repos only. So\nI'm cc'ing him and holding back by documentation patch.\n\nSverre, with your 86ac751, the following two are equivalent (modulo a\nwarning) on a nonexisting dir:\n\ngit clone dir\ngit init dir\n\nIs that intentional?\n\nMichael\n"},{"id":"161422","messageId":"AANLkTin3Pf4XD_gbwxPzW4uff9SbDWJdGug6jM_SHDo+@mail.gmail.com","threadId":"26531","inReplyTo":"4D5D1BCB.3010003@drmicha.warpmail.net","subject":"Re: git clone NonExistentLocation","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-02-17T14:03:29Z","receivedAt":"2011-02-17T14:03:29Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\n[Thanks for summarizing.]\n\nOn Thu, Feb 17, 2011 at 12:59, Michael J Gruber\n<git@drmicha.warpmail.net> wrote:\n> Sverre, with your 86ac751, the following two are equivalent (modulo a\n> warning) on a nonexisting dir:\n>\n> git clone dir\n> git init dir\n>\n> Is that intentional?\n\nNo, that was not intentional. The former should still be an error if\n'dir' is an empy directory.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"161429","messageId":"4D5D5275.5070501@drmicha.warpmail.net","threadId":"26531","inReplyTo":"AANLkTin3Pf4XD_gbwxPzW4uff9SbDWJdGug6jM_SHDo+@mail.gmail.com","subject":"Re: git clone NonExistentLocation","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-02-17T16:53:09Z","receivedAt":"2011-02-17T16:53:09Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Sverre Rabbelier venit, vidit, dixit 17.02.2011 15:03:\n> Heya,\n> \n> [Thanks for summarizing.]\n> \n> On Thu, Feb 17, 2011 at 12:59, Michael J Gruber\n> <git@drmicha.warpmail.net> wrote:\n>> Sverre, with your 86ac751, the following two are equivalent (modulo a\n>> warning) on a nonexisting dir:\n>>\n>> git clone dir\n>> git init dir\n>>\n>> Is that intentional?\n> \n> No, that was not intentional. The former should still be an error if\n> 'dir' is an empy directory.\n> \n\nDigging a little further: since a nonexisting directory is neither a dir\nnor a file, clone thinks it is not local (is_local=is_bundle=0). None of\nthe transport_* commands error out since the relevant one is guarded by\n86ac751...\n\nReverting that or forcing is_local=1 both help, but how to detect \"local\nnonexisting\" reliably?\n\nIn fact, I don't have a problem with the current state if we document it :)\n\nMichael\n"},{"id":"161496","messageId":"20110218040152.GA25466@sigill.intra.peff.net","threadId":"26531","inReplyTo":"4D5D5275.5070501@drmicha.warpmail.net","subject":"Re: git clone NonExistentLocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-18T04:01:52Z","receivedAt":"2011-02-18T04:01:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 17, 2011 at 05:53:09PM +0100, Michael J Gruber wrote:\n\n> Digging a little further: since a nonexisting directory is neither a dir\n> nor a file, clone thinks it is not local (is_local=is_bundle=0). None of\n> the transport_* commands error out since the relevant one is guarded by\n> 86ac751...\n> \n> Reverting that or forcing is_local=1 both help, but how to detect \"local\n> nonexisting\" reliably?\n> \n> In fact, I don't have a problem with the current state if we document it :)\n\nHmm, the current behavior is even weirder. This clones an empty\nrepository:\n\n  git clone does-not-exist\n\nbut this causes an error:\n\n  git clone does-not-exist new-dir\n\nRegardless, I think we should catch this error, as it is likely not\nwhat the user intended. Yes, there's a warning, but I just don't see in\nwhat circumstance this behavior would be useful, and the downside is\nthat you may have failed to actually create a copy of your data, which\ncould lead to data loss.\n\nI think the patch below is the right fix.\n\n-- >8 --\nSubject: [PATCH] clone: die when trying to clone missing local path\n\nSince 86ac751 (Allow cloning an empty repository,\n2009-01-23), doing:\n\n  git clone does-not-exist\n\nhas created does-not-exist as an empty repository. This was\nan unintentional side effect of 86ac751. Even weirder,\ndoing:\n\n  git clone does-not-exist new-dir\n\n_does_ fail, making this \"feature\" (if you want to consider\nit such) broken. Let's detect this situation and explicitly\ndie. It's almost certainly not what the user intended.\n\nThis patch also adds two tests. One for the missing path\ncase, and one to confirm that a similar case, cloning a\nnon-repository directory, fails.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c        |    5 ++++-\n t/t5701-clone-local.sh |   13 +++++++++++++\n 2 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 60d9a64..55785d0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -412,8 +412,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tpath = get_repo_path(repo_name, &is_bundle);\n \tif (path)\n \t\trepo = xstrdup(make_nonrelative_path(repo_name));\n-\telse if (!strchr(repo_name, ':'))\n+\telse if (!strchr(repo_name, ':')) {\n+\t\tif (!file_exists(repo_name))\n+\t\t\tdie(\"repository '%s' does not exist\", repo_name);\n \t\trepo = xstrdup(make_absolute_path(repo_name));\n+\t}\n \telse\n \t\trepo = repo_name;\n \tis_local = path && !is_bundle;\ndiff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh\nindex 0f4d487..6972258 100755\n--- a/t/t5701-clone-local.sh\n+++ b/t/t5701-clone-local.sh\n@@ -144,4 +144,17 @@ test_expect_success 'clone empty repository, and then push should not segfault.'\n \ttest_must_fail git push)\n '\n \n+test_expect_success 'cloning non-existent directory fails' '\n+\tcd \"$D\" &&\n+\trm -rf does-not-exist &&\n+\ttest_must_fail git clone does-not-exist\n+'\n+\n+test_expect_success 'cloning non-git directory fails' '\n+\tcd \"$D\" &&\n+\trm -rf not-a-git-repo not-a-git-repo-clone &&\n+\tmkdir not-a-git-repo &&\n+\ttest_must_fail git clone not-a-git-repo not-a-git-repo-clone\n+'\n+\n test_done\n-- \n1.7.4.1.3.g720b9\n"},{"id":"161511","messageId":"7vd3mp6ftp.fsf@alter.siamese.dyndns.org","threadId":"26531","inReplyTo":"20110218040152.GA25466@sigill.intra.peff.net","subject":"Re: git clone NonExistentLocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-18T06:02:10Z","receivedAt":"2011-02-18T06:02:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think the patch below is the right fix.\n> ...\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/clone.c        |    5 ++++-\n>  t/t5701-clone-local.sh |   13 +++++++++++++\n>  2 files changed, 17 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 60d9a64..55785d0 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -412,8 +412,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tpath = get_repo_path(repo_name, &is_bundle);\n>  \tif (path)\n>  \t\trepo = xstrdup(make_nonrelative_path(repo_name));\n> -\telse if (!strchr(repo_name, ':'))\n> +\telse if (!strchr(repo_name, ':')) {\n> +\t\tif (!file_exists(repo_name))\n> +\t\t\tdie(\"repository '%s' does not exist\", repo_name);\n>  \t\trepo = xstrdup(make_absolute_path(repo_name));\n> +\t}\n>  \telse\n>  \t\trepo = repo_name;\n>  \tis_local = path && !is_bundle;\n\nThanks, but I am confused.\n\nThe stuff goes through make_absolute_path() so we must be certain that\nthis has to be a local filesystem entity _if_ it is a repository.\n\nBut when will we see a file at repo_name in this new codepath?  In what\nsituation would get_repo_path(repo_name, &is_bundle) return NULL but the\nadded file_exists(repo_name) would yield true to bypess your die()?\n\nIf repo_name is a directory, or a regular file, get_repo_path() would give\nus a relative path to it, no?\n\nIOW, wouldn't the fix be more like this?  Your new test script seems to\npass with this.\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 60d9a64..2ee1fa9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -413,7 +413,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (path)\n \t\trepo = xstrdup(make_nonrelative_path(repo_name));\n \telse if (!strchr(repo_name, ':'))\n-\t\trepo = xstrdup(make_absolute_path(repo_name));\n+\t\tdie(\"repository '%s' does not exist\", repo_name);\n \telse\n \t\trepo = repo_name;\n \tis_local = path && !is_bundle;\n"},{"id":"161512","messageId":"20110218061133.GA25847@sigill.intra.peff.net","threadId":"26531","inReplyTo":"7vd3mp6ftp.fsf@alter.siamese.dyndns.org","subject":"Re: git clone NonExistentLocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-18T06:11:33Z","receivedAt":"2011-02-18T06:11:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 17, 2011 at 10:02:10PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I think the patch below is the right fix.\n> > ...\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> >  builtin/clone.c        |    5 ++++-\n> >  t/t5701-clone-local.sh |   13 +++++++++++++\n> >  2 files changed, 17 insertions(+), 1 deletions(-)\n> >\n> > diff --git a/builtin/clone.c b/builtin/clone.c\n> > index 60d9a64..55785d0 100644\n> > --- a/builtin/clone.c\n> > +++ b/builtin/clone.c\n> > @@ -412,8 +412,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> >  \tpath = get_repo_path(repo_name, &is_bundle);\n> >  \tif (path)\n> >  \t\trepo = xstrdup(make_nonrelative_path(repo_name));\n> > -\telse if (!strchr(repo_name, ':'))\n> > +\telse if (!strchr(repo_name, ':')) {\n> > +\t\tif (!file_exists(repo_name))\n> > +\t\t\tdie(\"repository '%s' does not exist\", repo_name);\n> >  \t\trepo = xstrdup(make_absolute_path(repo_name));\n> > +\t}\n> >  \telse\n> >  \t\trepo = repo_name;\n> >  \tis_local = path && !is_bundle;\n> \n> Thanks, but I am confused.\n> \n> The stuff goes through make_absolute_path() so we must be certain that\n> this has to be a local filesystem entity _if_ it is a repository.\n> \n> But when will we see a file at repo_name in this new codepath?  In what\n> situation would get_repo_path(repo_name, &is_bundle) return NULL but the\n> added file_exists(repo_name) would yield true to bypess your die()?\n\nHmm, good point. I didn't look carefully enough into get_repo_path. It\nis not really about finding a repo, but rather about finding a directory\nor filename. Plus, my file_exists() isn't quite right, anyway. We really\nwant to make sure $repo_name.git, $repo_name.bundle, etc don't exist.\nBut get_repo_path has already done that for us, so we should just trust\nits output.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 60d9a64..2ee1fa9 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -413,7 +413,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tif (path)\n>  \t\trepo = xstrdup(make_nonrelative_path(repo_name));\n>  \telse if (!strchr(repo_name, ':'))\n> -\t\trepo = xstrdup(make_absolute_path(repo_name));\n> +\t\tdie(\"repository '%s' does not exist\", repo_name);\n>  \telse\n>  \t\trepo = repo_name;\n>  \tis_local = path && !is_bundle;\n\nYeah, I think that is a better fix.\n\n-Peff\n"},{"id":"161528","messageId":"AANLkTinAALcC4O2atwAbYfz8BUnwO+z9E9b48mkVAMjj@mail.gmail.com","threadId":"26531","inReplyTo":"7vd3mp6ftp.fsf@alter.siamese.dyndns.org","subject":"Re: git clone NonExistentLocation","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-02-18T10:16:38Z","receivedAt":"2011-02-18T10:16:38Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Fri, Feb 18, 2011 at 07:02, Junio C Hamano <gitster@pobox.com> wrote:\n> IOW, wouldn't the fix be more like this?  Your new test script seems to\n> pass with this.\n\nThanks for fixing both.\n\n-- \nCheers,\n\nSverre Rabbelier\n"}]}