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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> 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 ':'?

> -	/*
> -	 * 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--;
 
 	/*