Re: git clone NonExistentLocation

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2011, 06:02 UTC
Message-ID: <7vd3mp6ftp.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110218040152.GA25466@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 25 quoted lines

> I think the patch below is the right fix.
> ...
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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()?

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;