git clone NonExistentLocation

Hi.

If I do:

 $ uname -a
Linux as100897 2.6.26-2-686 #1 SMP Thu Nov 25 01:53:57 UTC 2010 i686 GNU/Linux
 $ git version
git version 1.7.4.1
 $ ls -l NonExistentLocation
ls: cannot access NonExistentLocation: No such file or directory
 $ git clone NonExistentLocation
Cloning into NonExistentLocation...
warning: You appear to have cloned an empty repository.
 $

I get a new (empty) git repository in 'NonExistentLocation':

 $ tree -a NonExistentLocation
NonExistentLocation
`-- .git
    |-- HEAD
    |-- branches
    |-- config
    |-- description
    |-- hooks
    |   |-- applypatch-msg.sample
    |   |-- commit-msg.sample
    |   |-- post-commit.sample
    |   |-- post-receive.sample
    |   |-- post-update.sample
    |   |-- pre-applypatch.sample
    |   |-- pre-commit.sample
    |   |-- pre-rebase.sample
    |   |-- prepare-commit-msg.sample
    |   `-- update.sample
    |-- info
    |   `-- exclude
    |-- objects
    |   |-- info
    |   `-- pack
    `-- refs
        |-- heads
        `-- tags

10 directories, 14 files

Is this the intended behaviour ?

Thanks,
  Stefan
-- 
----------------------------------------------------------------
/dev/random says: An ounce of application is worth a ton of abstraction.

Stefan Naewe venit, vidit, dixit 17.02.2011 10:01:

Hi.

If I do:

 $ uname -a
Linux as100897 2.6.26-2-686 #1 SMP Thu Nov 25 01:53:57 UTC 2010 i686 GNU/Linux
 $ git version
git version 1.7.4.1
 $ ls -l NonExistentLocation
ls: cannot access NonExistentLocation: No such file or directory
 $ git clone NonExistentLocation
Cloning into NonExistentLocation...
warning: You appear to have cloned an empty repository.
 $

I get a new (empty) git repository in 'NonExistentLocation':

 $ tree -a NonExistentLocation
NonExistentLocation
`-- .git
    |-- HEAD
    |-- branches
    |-- config
    |-- description
    |-- hooks
    |   |-- applypatch-msg.sample
    |   |-- commit-msg.sample
    |   |-- post-commit.sample
    |   |-- post-receive.sample
    |   |-- post-update.sample
    |   |-- pre-applypatch.sample
    |   |-- pre-commit.sample
    |   |-- pre-rebase.sample
    |   |-- prepare-commit-msg.sample
    |   `-- update.sample
    |-- info
    |   `-- exclude
    |-- objects
    |   |-- info
    |   `-- pack
    `-- refs
        |-- heads
        `-- tags

10 directories, 14 files

Is this the intended behaviour ?

Thanks,
  Stefan

It is useful, and it even gives you a warning that it still might not be
what you intended. Would be funny if it were accidental. Indeed, a git
"log -S" on that warning reveals that it was introduced intentionally in

86ac751 (Allow cloning an empty repository, 2009-01-23)

Michael

On 2/17/2011 1:39 PM, Michael J Gruber wrote:

It is useful, and it even gives you a warning that it still might not be
what you intended. Would be funny if it were accidental. Indeed, a git
"log -S" on that warning reveals that it was introduced intentionally in

86ac751 (Allow cloning an empty repository, 2009-01-23)

OK. But that's about 'cloning an empty repository'.
'NonExistentLocation' is not empty initially - it simply does
not exist.

Contrast that to 'git clone http://url.does.not.exist'. You don't
get an empty repository in 'url.does.not.exist' after running that.

Regards,
  Stefan
-- 
----------------------------------------------------------------
/dev/random says: ==/==/==/==Police tagline==/==/==Do not cross ==/==/==/==

Stefan Naewe venit, vidit, dixit 17.02.2011 13:52:

On 2/17/2011 1:39 PM, Michael J Gruber wrote:

quoted

It is useful, and it even gives you a warning that it still might not be
what you intended. Would be funny if it were accidental. Indeed, a git
"log -S" on that warning reveals that it was introduced intentionally in

86ac751 (Allow cloning an empty repository, 2009-01-23)

OK. But that's about 'cloning an empty repository'.
'NonExistentLocation' is not empty initially - it simply does
not exist.

Contrast that to 'git clone http://url.does.not.exist'. You don't
get an empty repository in 'url.does.not.exist' after running that.

OK, the transport layer errors out in that case.

Rereading Sverre's commit message, I'm still not sure whether this case
was intended or not. The test case covers existing empty repos only. So
I'm cc'ing him and holding back by documentation patch.

Sverre, with your 86ac751, the following two are equivalent (modulo a
warning) on a nonexisting dir:

git clone dir
git init dir

Is that intentional?

Michael

Heya,

[Thanks for summarizing.]

On Thu, Feb 17, 2011 at 12:59, Michael J Gruber
[off-list ref] wrote:

Sverre, with your 86ac751, the following two are equivalent (modulo a
warning) on a nonexisting dir:

git clone dir
git init dir

Is that intentional?

No, that was not intentional. The former should still be an error if
'dir' is an empy directory.

-- 
Cheers,

Sverre Rabbelier

Sverre Rabbelier venit, vidit, dixit 17.02.2011 15:03:

Heya,

[Thanks for summarizing.]

On Thu, Feb 17, 2011 at 12:59, Michael J Gruber
[off-list ref] wrote:

quoted

Sverre, with your 86ac751, the following two are equivalent (modulo a
warning) on a nonexisting dir:

git clone dir
git init dir

Is that intentional?

No, that was not intentional. The former should still be an error if
'dir' is an empy directory.

Digging a little further: since a nonexisting directory is neither a dir
nor a file, clone thinks it is not local (is_local=is_bundle=0). None of
the transport_* commands error out since the relevant one is guarded by
86ac751...

Reverting that or forcing is_local=1 both help, but how to detect "local
nonexisting" reliably?

In fact, I don't have a problem with the current state if we document it :)

Michael

On Thu, Feb 17, 2011 at 05:53:09PM +0100, Michael J Gruber wrote:

Digging a little further: since a nonexisting directory is neither a dir
nor a file, clone thinks it is not local (is_local=is_bundle=0). None of
the transport_* commands error out since the relevant one is guarded by
86ac751...

Reverting that or forcing is_local=1 both help, but how to detect "local
nonexisting" reliably?

In fact, I don't have a problem with the current state if we document it :)

Hmm, the current behavior is even weirder. This clones an empty
repository:

  git clone does-not-exist

but this causes an error:

  git clone does-not-exist new-dir

Regardless, I think we should catch this error, as it is likely not
what the user intended. Yes, there's a warning, but I just don't see in
what circumstance this behavior would be useful, and the downside is
that you may have failed to actually create a copy of your data, which
could lead to data loss.

I think the patch below is the right fix.

-- >8 --
Subject: [PATCH] clone: die when trying to clone missing local path

Since 86ac751 (Allow cloning an empty repository,
2009-01-23), doing:

  git clone does-not-exist

has created does-not-exist as an empty repository. This was
an unintentional side effect of 86ac751. Even weirder,
doing:

  git clone does-not-exist new-dir

_does_ fail, making this "feature" (if you want to consider
it such) broken. Let's detect this situation and explicitly
die. It's almost certainly not what the user intended.

This patch also adds two tests. One for the missing path
case, and one to confirm that a similar case, cloning a
non-repository directory, fails.

Signed-off-by: Jeff King <redacted>
---
 builtin/clone.c        |    5 ++++-
 t/t5701-clone-local.sh |   13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 60d9a64..55785d0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -412,8 +412,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	path = get_repo_path(repo_name, &is_bundle);
 	if (path)
 		repo = xstrdup(make_nonrelative_path(repo_name));
-	else if (!strchr(repo_name, ':'))
+	else if (!strchr(repo_name, ':')) {
+		if (!file_exists(repo_name))
+			die("repository '%s' does not exist", repo_name);
 		repo = xstrdup(make_absolute_path(repo_name));
+	}
 	else
 		repo = repo_name;
 	is_local = path && !is_bundle;

diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index 0f4d487..6972258 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh

@@ -144,4 +144,17 @@ test_expect_success 'clone empty repository, and then push should not segfault.'
 	test_must_fail git push)
 '
 
+test_expect_success 'cloning non-existent directory fails' '
+	cd "$D" &&
+	rm -rf does-not-exist &&
+	test_must_fail git clone does-not-exist
+'
+
+test_expect_success 'cloning non-git directory fails' '
+	cd "$D" &&
+	rm -rf not-a-git-repo not-a-git-repo-clone &&
+	mkdir not-a-git-repo &&
+	test_must_fail git clone not-a-git-repo not-a-git-repo-clone
+'
+
 test_done

-- 
1.7.4.1.3.g720b9