Re: [PATCH v2 5/6] clone: fix hostname parsing when guessing dir

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2015, 17:42 UTC
Message-ID: <xmqq7fpiamiq.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1438185076-28870-6-git-send-email-ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 6 quoted lines

> We fail to guess a sensible directory name for a newly cloned
> repository when the path component of the URL is empty. E.g.
> cloning a repository 'ssh://user:password@example.com/' we create
> a directory 'password@example.com' for the clone.
>
> Fix this by ...

It is clear that you do not want to have that password in the resulting directory name from the problem description, but you started saying "Fix this" without saying what the desired outcome is. "We want to use only the hostname, e.g. 'example.com', in such a case instead." or something, perhaps, at the end of the first paragraph? "Fix this by doing such and such" becomes understandable only after we know what end result you want to achieve by "doing such and such".

> ... using parse_connect_url to split host and path
> components and explicitly checking whether we need to fall back
> to the hostname for guessing a directory name.

I cannot help wonder why this much change (including patches 3 and
4) is needed.  Isn't it just the matter of making this part of the
existing code be aware of '@' in addition to ':'?

Show 8 quoted lines

> -	/*
> -	 * Find last component, but be prepared that repo could have
> -	 * the form  "remote.example.com:foo.git", i.e. no slash
> -	 * in the directory part.
> -	 */
> -	start = end;
> -	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
> -		start--;

Regardless of the issue you are trying to address, we may want to limit that "be prepared for and careful with ':'" logic in the existing code to the case where the "last component" does not have any other component before it. That is:

	http://example.com/foo:bar.git/

would be stripped to

	http://example.com/foo:bar

and then we scan backwards for ':' or '/' and declare that "bar" is the name of the repository, but we would probably want "foo:bar" instead (or we may not, as some filesystems do not want to have a colon in its path components).

 builtin/clone.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a72ff7e..3d6328a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -165,10 +165,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	/*
 	 * Find last component, but be prepared that repo could have
 	 * the form  "remote.example.com:foo.git", i.e. no slash
-	 * in the directory part.
+	 * in the directory part, or "http://user@example.com/" with
+	 * the whole site serving a single repository.
 	 */
 	start = end;
-	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
+	while (repo < start &&
+	       !(is_dir_sep(start[-1]) || start[-1] == ':' || start[-1] == '@'))
 		start--;
 
 	/*