Re: git clone NonExistentLocation

Jeff King [off-list ref] writes:

Thanks, but I am confused.

The stuff goes through make_absolute_path() so we must be certain that
this has to be a local filesystem entity _if_ it is a repository.

But when will we see a file at repo_name in this new codepath?  In what
situation would get_repo_path(repo_name, &is_bundle) return NULL but the
added file_exists(repo_name) would yield true to bypess your die()?

If repo_name is a directory, or a regular file, get_repo_path() would give
us a relative path to it, no?

IOW, wouldn't the fix be more like this?  Your new test script seems to
pass with this.

diff --git a/builtin/clone.c b/builtin/clone.c
index 60d9a64..2ee1fa9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

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

On Thu, Feb 17, 2011 at 10:02:10PM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

I think the patch below is the right fix.
...
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;

Thanks, but I am confused.

The stuff goes through make_absolute_path() so we must be certain that
this has to be a local filesystem entity _if_ it is a repository.

But when will we see a file at repo_name in this new codepath?  In what
situation would get_repo_path(repo_name, &is_bundle) return NULL but the
added file_exists(repo_name) would yield true to bypess your die()?

Hmm, good point. I didn't look carefully enough into get_repo_path. It
is not really about finding a repo, but rather about finding a directory
or filename. Plus, my file_exists() isn't quite right, anyway. We really
want to make sure $repo_name.git, $repo_name.bundle, etc don't exist.
But get_repo_path has already done that for us, so we should just trust
its output.

Yeah, I think that is a better fix.

-Peff

Heya,

On Fri, Feb 18, 2011 at 07:02, Junio C Hamano [off-list ref] wrote:

IOW, wouldn't the fix be more like this?  Your new test script seems to
pass with this.

Thanks for fixing both.

-- 
Cheers,

Sverre Rabbelier