[PATCH] clone: fix repo name when cloning a server's root

[PATCH] clone: fix repo name when cloning a server's root

[Patrick Steinhardt]

When cloning a repository from a server's root, that is the URL's
path component is a '/' only, we fail to generate a sensible
repository name when the URL contains authentication data. This
is especially bad when cloning URLs like
'ssh://user:passwd@example.com/', which results in a repository
'passwd@example.com' being created.

Improve the behavior by also regarding '@'-signs as a separator
when scanning the URL. In the mentioned case this would instead
result in a directory 'example.com' being created.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
I was not able to come by with a useful test as that would
require being able to clone a root directory. I couldn't find
anything in the current tests that looks like what I want to do.
Does anybody have an idea on how to achieve this?

 builtin/clone.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a72ff7e..aaf38b2 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -164,11 +164,13 @@ 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.
+	 * the form  "remote.example.com:foo.git", i.e. no slash in
+	 * the directory part, or "user:password@remote.example.com/",
+	 * that is the path component may be empty.
 	 */
 	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--;
 
 	/*
-- 
2.4.6

Re: [PATCH] clone: fix repo name when cloning a server's root

[Duy Nguyen]

On Mon, Jul 27, 2015 at 6:48 PM, Patrick Steinhardt <ps@pks.im> wrote:
> When cloning a repository from a server's root, that is the URL's
> path component is a '/' only, we fail to generate a sensible
> repository name when the URL contains authentication data. This
> is especially bad when cloning URLs like
> 'ssh://user:passwd@example.com/', which results in a repository
> 'passwd@example.com' being created.
>
> Improve the behavior by also regarding '@'-signs as a separator
> when scanning the URL. In the mentioned case this would instead
> result in a directory 'example.com' being created.

My initial reaction was, if you put password on the command line, you
deserve it. However, as we improve this heuristics, perhaps it's
better to export parse_connect_url() from connect.c and use it here?
We would have more robust parsing. You can create a repo named
example.com given the url ssh://user:pass@example.com:123/. Maybe it's
overkill?

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> I was not able to come by with a useful test as that would
> require being able to clone a root directory. I couldn't find
> anything in the current tests that looks like what I want to do.
> Does anybody have an idea on how to achieve this?

There's t/t1509/prepare-chroot.sh that will prepare a chroot for this
purpose. You'll need linux, busybox and chroot permission.
-- 
Duy

Re: [PATCH] clone: fix repo name when cloning a server's root

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

On Mon, Jul 27, 2015 at 07:51:30PM +0700, Duy Nguyen wrote:
> On Mon, Jul 27, 2015 at 6:48 PM, Patrick Steinhardt <ps@pks.im> wrote:
> > When cloning a repository from a server's root, that is the URL's
> > path component is a '/' only, we fail to generate a sensible
> > repository name when the URL contains authentication data. This
> > is especially bad when cloning URLs like
> > 'ssh://user:passwd@example.com/', which results in a repository
> > 'passwd@example.com' being created.
> >
> > Improve the behavior by also regarding '@'-signs as a separator
> > when scanning the URL. In the mentioned case this would instead
> > result in a directory 'example.com' being created.
> 
> My initial reaction was, if you put password on the command line, you
> deserve it. However, as we improve this heuristics, perhaps it's
> better to export parse_connect_url() from connect.c and use it here?
> We would have more robust parsing. You can create a repo named
> example.com given the url ssh://user:pass@example.com:123/. Maybe it's
> overkill?

Sure, specifying passwords on command line should not be done
easily. Still those heuristics fail for everything that does
not include an additional [:/] when the URL's path is empty. So I
guess using parse_connect_url() would be the most sane solution
for this, as it will also fix the case when you specify a port,
which would currently use the port as directory name. I'll whip
up a new version that uses parse_connect_url().

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > I was not able to come by with a useful test as that would
> > require being able to clone a root directory. I couldn't find
> > anything in the current tests that looks like what I want to do.
> > Does anybody have an idea on how to achieve this?
> 
> There's t/t1509/prepare-chroot.sh that will prepare a chroot for this
> purpose. You'll need linux, busybox and chroot permission.

Thanks for the hint.

Patrick

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clone: fix repo name when cloning a server's root

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> I was not able to come by with a useful test as that would
>> require being able to clone a root directory. I couldn't find
>> anything in the current tests that looks like what I want to do.
>> Does anybody have an idea on how to achieve this?
>
> There's t/t1509/prepare-chroot.sh that will prepare a chroot for this
> purpose. You'll need linux, busybox and chroot permission.

If you do not use ssh:// or file://, it should be trivial to
arrange, no?  http://site/repo will typically not be serving
/repo from the root of the filesystem.

[PATCH v2 0/6] clone: fix repo name when cloning a server's root

[Patrick Steinhardt]

As proposed the previous patch has been changed such that we now
utilize parse_connect_url(), which splits a connect URL by host
and path components. If the path component is empty we now try to
fall back to the host component, stripping it of its port and
authentication information.

The first two patches fix the t1509-root-worktree tests. They
currently were not working due to broken &&-chains and are
required to test cloning from /.

The next two patches expose parse_connect_url(), which is then
used in the fifth patch. The last patch provides tests for the
behavior. To keep it simple we just use file:// URIs, which have
the option of including a host in there, e.g.
file://127.0.0.1/foobar.

Patrick Steinhardt (6):
  tests: fix broken && chains in t1509-root-worktree
  tests: fix cleanup after tests in t1509-root-worktree
  connect: expose parse_connect_url()
  connect: move error check to caller of parse_connect_url
  clone: fix hostname parsing when guessing dir
  clone: add tests for cloning / without workdir name

 builtin/clone.c          | 73 +++++++++++++++++++++++++++++++++++-------------
 connect.c                | 19 +++----------
 connect.h                | 13 +++++++++
 t/t1509-root-worktree.sh | 43 +++++++++++++++++++++++++---
 4 files changed, 109 insertions(+), 39 deletions(-)

-- 
2.5.0

[PATCH v2 1/6] tests: fix broken && chains in t1509-root-worktree

[Patrick Steinhardt]

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1509-root-worktree.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
index b6977d4..0c80129 100755
--- a/t/t1509-root-worktree.sh
+++ b/t/t1509-root-worktree.sh
@@ -125,7 +125,7 @@ fi
 ONE_SHA1=d00491fd7e5bb6fa28c517a0bb32b8b506539d4d
 
 test_expect_success 'setup' '
-	rm -rf /foo
+	rm -rf /foo &&
 	mkdir /foo &&
 	mkdir /foo/bar &&
 	echo 1 > /foo/foome &&
@@ -218,7 +218,7 @@ unset GIT_WORK_TREE
 
 test_expect_success 'go to /' 'cd /'
 test_expect_success 'setup' '
-	rm -rf /.git
+	rm -rf /.git &&
 	echo "Initialized empty Git repository in /.git/" > expected &&
 	git init > result &&
 	test_cmp expected result
@@ -241,8 +241,8 @@ say "auto bare gitdir"
 
 # DESTROYYYYY!!!!!
 test_expect_success 'setup' '
-	rm -rf /refs /objects /info /hooks
-	rm /*
+	rm -rf /refs /objects /info /hooks &&
+	rm /* &&
 	cd / &&
 	echo "Initialized empty Git repository in /" > expected &&
 	git init --bare > result &&
-- 
2.5.0

[PATCH v2 2/6] tests: fix cleanup after tests in t1509-root-worktree

[Patrick Steinhardt]

During cleanup we do a simple 'rm /*' to remove leftover files
from previous tests. As 'rm' errors out when there is anything it
cannot delete and there are directories present at '/' it will
throw an error, causing the '&&' chain to fail.

Fix this by explicitly removing the files.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1509-root-worktree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
index 0c80129..553a3f6 100755
--- a/t/t1509-root-worktree.sh
+++ b/t/t1509-root-worktree.sh
@@ -242,7 +242,7 @@ say "auto bare gitdir"
 # DESTROYYYYY!!!!!
 test_expect_success 'setup' '
 	rm -rf /refs /objects /info /hooks &&
-	rm /* &&
+	rm -f /expected /ls.expected /me /result &&
 	cd / &&
 	echo "Initialized empty Git repository in /" > expected &&
 	git init --bare > result &&
-- 
2.5.0

[PATCH v2 3/6] connect: expose parse_connect_url()

[Patrick Steinhardt]

Expose parse_connect_url which is to be used later in this patch
series.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 connect.c | 13 +------------
 connect.h | 13 +++++++++++++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/connect.c b/connect.c
index c0144d8..bdbcee4 100644
--- a/connect.c
+++ b/connect.c
@@ -228,13 +228,6 @@ int server_supports(const char *feature)
 	return !!server_feature_value(feature, NULL);
 }
 
-enum protocol {
-	PROTO_LOCAL = 1,
-	PROTO_FILE,
-	PROTO_SSH,
-	PROTO_GIT
-};
-
 int url_is_local_not_ssh(const char *url)
 {
 	const char *colon = strchr(url, ':');
@@ -580,11 +573,7 @@ static char *get_port(char *host)
 	return NULL;
 }
 
-/*
- * Extract protocol and relevant parts from the specified connection URL.
- * The caller must free() the returned strings.
- */
-static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
+enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 				       char **ret_path)
 {
 	char *url;
diff --git a/connect.h b/connect.h
index c41a685..245890f 100644
--- a/connect.h
+++ b/connect.h
@@ -11,4 +11,17 @@ extern int parse_feature_request(const char *features, const char *feature);
 extern const char *server_feature_value(const char *feature, int *len_ret);
 extern int url_is_local_not_ssh(const char *url);
 
+enum protocol {
+	PROTO_LOCAL = 1,
+	PROTO_FILE,
+	PROTO_SSH,
+	PROTO_GIT
+};
+
+/*
+ * Extract protocol and relevant parts from the specified connection URL.
+ * The caller must free() the returned strings.
+ */
+extern enum protocol parse_connect_url(const char *url_orig, char **ret_host, char **ret_path);
+
 #endif
-- 
2.5.0

[PATCH v2 4/6] connect: move error check to caller of parse_connect_url

[Patrick Steinhardt]

parse_connect_url() checks if the path component of the URL is
empty and if so causes the program to die. As the function is to
be used at other call sites which do not require this check, move
up the error checking to the existing caller.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 connect.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index bdbcee4..e8b813d 100644
--- a/connect.c
+++ b/connect.c
@@ -613,9 +613,6 @@ enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 	else
 		path = strchr(end, separator);
 
-	if (!path || !*path)
-		die("No path specified. See 'man git-pull' for valid url syntax");
-
 	/*
 	 * null-terminate hostname and point path to ~ for URL's like this:
 	 *    ssh://host.xz/~user/repo
@@ -665,6 +662,9 @@ struct child_process *git_connect(int fd[2], const char *url,
 	signal(SIGCHLD, SIG_DFL);
 
 	protocol = parse_connect_url(url, &hostandport, &path);
+	if (!path || !*path)
+		die("No path specified. See 'man git-pull' for valid url syntax");
+
 	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));
-- 
2.5.0

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

[Patrick Steinhardt]

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 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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c | 73 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 20 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a72ff7e..4547729 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -9,6 +9,7 @@
  */
 
 #include "builtin.h"
+#include "connect.h"
 #include "lockfile.h"
 #include "parse-options.h"
 #include "fetch-pack.h"
@@ -147,34 +148,62 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
 	const char *end = repo + strlen(repo), *start;
+	char *host, *path;
 	size_t len;
 	char *dir;
 
+	parse_connect_url(repo, &host, &path);
 	/*
-	 * Strip trailing spaces, slashes and /.git
+	 * If the path component of the URL is empty (e.g. it is
+	 * empty or only contains a '/') we fall back to the host
+	 * name.
 	 */
-	while (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
-		end--;
-	if (end - repo > 5 && is_dir_sep(end[-5]) &&
-	    !strncmp(end - 4, ".git", 4)) {
-		end -= 5;
-		while (repo < end && is_dir_sep(end[-1]))
+	if (!path || !*path || !strcmp(path, "/")) {
+		if (*host == '\0')
+			die("No directory name could be guessed.\n"
+				"Please specify a directory on the command line");
+		/*
+		 * Strip authentication information if it exists.
+		 */
+		start = strchr(host, '@');
+		if (start)
+			start++;
+		else
+			start = host;
+		/*
+		 * Strip port if it exitsts.
+		 */
+		end = strchr(start, ':');
+		if (!end)
+			end = start + strlen(start);
+		len = end - start;
+	} else {
+		/*
+		 * Strip trailing spaces, slashes and /.git
+		 */
+		while (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
 			end--;
-	}
+		if (end - repo > 5 && is_dir_sep(end[-5]) &&
+			!strncmp(end - 4, ".git", 4)) {
+			end -= 5;
+			while (repo < end && is_dir_sep(end[-1]))
+				end--;
+		}
 
-	/*
-	 * 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--;
+		/*
+		 * 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--;
 
-	/*
-	 * Strip .{bundle,git}.
-	 */
-	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
+		/*
+		 * Strip .{bundle,git}.
+		 */
+		strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
+	}
 
 	if (is_bare)
 		dir = xstrfmt("%.*s.git", (int)len, start);
@@ -203,6 +232,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 		if (out > dir && prev_space)
 			out[-1] = '\0';
 	}
+
+	free(host);
+	free(path);
+
 	return dir;
 }
 
-- 
2.5.0

[PATCH v2 6/6] clone: add tests for cloning with empty path

[Patrick Steinhardt]

Test behavior of `git clone` when working with an empty path
component. This may be the case when cloning a file system's root
directory or from a remote server's root.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1509-root-worktree.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
index 553a3f6..acfa133 100755
--- a/t/t1509-root-worktree.sh
+++ b/t/t1509-root-worktree.sh
@@ -237,6 +237,45 @@ test_foobar_foobar
 
 test_expect_success 'cleanup' 'rm -rf /.git'
 
+say "clone .git at root without reponame"
+
+test_expect_success 'go to /' 'cd /'
+test_expect_success 'setup' '
+	echo "Initialized empty Git repository in /.git/" > expected &&
+	git init > result &&
+	test_cmp expected result
+'
+
+test_clone_expect_dir() {
+	URL="$1"
+	DIR="$2"
+	echo "Cloning into '$DIR'..." >expected
+	echo "warning: You appear to have cloned an empty repository." >>expected
+	git clone "$URL" 2>result >result
+	rm -r "$DIR"
+	test_cmp expected result
+}
+
+test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'
+test_expect_success 'simple clone of /' '
+	echo "fatal: No directory name could be guessed." > expected &&
+	echo "Please specify a directory on the command line" >> expected &&
+	test_expect_code 128 git clone / 2>result >result &&
+	test_cmp expected result'
+
+test_expect_success 'clone with file://' '
+	test_clone_expect_dir file://127.0.0.1/ 127.0.0.1'
+test_expect_success 'clone with file://user@' '
+	test_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'
+test_expect_success 'clone with file://user:password@' '
+	test_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'
+test_expect_success 'clone with file://:port' '
+	test_clone_expect_dir file://127.0.0.1:9999/ 127.0.0.1'
+test_expect_success 'clone with file://user:password@:port' '
+	test_clone_expect_dir file://user:password@127.0.0.1:9999/ 127.0.0.1'
+
+test_expect_success 'cleanup' 'rm -rf /.git /clones'
+
 say "auto bare gitdir"
 
 # DESTROYYYYY!!!!!
-- 
2.5.0

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

[Junio C Hamano]

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

Re: [PATCH v2 4/6] connect: move error check to caller of parse_connect_url

[Eric Sunshine]

On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt <ps@pks.im> wrote:
> parse_connect_url() checks if the path component of the URL is
> empty and if so causes the program to die. As the function is to
> be used at other call sites which do not require this check, move
> up the error checking to the existing caller.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/connect.c b/connect.c
> index bdbcee4..e8b813d 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -613,9 +613,6 @@ enum protocol parse_connect_url(const char *url_orig, char **ret_host,
>         else
>                 path = strchr(end, separator);
>
> -       if (!path || !*path)
> -               die("No path specified. See 'man git-pull' for valid url syntax");
> -

Given that there are several dereferences of 'path' following this bit
of code, won't this change lead to a crash when path==NULL?

>         /*
>          * null-terminate hostname and point path to ~ for URL's like this:
>          *    ssh://host.xz/~user/repo
> @@ -665,6 +662,9 @@ struct child_process *git_connect(int fd[2], const char *url,
>         signal(SIGCHLD, SIG_DFL);
>
>         protocol = parse_connect_url(url, &hostandport, &path);
> +       if (!path || !*path)
> +               die("No path specified. See 'man git-pull' for valid url syntax");
> +
>         if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
>                 printf("Diag: url=%s\n", url ? url : "NULL");
>                 printf("Diag: protocol=%s\n", prot_name(protocol));
> --
> 2.5.0

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3690 bytes --]

On Wed, Jul 29, 2015 at 10:42:21AM -0700, Junio C Hamano wrote:
> 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".

Agreed, will fix with the next iteration.

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

Actually no, as host and path components need to be treated
differently. See below.

> > -	/*
> > -	 * 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).

This case is exactly why I did include patches 3 and 4. We've got
two cases that need to be distinguished:

1. we've got a non-empty path component (that is it contains more
   than just a '/'). In this case we want to take its last part
   and strip it of things like '.git'. We should only honor ':'
   as a path separator if it is the first character in the path
   component, otherwise only honor '/'.

2. we've got an empty path component. In this case we want to
   inspect the host part. If it is empty we have to error out,
   otherwise we want to strip it of authentication information
   (everythin up to and including '@') and port information
   (everything following the ':').

So both cases are treated entirely different. For your example
we'd first split up 'http://example.com/foo:bar.git' into the
host 'example.com' and the path '/foo:bar.git'. As
'parse_connect_url()' does exactly what we need, e.g. split up
host and path, I think it is only natural to reuse it.

But actually you are right, currently I still have the old logic
in place that splits on colons in the path component. In my case
it would be 'parse_connect_url()'s responsibility to detect if
host and path are not separated by '/' but by ':' and thus we'd
not run into the problem with 'foo:bar.git'. I'll verify that
behavior though and write some tests.

Patrick

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 4/6] connect: move error check to caller of parse_connect_url

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On Wed, Jul 29, 2015 at 04:32:13PM -0400, Eric Sunshine wrote:
> On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt <ps@pks.im> wrote:
> > parse_connect_url() checks if the path component of the URL is
> > empty and if so causes the program to die. As the function is to
> > be used at other call sites which do not require this check, move
> > up the error checking to the existing caller.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/connect.c b/connect.c
> > index bdbcee4..e8b813d 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -613,9 +613,6 @@ enum protocol parse_connect_url(const char *url_orig, char **ret_host,
> >         else
> >                 path = strchr(end, separator);
> >
> > -       if (!path || !*path)
> > -               die("No path specified. See 'man git-pull' for valid url syntax");
> > -
> 
> Given that there are several dereferences of 'path' following this bit
> of code, won't this change lead to a crash when path==NULL?

Correct, will fix, thanks.

Patrick

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> 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).
>
> This case is exactly why I did include patches 3 and 4.

Well, but there is the above "or we may not" ;-)

> But actually you are right, currently I still have the old logic
> in place that splits on colons in the path component. 

Yes.  The reason why I suggested the simple route was exactly
because I noticed that you didn't seem to care about the above
"$site/foo:bar.git/" => "$site/foo:bar" => "bar" transform.

And I think people might depend on that behaviour.  "Fixing" that
may even be seen as a regression.

When was the last time you created a foo@bar.git repository?  Is it
really worth the code churn to do patches 3/4?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Well, but there is the above "or we may not" ;-)
>
>> But actually you are right, currently I still have the old logic
>> in place that splits on colons in the path component. 
>
> Yes.  The reason why I suggested the simple route was exactly
> because I noticed that you didn't seem to care about the above
> "$site/foo:bar.git/" => "$site/foo:bar" => "bar" transform.
>
> And I think people might depend on that behaviour.  "Fixing" that
> may even be seen as a regression.
>
> When was the last time you created a foo@bar.git repository?

Actually, this was an unrelated question and a wrong one to ask at
that.

Even though I personally haven't created foo:bar.git repository,
because it is no longer 2005, it is highly likely that somewhere
there is such a person who depends on the current behaviour of
turning that to "bar" on the cloned side.  Similarly, even if we the
people who read the Git mailing list collectively do not know
anybody who has foo@bar.git repository, it is highly likely that
somewhere there is such a person who depends on the current
behaviour of turning that to "foo@bar" on the cloned side.

So the ideal would be to keep turning $site/foo:bar.git to bar,
$site/foo@bar.git to foo@bar, and $scheme//u@p:$host/ to $host.

And it would be ideal if we do so without much code churn.  "The
whole site is dedicated to host a single repository at the root" is
a highly unlikely set-up and it feels wasteful to spend too much
effort on.

Re: [PATCH v2 6/6] clone: add tests for cloning with empty path

[Eric Sunshine]

On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt <ps@pks.im> wrote:
> Test behavior of `git clone` when working with an empty path
> component. This may be the case when cloning a file system's root
> directory or from a remote server's root.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
> index 553a3f6..acfa133 100755
> --- a/t/t1509-root-worktree.sh
> +++ b/t/t1509-root-worktree.sh
> @@ -237,6 +237,45 @@ test_foobar_foobar
>
>  test_expect_success 'cleanup' 'rm -rf /.git'
>
> +say "clone .git at root without reponame"
> +
> +test_expect_success 'go to /' 'cd /'
> +test_expect_success 'setup' '
> +       echo "Initialized empty Git repository in /.git/" > expected &&
> +       git init > result &&
> +       test_cmp expected result
> +'

I'd say something here about current style omitting the space after
the redirection operator (>), however, this script uniformly includes
the space, and being consistent with the existing style is important,
so I won't mention it. ;-)

> +test_clone_expect_dir() {
> +       URL="$1"
> +       DIR="$2"
> +       echo "Cloning into '$DIR'..." >expected
> +       echo "warning: You appear to have cloned an empty repository." >>expected

    echo >expected <<-\EOF
    Cloning into...
    warning: You appear...
    EOF

is more readable and maintainable.

> +       git clone "$URL" 2>result >result

    git clone "$URL" >result 2>&1

> +       rm -r "$DIR"
> +       test_cmp expected result
> +}

While not mandatory since it works as expected in its current form, it
would be nice to see a fully intact &&-chain in this function. That
way, if someone some day adds code which doesn't impact 'result' but
which might somehow fail, then the failure will be noticed.

> +
> +test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'
> +test_expect_success 'simple clone of /' '
> +       echo "fatal: No directory name could be guessed." > expected &&
> +       echo "Please specify a directory on the command line" >> expected &&

    cat >expected <<-\EOF
    fatal: No directory...
    Please specify...
    EOF

> +       test_expect_code 128 git clone / 2>result >result &&

    test_expect_code 128 git clone / >result 2>&1 &&

> +       test_cmp expected result'
> +
> +test_expect_success 'clone with file://' '
> +       test_clone_expect_dir file://127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://user@' '
> +       test_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://user:password@' '
> +       test_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://:port' '
> +       test_clone_expect_dir file://127.0.0.1:9999/ 127.0.0.1'
> +test_expect_success 'clone with file://user:password@:port' '
> +       test_clone_expect_dir file://user:password@127.0.0.1:9999/ 127.0.0.1'
> +
> +test_expect_success 'cleanup' 'rm -rf /.git /clones'
> +
>  say "auto bare gitdir"
>
>  # DESTROYYYYY!!!!!
> --
> 2.5.0

Re: [PATCH v2 6/6] clone: add tests for cloning with empty path

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt <ps@pks.im> wrote:
>> Test behavior of `git clone` when working with an empty path
>> component. This may be the case when cloning a file system's root
>> directory or from a remote server's root.
>>
>> Signed-off-by: Patrick Steinhardt <ps@pks.im>
>> ---
>> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
>> index 553a3f6..acfa133 100755
>> --- a/t/t1509-root-worktree.sh
>> +++ b/t/t1509-root-worktree.sh
>> @@ -237,6 +237,45 @@ test_foobar_foobar

All true, but a more interesting question is why add more to this
test, which is known to be skipped by everybody?  The issue being
corrected is that any "<scheme>://<user>@<pass>:<site>/" that says
"the whole site serves a single repository" is problematic.

Surely, file:// and ssh:// may be examples of schemes that require
the filesystem root to be usable as the trash directory to test,
requiring a dedicated VM (causing most people to skip t1509), but
wouldn't "http://<user>@<pass>:<site>/" be easier to arrange to make
the whole site serve a single repository?

Re: [PATCH v2 6/6] clone: add tests for cloning with empty path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]

On Thu, Jul 30, 2015 at 05:58:04PM -0700, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt <ps@pks.im> wrote:
> >> Test behavior of `git clone` when working with an empty path
> >> component. This may be the case when cloning a file system's root
> >> directory or from a remote server's root.
> >>
> >> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> >> ---
> >> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
> >> index 553a3f6..acfa133 100755
> >> --- a/t/t1509-root-worktree.sh
> >> +++ b/t/t1509-root-worktree.sh
> >> @@ -237,6 +237,45 @@ test_foobar_foobar
> 
> All true, but a more interesting question is why add more to this
> test, which is known to be skipped by everybody?  The issue being
> corrected is that any "<scheme>://<user>@<pass>:<site>/" that says
> "the whole site serves a single repository" is problematic.
> 
> Surely, file:// and ssh:// may be examples of schemes that require
> the filesystem root to be usable as the trash directory to test,
> requiring a dedicated VM (causing most people to skip t1509), but
> wouldn't "http://<user>@<pass>:<site>/" be easier to arrange to make
> the whole site serve a single repository?

Sure it would be. But unfortunately I haven't been able to get
t/lib-httpd working at my end, so that's why I then chose to
implement the tests with t1509. I agree though that the other
solution would be preferable, but I currently am not able to
provide those.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

On Thu, Jul 30, 2015 at 09:53:07AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Well, but there is the above "or we may not" ;-)
> >
> >> But actually you are right, currently I still have the old logic
> >> in place that splits on colons in the path component. 
> >
> > Yes.  The reason why I suggested the simple route was exactly
> > because I noticed that you didn't seem to care about the above
> > "$site/foo:bar.git/" => "$site/foo:bar" => "bar" transform.
> >
> > And I think people might depend on that behaviour.  "Fixing" that
> > may even be seen as a regression.
> >
> > When was the last time you created a foo@bar.git repository?
> 
> Actually, this was an unrelated question and a wrong one to ask at
> that.
> 
> Even though I personally haven't created foo:bar.git repository,
> because it is no longer 2005, it is highly likely that somewhere
> there is such a person who depends on the current behaviour of
> turning that to "bar" on the cloned side.  Similarly, even if we the
> people who read the Git mailing list collectively do not know
> anybody who has foo@bar.git repository, it is highly likely that
> somewhere there is such a person who depends on the current
> behaviour of turning that to "foo@bar" on the cloned side.
> 
> So the ideal would be to keep turning $site/foo:bar.git to bar,
> $site/foo@bar.git to foo@bar, and $scheme//u@p:$host/ to $host.
> 
> And it would be ideal if we do so without much code churn.  "The
> whole site is dedicated to host a single repository at the root" is
> a highly unlikely set-up and it feels wasteful to spend too much
> effort on.

One more question for backwards compatibility remains then.
Currently when we clone something like 'http://example.com:2222/'
we'd create a git repository '2222' as we'd split on the first
occurrence of ':'. Should we remain backwards compatible here, as
well, or change the behavior to use 'example.com' as repository
name?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jeff King]

On Mon, Aug 03, 2015 at 10:34:14AM +0200, Patrick Steinhardt wrote:

> One more question for backwards compatibility remains then.
> Currently when we clone something like 'http://example.com:2222/'
> we'd create a git repository '2222' as we'd split on the first
> occurrence of ':'. Should we remain backwards compatible here, as
> well, or change the behavior to use 'example.com' as repository
> name?

I don't think naming the repo "2222" makes much sense; I'd consider it a
bug. The only sensible names are "example.com" or "example.com:2222"
(the latter is more specific if you are going to clone the root off of
several different ports, but that seems rather unlikely; the former is
probably what I'd expect).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 03, 2015 at 10:34:14AM +0200, Patrick Steinhardt wrote:
>
>> One more question for backwards compatibility remains then.
>> Currently when we clone something like 'http://example.com:2222/'
>> we'd create a git repository '2222' as we'd split on the first
>> occurrence of ':'. Should we remain backwards compatible here, as
>> well, or change the behavior to use 'example.com' as repository
>> name?
>
> I don't think naming the repo "2222" makes much sense; I'd consider it a
> bug. The only sensible names are "example.com" or "example.com:2222"
> (the latter is more specific if you are going to clone the root off of
> several different ports, but that seems rather unlikely; the former is
> probably what I'd expect).

Yeah, I tend to agree.

[PATCH v3 0/6] fix repo name when cloning a server's root

[Patrick Steinhardt]

This is the third version of this patch series. It aims to
improve guessing directory names such that we do not include
authentication data and port numbers in them.

This version drops the patches exposing 'parse_connect_url()' and
instead does the stripping of authentification data and port
inside the URI in 'guess_dir_name()'.

Actually I'm not that happy with the patch as it currently stands
as it requires a lot of complexity to correctly strip the URI
such that we do not mishandle several corner cases. At least I
didn't find any shorter way of doing what I want without breaking
backwards compatibility. I'll try to explain why the more complex
ways of handling the URI are required:

 - The naive way of just adding '@' as path separator would break
   cloning repositories like '/foo/bar@baz.git' (which would
   currently become 'bar@baz' but would become 'baz' only).

 - Skipping the scheme initially is required because without it we
   wouldn't be able to scan until the next dir separator in the
   host part when stripping authentication information.

 - First checking for '/' in the current stripped URI when we
   want to remove the port is required because we do not want to
   strip port numbers when cloning from something like
   '/foo/bar:2222.git' (which would currently become '2222' but
   would then be stripped of the ':2222' part and instead become
   'bar'). Still, this breaks on cloning a bare repository in the
   current dir (e.g. cloning 'bar:2222.git', which should become
   '2222' because it is not a port number but would become
   'bar').

As you can see, there is a lot of complexity in there and I'm not
convinced this is better than just exposing
'parse_connect_url()', which already handles everything for us.
Maybe I'm just being blind for the obvious solution here, though.

Patrick Steinhardt (6):
  tests: fix broken && chains in t1509-root-worktree
  tests: fix cleanup after tests in t1509-root-worktree
  clone: do not include authentication data in guessed dir
  clone: do not use port number as dir name
  clone: abort if no dir name could be guessed
  clone: add tests for cloning with empty path

 builtin/clone.c          | 67 ++++++++++++++++++++++++++++++++++++++++--------
 t/t1509-root-worktree.sh | 51 +++++++++++++++++++++++++++++++++---
 2 files changed, 103 insertions(+), 15 deletions(-)

-- 
2.5.0

[PATCH v3 1/6] tests: fix broken && chains in t1509-root-worktree

[Patrick Steinhardt]

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1509-root-worktree.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
index b6977d4..0c80129 100755
--- a/t/t1509-root-worktree.sh
+++ b/t/t1509-root-worktree.sh
@@ -125,7 +125,7 @@ fi
 ONE_SHA1=d00491fd7e5bb6fa28c517a0bb32b8b506539d4d
 
 test_expect_success 'setup' '
-	rm -rf /foo
+	rm -rf /foo &&
 	mkdir /foo &&
 	mkdir /foo/bar &&
 	echo 1 > /foo/foome &&
@@ -218,7 +218,7 @@ unset GIT_WORK_TREE
 
 test_expect_success 'go to /' 'cd /'
 test_expect_success 'setup' '
-	rm -rf /.git
+	rm -rf /.git &&
 	echo "Initialized empty Git repository in /.git/" > expected &&
 	git init > result &&
 	test_cmp expected result
@@ -241,8 +241,8 @@ say "auto bare gitdir"
 
 # DESTROYYYYY!!!!!
 test_expect_success 'setup' '
-	rm -rf /refs /objects /info /hooks
-	rm /*
+	rm -rf /refs /objects /info /hooks &&
+	rm /* &&
 	cd / &&
 	echo "Initialized empty Git repository in /" > expected &&
 	git init --bare > result &&
-- 
2.5.0

[PATCH v3 2/6] tests: fix cleanup after tests in t1509-root-worktree

[Patrick Steinhardt]

During cleanup we do a simple 'rm /*' to remove leftover files
from previous tests. As 'rm' errors out when there is anything it
cannot delete and there are directories present at '/' it will
throw an error, causing the '&&' chain to fail.

Fix this by explicitly removing the files.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1509-root-worktree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
index 0c80129..553a3f6 100755
--- a/t/t1509-root-worktree.sh
+++ b/t/t1509-root-worktree.sh
@@ -242,7 +242,7 @@ say "auto bare gitdir"
 # DESTROYYYYY!!!!!
 test_expect_success 'setup' '
 	rm -rf /refs /objects /info /hooks &&
-	rm /* &&
+	rm -f /expected /ls.expected /me /result &&
 	cd / &&
 	echo "Initialized empty Git repository in /" > expected &&
 	git init --bare > result &&
-- 
2.5.0

[PATCH v3 3/6] clone: do not include authentication data in guessed dir

[Patrick Steinhardt]

If the URI contains authentication data and the URI's path
component is empty we fail to guess a sensible directory name.
E.g. cloning a repository 'ssh://user:password@example.com/' we
guess a directory name 'password@example.com' where we would want
the hostname only, e.g. 'example.com'.

Fix this by first skipping authentication data.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a72ff7e..794a933 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -146,35 +146,59 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
-	const char *end = repo + strlen(repo), *start;
+	const char *end = repo + strlen(repo), *start, *ptr;
 	size_t len;
 	char *dir;
 
 	/*
+	 * Skip scheme.
+	 */
+	start = strstr(repo, "://");
+	if (start == NULL)
+		start = repo;
+	else
+		start += 3;
+
+	/*
+	 * Skip authentication data.
+	 */
+	ptr = start;
+	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
+		ptr++;
+	if (*ptr == '@')
+		start = ptr + 1;
+
+	/*
 	 * Strip trailing spaces, slashes and /.git
 	 */
-	while (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
+	while (start < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
 		end--;
-	if (end - repo > 5 && is_dir_sep(end[-5]) &&
+	if (end - start > 5 && is_dir_sep(end[-5]) &&
 	    !strncmp(end - 4, ".git", 4)) {
 		end -= 5;
-		while (repo < end && is_dir_sep(end[-1]))
+		while (start < end && is_dir_sep(end[-1]))
 			end--;
 	}
 
 	/*
-	 * 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.
+	 * Find last component. To remain backwards compatible we
+	 * also regard colons as path separators, such that
+	 * cloning a repository 'foo:bar.git' would result in a
+	 * directory 'bar' being guessed. This is skipped if the
+	 * current path does not contain any path separators.
 	 */
-	start = end;
-	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
-		start--;
+	if (memchr(start, '/', end - start) != NULL
+	    || memchr(start, ':', end - start) != NULL) {
+		start = end;
+		while (!is_dir_sep(start[-1]) && start[-1] != ':')
+			start--;
+	}
+	len = end - start;
 
 	/*
 	 * Strip .{bundle,git}.
 	 */
-	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
+	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");
 
 	if (is_bare)
 		dir = xstrfmt("%.*s.git", (int)len, start);
-- 
2.5.0

[PATCH v3 4/6] clone: do not use port number as dir name

[Patrick Steinhardt]

If the URI contains a port number and the URI's path component is
empty we fail to guess a sensible directory name. E.g. cloning a
repository 'ssh://example.com:2222/' we guess a directory name
'2222' where we would want the hostname only, e.g. 'example.com'.

Fix this by stripping trailing port numbers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 794a933..a163797 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -181,6 +181,23 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	}
 
 	/*
+	 * Strip trailing port number if we've got only a
+	 * hostname (that is, there is no dir separator but a
+	 * colon). This check is required such that we do not
+	 * strip URI's like '/foo/bar:2222.git', which should
+	 * result in a dir '2222' being guessed due to backwards
+	 * compatibility.
+	 */
+	if (memchr(start, '/', end - start) == NULL
+	    && memchr(start, ':', end - start) != NULL) {
+		ptr = end;
+		while (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')
+			ptr--;
+		if (start < ptr && ptr[-1] == ':')
+			end = ptr - 1;
+	}
+
+	/*
 	 * Find last component. To remain backwards compatible we
 	 * also regard colons as path separators, such that
 	 * cloning a repository 'foo:bar.git' would result in a
-- 
2.5.0

[PATCH v3 5/6] clone: abort if no dir name could be guessed

[Patrick Steinhardt]

Due to various components of the URI being stripped off it may
happen that we fail to guess a directory name. We currently error
out with a message that it is impossible to create the working
tree '' in such cases. Instead, error out early with a sensible
error message hinting that a directory name should be specified
manually on the command line.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index a163797..2adc712 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -217,6 +217,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	 */
 	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");
 
+	if (!len || (len == 1 && *start == '/'))
+	    die("No directory name could be guessed.\n"
+		"Please specify a directory on the command line");
+
 	if (is_bare)
 		dir = xstrfmt("%.*s.git", (int)len, start);
 	else
-- 
2.5.0

[PATCH v3 6/6] clone: add tests for cloning with empty path

[Patrick Steinhardt]

Test behavior of `git clone` when working with an empty path
component. This may be the case when cloning a file system's root
directory or from a remote server's root.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1509-root-worktree.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
index 553a3f6..d521ca3 100755
--- a/t/t1509-root-worktree.sh
+++ b/t/t1509-root-worktree.sh
@@ -237,6 +237,49 @@ test_foobar_foobar
 
 test_expect_success 'cleanup' 'rm -rf /.git'
 
+say "clone .git at root without reponame"
+
+test_expect_success 'go to /' 'cd /'
+test_expect_success 'setup' '
+	echo "Initialized empty Git repository in /.git/" > expected &&
+	git init > result &&
+	test_cmp expected result
+'
+
+test_clone_expect_dir() {
+	URL="$1"
+	DIR="$2"
+	cat <<-EOF >expected &&
+		Cloning into '$DIR'...
+		warning: You appear to have cloned an empty repository.
+		EOF
+	git clone "$URL" >result 2>&1 &&
+	rm -rf "$DIR" &&
+	test_cmp expected result
+}
+
+test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'
+test_expect_success 'simple clone of /' '
+	cat <<-EOF >expected &&
+		fatal: No directory name could be guessed.
+		Please specify a directory on the command line
+		EOF
+	test_expect_code 128 git clone / >result 2>&1 &&
+	test_cmp expected result'
+
+test_expect_success 'clone with file://host/' '
+	test_clone_expect_dir file://127.0.0.1/ 127.0.0.1'
+test_expect_success 'clone with file://user@host/' '
+	test_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'
+test_expect_success 'clone with file://user:password@host/' '
+	test_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'
+test_expect_success 'clone with file://host:port/' '
+	test_clone_expect_dir file://127.0.0.1:9999/ 127.0.0.1'
+test_expect_success 'clone with file://user:password@host:port' '
+	test_clone_expect_dir file://user:password@127.0.0.1:9999/ 127.0.0.1'
+
+test_expect_success 'cleanup' 'rm -rf /.git /clones'
+
 say "auto bare gitdir"
 
 # DESTROYYYYY!!!!!
-- 
2.5.0

Re: [PATCH v3 6/6] clone: add tests for cloning with empty path

[Eric Sunshine]

On Tue, Aug 4, 2015 at 7:29 AM, Patrick Steinhardt <ps@pks.im> wrote:
> Test behavior of `git clone` when working with an empty path
> component. This may be the case when cloning a file system's root
> directory or from a remote server's root.

A few minor, mostly style-related, comments below...

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
> index 553a3f6..d521ca3 100755
> --- a/t/t1509-root-worktree.sh
> +++ b/t/t1509-root-worktree.sh
> @@ -237,6 +237,49 @@ test_foobar_foobar
>
>  test_expect_success 'cleanup' 'rm -rf /.git'
>
> +say "clone .git at root without reponame"
> +
> +test_expect_success 'go to /' 'cd /'
> +test_expect_success 'setup' '
> +       echo "Initialized empty Git repository in /.git/" > expected &&
> +       git init > result &&
> +       test_cmp expected result
> +'
> +
> +test_clone_expect_dir() {
> +       URL="$1"
> +       DIR="$2"

It would be nice for the &&-chain to be intact for these two lines, as
well, since you never know where someone may insert code in the
future. If code is inserted above or between these lines, and the code
fails, its failure will go unnoticed due to the broken &&-chain.

> +       cat <<-EOF >expected &&

In this codebase, it's customary to say:

    cat >expected <<-EOF &&

> +               Cloning into '$DIR'...
> +               warning: You appear to have cloned an empty repository.
> +               EOF

In this codebase, it's customary to place the content and EOF at the
same indentation level as the opening 'cat <<EOF'.

> +       git clone "$URL" >result 2>&1 &&
> +       rm -rf "$DIR" &&
> +       test_cmp expected result
> +}
> +
> +test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'
> +test_expect_success 'simple clone of /' '
> +       cat <<-EOF >expected &&

Since you don't expect any variable interpolation in this case, you
can telegraph that intent more clearly via:

    cat >expected <<-\EOF &&

> +               fatal: No directory name could be guessed.
> +               Please specify a directory on the command line
> +               EOF
> +       test_expect_code 128 git clone / >result 2>&1 &&
> +       test_cmp expected result'
> +
> +test_expect_success 'clone with file://host/' '
> +       test_clone_expect_dir file://127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://user@host/' '
> +       test_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://user:password@host/' '
> +       test_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://host:port/' '
> +       test_clone_expect_dir file://127.0.0.1:9999/ 127.0.0.1'
> +test_expect_success 'clone with file://user:password@host:port' '
> +       test_clone_expect_dir file://user:password@127.0.0.1:9999/ 127.0.0.1'
> +
> +test_expect_success 'cleanup' 'rm -rf /.git /clones'
> +
>  say "auto bare gitdir"
>
>  # DESTROYYYYY!!!!!
> --
> 2.5.0

[PATCH v4 0/3] fix repo name when cloning a server's root

[Patrick Steinhardt]

This is version 4 of my patch series, which aims to improve
guessed directory names when we clone a server's root, that is we
have empty path components.

This version is still preliminary as it is based upon the patches
by Peff ([PATCH 0/2] fix clone guess_dir_name regression in
v2.4.8) which have not been merged yet.

As his patches include newly added tests that allow for cloning
from a server's root without using chroot, the patches regarding
t1509 have been dropped. I've posted the fixes to them as a
separate patch series instead.

Previously I've included a guard such that we only try to find
the last component if we've got a '/' or ':' in the path. This is
not really required, though, as in the case where we ain't got
one of those characters we'll simply skip to the beginning again,
causing this to be a no-op, due to the port already being
stripped. So I've simply dropped the guard to minimize code
churn.

Patrick Steinhardt (3):
  clone: do not include authentication data in guessed dir
  clone: do not use port number as dir name
  clone: abort if no dir name could be guessed

 builtin/clone.c          | 61 ++++++++++++++++++++++++++++++++++++++++--------
 t/t5603-clone-dirname.sh | 10 ++++++--
 2 files changed, 59 insertions(+), 12 deletions(-)

-- 
2.5.0

[PATCH v4 1/3] clone: do not include authentication data in guessed dir

[Patrick Steinhardt]

If the URI contains authentication data and the URI's path
component is empty we fail to guess a sensible directory name.
E.g. cloning a repository 'ssh://user:password@example.com/' we
guess a directory name 'password@example.com' where we would want
the hostname only, e.g. 'example.com'.

Fix this by first skipping authentication data.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c          | 40 ++++++++++++++++++++++++++++++----------
 t/t5603-clone-dirname.sh |  3 ++-
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index bf45199..3ddf8b9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -146,30 +146,50 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
-	const char *end = repo + strlen(repo), *start;
+	const char *end = repo + strlen(repo), *start, *ptr;
 	size_t len;
 	char *dir;
 
 	/*
+	 * Skip scheme.
+	 */
+	start = strstr(repo, "://");
+	if (start == NULL)
+		start = repo;
+	else
+		start += 3;
+
+	/*
+	 * Skip authentication data.
+	 */
+	ptr = start;
+	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
+		ptr++;
+	if (*ptr == '@')
+		start = ptr + 1;
+
+	/*
 	 * Strip trailing spaces, slashes and /.git
 	 */
-	while (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
+	while (start < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
 		end--;
-	if (end - repo > 5 && is_dir_sep(end[-5]) &&
+	if (end - start > 5 && is_dir_sep(end[-5]) &&
 	    !strncmp(end - 4, ".git", 4)) {
 		end -= 5;
-		while (repo < end && is_dir_sep(end[-1]))
+		while (start < end && is_dir_sep(end[-1]))
 			end--;
 	}
 
 	/*
-	 * 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.
+	 * Find last component. To remain backwards compatible we
+	 * also regard colons as path separators, such that
+	 * cloning a repository 'foo:bar.git' would result in a
+	 * directory 'bar' being guessed.
 	 */
-	start = end;
-	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
-		start--;
+	ptr = end;
+	while (start < ptr && !is_dir_sep(ptr[-1]) && ptr[-1] != ':')
+		ptr--;
+	start = ptr;
 
 	/*
 	 * Strip .{bundle,git}.
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index 46725b9..3a454f9 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -64,6 +64,7 @@ test_clone_dir ssh://host/foo/.git/ foo
 # omitting the path should default to the hostname
 test_clone_dir ssh://host/ host
 test_clone_dir ssh://host:1234/ host fail
-test_clone_dir ssh://user@host/ host fail
+test_clone_dir ssh://user@host/ host
+test_clone_dir ssh://user:password@host/ host
 
 test_done
-- 
2.5.0

[PATCH v4 2/3] clone: do not use port number as dir name

[Patrick Steinhardt]

If the URI contains a port number and the URI's path component is
empty we fail to guess a sensible directory name. E.g. cloning a
repository 'ssh://example.com:2222/' we guess a directory name
'2222' where we would want the hostname only, e.g. 'example.com'.

Fix this by stripping trailing port numbers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c          | 17 +++++++++++++++++
 t/t5603-clone-dirname.sh |  7 ++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 3ddf8b9..7d93e13 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -181,6 +181,23 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	}
 
 	/*
+	 * Strip trailing port number if we've got only a
+	 * hostname (that is, there is no dir separator but a
+	 * colon). This check is required such that we do not
+	 * strip URI's like '/foo/bar:2222.git', which should
+	 * result in a dir '2222' being guessed due to backwards
+	 * compatibility.
+	 */
+	if (memchr(start, '/', end - start) == NULL
+	    && memchr(start, ':', end - start) != NULL) {
+		ptr = end;
+		while (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')
+			ptr--;
+		if (start < ptr && ptr[-1] == ':')
+			end = ptr - 1;
+	}
+
+	/*
 	 * Find last component. To remain backwards compatible we
 	 * also regard colons as path separators, such that
 	 * cloning a repository 'foo:bar.git' would result in a
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index 3a454f9..27dbd6c 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -63,8 +63,13 @@ test_clone_dir ssh://host/foo/.git/ foo
 
 # omitting the path should default to the hostname
 test_clone_dir ssh://host/ host
-test_clone_dir ssh://host:1234/ host fail
+test_clone_dir ssh://host:1234/ host
 test_clone_dir ssh://user@host/ host
 test_clone_dir ssh://user:password@host/ host
+test_clone_dir ssh://user:password@host:1234/ host
+
+# trailing port-like numbers should not be stripped for paths
+test_clone_dir ssh://user:password@host/test:1234 1234
+test_clone_dir ssh://user:password@host/test:1234.git 1234
 
 test_done
-- 
2.5.0

[PATCH v4 3/3] clone: abort if no dir name could be guessed

[Patrick Steinhardt]

Due to various components of the URI being stripped off it may
happen that we fail to guess a directory name. We currently error
out with a message that it is impossible to create the working
tree '' in such cases. Instead, error out early with a sensible
error message hinting that a directory name should be specified
manually on the command line.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 7d93e13..5834978 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -214,6 +214,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	len = end - start;
 	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");
 
+	if (!len || (len == 1 && *start == '/'))
+	    die("No directory name could be guessed.\n"
+		"Please specify a directory on the command line");
+
 	if (is_bare)
 		dir = xstrfmt("%.*s.git", (int)len, start);
 	else
-- 
2.5.0

Re: [PATCH v3 0/6] fix repo name when cloning a server's root

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>  - The naive way of just adding '@' as path separator would break
>    cloning repositories like '/foo/bar@baz.git' (which would
>    currently become 'bar@baz' but would become 'baz' only).
>
>  - Skipping the scheme initially is required because without it we
>    wouldn't be able to scan until the next dir separator in the
>    host part when stripping authentication information.
>
>  - First checking for '/' in the current stripped URI when we
>    want to remove the port is required because we do not want to
>    strip port numbers when cloning from something like
>    '/foo/bar:2222.git' (which would currently become '2222' but
>    would then be stripped of the ':2222' part and instead become
>    'bar'). Still, this breaks on cloning a bare repository in the
>    current dir (e.g. cloning 'bar:2222.git', which should become
>    '2222' because it is not a port number but would become
>    'bar').

This is a very good write-up.

Please make sure all of the above appears in the commit log message
somewhere.

> As you can see, there is a lot of complexity in there and I'm not
> convinced this is better than just exposing
> 'parse_connect_url()', which already handles everything for us.

If the function "handles everything for us", that's fine, but the
primary reason I am hesitant is because parse_connect_url() was
designed specifically not to have to worry about some protocols
(e.g. I think feeding it a "http://" would fail, and more
importantly, its current callers want such a call to fail).  Also it
is meant to handle some non-protocols (e.g. scp style host:path that
does not follow <scheme>://...).

Also does it handle the "2222" case above?  I do not think
parse_connect_url() even calls get_host_and_port() to be able to
tell what "2222" means in these examples.

> Maybe I'm just being blind for the obvious solution here, though.
>
> Patrick Steinhardt (6):
>   tests: fix broken && chains in t1509-root-worktree
>   tests: fix cleanup after tests in t1509-root-worktree
>   clone: do not include authentication data in guessed dir
>   clone: do not use port number as dir name
>   clone: abort if no dir name could be guessed
>   clone: add tests for cloning with empty path
>
>  builtin/clone.c          | 67 ++++++++++++++++++++++++++++++++++++++++--------
>  t/t1509-root-worktree.sh | 51 +++++++++++++++++++++++++++++++++---
>  2 files changed, 103 insertions(+), 15 deletions(-)

Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> If the URI contains authentication data and the URI's path
> component is empty we fail to guess a sensible directory name.
> E.g. cloning a repository 'ssh://user:password@example.com/' we
> guess a directory name 'password@example.com' where we would want
> the hostname only, e.g. 'example.com'.
>
> ...
> +	ptr = start;
> +	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
> +		ptr++;

Hmm....

> +	if (*ptr == '@')
> +		start = ptr + 1;
> +
> +	 * Find last component. To remain backwards compatible we
> +	 * also regard colons as path separators, such that
> +	 * cloning a repository 'foo:bar.git' would result in a
> +	 * directory 'bar' being guessed.
>  	 */

I think this is a reasonable thing to do (besides, I think some
people cannot have colon in their filenames, so keeping this aspect
the same as before would avoid unintended regressions).

> -	start = end;
> -	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
> -		start--;
> +	ptr = end;
> +	while (start < ptr && !is_dir_sep(ptr[-1]) && ptr[-1] != ':')
> +		ptr--;
> +	start = ptr;
>  
>  	/*
>  	 * Strip .{bundle,git}.
> diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
> index 46725b9..3a454f9 100755
> --- a/t/t5603-clone-dirname.sh
> +++ b/t/t5603-clone-dirname.sh
> @@ -64,6 +64,7 @@ test_clone_dir ssh://host/foo/.git/ foo
>  # omitting the path should default to the hostname
>  test_clone_dir ssh://host/ host
>  test_clone_dir ssh://host:1234/ host fail
> -test_clone_dir ssh://user@host/ host fail
> +test_clone_dir ssh://user@host/ host
> +test_clone_dir ssh://user:password@host/ host

Perhaps add

"test_clone_dir ssh://user:passw@rd@host/ host"

here?  How is this expected to be parsed?

>  test_done

Re: [PATCH v4 3/3] clone: abort if no dir name could be guessed

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Due to various components of the URI being stripped off it may
> happen that we fail to guess a directory name. We currently error
> out with a message that it is impossible to create the working
> tree '' in such cases. Instead, error out early with a sensible
> error message hinting that a directory name should be specified
> manually on the command line.

Sounds like a sensible thing to do.

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/clone.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 7d93e13..5834978 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -214,6 +214,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>  	len = end - start;
>  	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");
>  
> +	if (!len || (len == 1 && *start == '/'))
> +	    die("No directory name could be guessed.\n"
> +		"Please specify a directory on the command line");
> +
>  	if (is_bare)
>  		dir = xstrfmt("%.*s.git", (int)len, start);
>  	else

Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Perhaps add
>
> "test_clone_dir ssh://user:passw@rd@host/ host"
>
> here?  How is this expected to be parsed?

For completeness, here is what I think the end result (together with
Peff's series) of the test should look like.

The first hunk is merely style.  We could drop 'in "$@"' from there
and some people may argue that it would be more obvious, but I think
being explict is fine.

As to the second hunk:

 - the first batch is for "trailing slash removal" for scp-like
   syntax;

 - the second batch is for "omitting path should default to host" for
   the same;

 - the third batch is for "omitting authentication material" for the
   same.

Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
tests fail for the same reason (finding @ should be greedy, I think).

 t/t5603-clone-dirname.sh | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index 27dbd6c..4897ea8 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -22,7 +22,8 @@ test_clone_dir () {
 	expect=success
 	bare=non-bare
 	clone_opts=
-	for i in "$@"; do
+	for i in "$@"
+	do
 		case "$i" in
 		fail)
 			expect=failure
@@ -61,12 +62,23 @@ test_clone_dir ssh://host/foo/ foo
 test_clone_dir ssh://host/foo.git/ foo
 test_clone_dir ssh://host/foo/.git/ foo
 
+test_clone_dir host:foo/ foo
+test_clone_dir host:foo.git/ foo
+test_clone_dir host:foo/.git/ foo
+
 # omitting the path should default to the hostname
 test_clone_dir ssh://host/ host
 test_clone_dir ssh://host:1234/ host
 test_clone_dir ssh://user@host/ host
+test_clone_dir host:/ host
+
+# auth materials should be redacted
 test_clone_dir ssh://user:password@host/ host
 test_clone_dir ssh://user:password@host:1234/ host
+test_clone_dir ssh://user:passw@rd@host:1234/ host
+test_clone_dir user@host:/ host
+test_clone_dir user:password@host:/ host
+test_clone_dir user:passw@rd@host:/ host
 
 # trailing port-like numbers should not be stripped for paths
 test_clone_dir ssh://user:password@host/test:1234 1234

Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> For completeness, here is what I think the end result (together with
> Peff's series) of the test should look like.
> ...
> Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
> tests fail for the same reason (finding @ should be greedy, I think).

And I think this should make it pass.  Just remember the last
occurrence of '@' by moving the 'start' every time we see an '@'
sign.

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

diff --git a/builtin/clone.c b/builtin/clone.c
index cae288f..5d86439 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 		start += 3;
 
 	/*
-	 * Skip authentication data.
+	 * Skip authentication data, if exists.
 	 */
-	ptr = start;
-	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
-		ptr++;
-	if (*ptr == '@')
-		start = ptr + 1;
+	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
+		if (*ptr == '@')
+			start = ptr + 1;
+	}
 
 	/*
 	 * Strip trailing spaces, slashes and /.git

Re: [PATCH v3 0/6] fix repo name when cloning a server's root

[Jeff King]

On Wed, Aug 05, 2015 at 10:34:34AM -0700, Junio C Hamano wrote:

> > As you can see, there is a lot of complexity in there and I'm not
> > convinced this is better than just exposing
> > 'parse_connect_url()', which already handles everything for us.
> 
> If the function "handles everything for us", that's fine, but the
> primary reason I am hesitant is because parse_connect_url() was
> designed specifically not to have to worry about some protocols
> (e.g. I think feeding it a "http://" would fail, and more
> importantly, its current callers want such a call to fail).  Also it
> is meant to handle some non-protocols (e.g. scp style host:path that
> does not follow <scheme>://...).

True, but the transport code _is_ handling that at some point. It makes
me wonder if it would be possible to push the call to transport_get
further up inside cmd_clone(), and then provide some way to query the
remote path and hostname from the transport code. Then guess_dir_name
could just go away entirely, in favor of something like:

  dir_name = transport_get_path(transport);
  if (!*dir_name)
	dir_name = transport_get_host(transport);

That may be overly simplistic or unworkable, though. I haven't dug into
the code.

> Also does it handle the "2222" case above?  I do not think
> parse_connect_url() even calls get_host_and_port() to be able to
> tell what "2222" means in these examples.

Speaking of which, has anyone tested whether the old or new code handles
external remote helpers? Certainly:

  foo::https://host/repo.git

should still use repo.git. But technically the string handed to
git-remote-foo does not have to look anything like a URL. In those cases
neither guess_dir_name nor the transport code have any idea what anything
to the right of the "::" means; we probably have to resort to blind
guessing based on characters like colon and slash.

-Peff

Re: [PATCH v3 0/6] fix repo name when cloning a server's root

[Torsten Bögershausen]

On 2015-08-05 23.19, Jeff King wrote:
> On Wed, Aug 05, 2015 at 10:34:34AM -0700, Junio C Hamano wrote:
> 
>>> As you can see, there is a lot of complexity in there and I'm not
>>> convinced this is better than just exposing
>>> 'parse_connect_url()', which already handles everything for us.
I try expose and use parse_connect_url():
It handles the scp-like syntax "host:/path,
literall IPV6 addresses, port numbers,
':' without a port number and all other Git specific parsing,
which is inside and outside the RFC 3986.
(I should know, because I managed to break the parser twice,
and fix it)

I added a diagnostics to connect.c, and if you run the a simply test,
we can see that the colon slash logic is often unsufficient:

tb@mypc:~/projects/git/tb.150731_connect> ./git fetch-pack --diag-url ssh://host/
Diag: url=ssh://host/
Diag: protocol=ssh
Diag: userandhost=host
Diag: port=NONE
Diag: path=/
Diag: guesseddir=host/
tb@macce:~/projects/git/tb.150731_connect> ./git fetch-pack --diag-url ssh://host:/
Diag: url=ssh://host:/
Diag: protocol=ssh
Diag: userandhost=host
Diag: port=NONE
Diag: path=/
Diag: guesseddir=/


On top of that, you can easily write test cases in t5601, as many as you want.
The (minor) drawback is that it doesn't handle http:// or https://,
but that is easy to add in the parser, and doesn't break existing code.

The major which remains is to search for '@' in userandhost,
and strip that off.
(Or when there is a '@', search for a ':' before the '@', and strip that off)
After that, all non-printable characters should be %-escaped.
If we replace ':' as non-printable as well, we can make Windows users 1% more happy.


>>
>> If the function "handles everything for us", that's fine, but the
>> primary reason I am hesitant is because parse_connect_url() was
>> designed specifically not to have to worry about some protocols
>> (e.g. I think feeding it a "http://" would fail, and more
>> importantly, its current callers want such a call to fail).  Also it
>> is meant to handle some non-protocols (e.g. scp style host:path that
>> does not follow <scheme>://...).
> 
> True, but the transport code _is_ handling that at some point. It makes
> me wonder if it would be possible to push the call to transport_get
> further up inside cmd_clone(), and then provide some way to query the
> remote path and hostname from the transport code. Then guess_dir_name
> could just go away entirely, in favor of something like:
> 
>   dir_name = transport_get_path(transport);
>   if (!*dir_name)
> 	dir_name = transport_get_host(transport);
> 
> That may be overly simplistic or unworkable, though. I haven't dug into
> the code.
> 
>> Also does it handle the "2222" case above?  I do not think
>> parse_connect_url() even calls get_host_and_port() to be able to
>> tell what "2222" means in these examples.
> 
> Speaking of which, has anyone tested whether the old or new code handles
> external remote helpers? Certainly:
> 
>   foo::https://host/repo.git
> 
> should still use repo.git. But technically the string handed to
> git-remote-foo does not have to look anything like a URL. In those cases
> neither guess_dir_name nor the transport code have any idea what anything
> to the right of the "::" means; we probably have to resort to blind
> guessing based on characters like colon and slash.
> 
It is easy to strip the foo:: part of the url, assume that
the remote helper uses a RFC 3986 similar url syntax, so that we
can feed the reminding https://host/repo.git into the parser (see above).

If the remote helper doesn't do this, we can't guess anything, can we ?
So error out and tell the user seems the right thing to do.

In the hope that this is useful, pushed my prototype branch to
https://github.com/tboegi/git/tree/150731_connect_diag_guess_name

Re: [PATCH v3 0/6] fix repo name when cloning a server's root

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> It is easy to strip the foo:: part of the url, assume that
> the remote helper uses a RFC 3986 similar url syntax, so that we
> can feed the reminding https://host/repo.git into the parser (see above).

The thing that worries me is that foo:: syntax and external helper
interface was invented by Daniel Barkalow primarily because he
wanted to allow things that do *not* fit in that URL-like scheme,
for example see:

  http://thread.gmane.org/gmane.comp.version-control.git/125374/focus=125405

> If the remote helper doesn't do this, we can't guess anything, can we ?
> So error out and tell the user seems the right thing to do.

Yes.  A blind guess that fails spectacularly is far worse than an
outright rejection that is cautious.

Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > For completeness, here is what I think the end result (together with
> > Peff's series) of the test should look like.
> > ...
> > Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
> > tests fail for the same reason (finding @ should be greedy, I think).
> 
> And I think this should make it pass.  Just remember the last
> occurrence of '@' by moving the 'start' every time we see an '@'
> sign.
> 
>  builtin/clone.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index cae288f..5d86439 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>  		start += 3;
>  
>  	/*
> -	 * Skip authentication data.
> +	 * Skip authentication data, if exists.
>  	 */
> -	ptr = start;
> -	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
> -		ptr++;
> -	if (*ptr == '@')
> -		start = ptr + 1;
> +	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
> +		if (*ptr == '@')
> +			start = ptr + 1;
> +	}
>  
>  	/*
>  	 * Strip trailing spaces, slashes and /.git

I guess it makes sense to skip over @-signs greedily. Is there
anything I need to do here or will you squash those changes in?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > For completeness, here is what I think the end result (together with
>> > Peff's series) of the test should look like.
>> > ...
>> > Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
>> > tests fail for the same reason (finding @ should be greedy, I think).
>> 
>> And I think this should make it pass.  Just remember the last
>> occurrence of '@' by moving the 'start' every time we see an '@'
>> sign.
>> 
>>  builtin/clone.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>> 
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index cae288f..5d86439 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>>  		start += 3;
>>  
>>  	/*
>> -	 * Skip authentication data.
>> +	 * Skip authentication data, if exists.
>>  	 */
>> -	ptr = start;
>> -	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
>> -		ptr++;
>> -	if (*ptr == '@')
>> -		start = ptr + 1;
>> +	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
>> +		if (*ptr == '@')
>> +			start = ptr + 1;
>> +	}
>>  
>>  	/*
>>  	 * Strip trailing spaces, slashes and /.git
>
> I guess it makes sense to skip over @-signs greedily. Is there
> anything I need to do here or will you squash those changes in?

Sorry but I won't have patience to split the "these need squashing
in" into multiple pieces and add them to appropriate commits in the
5 patch series (counting Peff's two fixes on top of which you built
your 3 patch series) correctly.  You as the original author are much
better equipped to do so than I am, so I'd appreciate if you can
reroll them as a 5-patch series.

Among the changes, the fix to builtin/clone.c needs to be squashed
into your "clone: do not include authentication data in guessed
dir", and the remainder of the patch to t5603 should go to Peff's
"clone: add tests for output directory", but with most of them
marked initially as failing.  And then as you fix them in your
patches, some of the "fail" mark should be dropped.

Thanks.

 builtin/clone.c          | 11 +++++------
 t/t5603-clone-dirname.sh | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index cae288f..5d86439 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 		start += 3;
 
 	/*
-	 * Skip authentication data.
+	 * Skip authentication data, if exists.
 	 */
-	ptr = start;
-	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
-		ptr++;
-	if (*ptr == '@')
-		start = ptr + 1;
+	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
+		if (*ptr == '@')
+			start = ptr + 1;
+	}
 
 	/*
 	 * Strip trailing spaces, slashes and /.git
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index 27dbd6c..4897ea8 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -22,7 +22,8 @@ test_clone_dir () {
 	expect=success
 	bare=non-bare
 	clone_opts=
-	for i in "$@"; do
+	for i in "$@"
+	do
 		case "$i" in
 		fail)
 			expect=failure
@@ -61,12 +62,23 @@ test_clone_dir ssh://host/foo/ foo
 test_clone_dir ssh://host/foo.git/ foo
 test_clone_dir ssh://host/foo/.git/ foo
 
+test_clone_dir host:foo/ foo
+test_clone_dir host:foo.git/ foo
+test_clone_dir host:foo/.git/ foo
+
 # omitting the path should default to the hostname
 test_clone_dir ssh://host/ host
 test_clone_dir ssh://host:1234/ host
 test_clone_dir ssh://user@host/ host
+test_clone_dir host:/ host
+
+# auth materials should be redacted
 test_clone_dir ssh://user:password@host/ host
 test_clone_dir ssh://user:password@host:1234/ host
+test_clone_dir ssh://user:passw@rd@host:1234/ host
+test_clone_dir user@host:/ host
+test_clone_dir user:password@host:/ host
+test_clone_dir user:passw@rd@host:/ host
 
 # trailing port-like numbers should not be stripped for paths
 test_clone_dir ssh://user:password@host/test:1234 1234
-- 
2.5.0-423-ga363579

Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2537 bytes --]

On Fri, Aug 07, 2015 at 01:45:54PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:
> >> Junio C Hamano <gitster@pobox.com> writes:
> >> 
> >> > For completeness, here is what I think the end result (together with
> >> > Peff's series) of the test should look like.
> >> > ...
> >> > Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
> >> > tests fail for the same reason (finding @ should be greedy, I think).
> >> 
> >> And I think this should make it pass.  Just remember the last
> >> occurrence of '@' by moving the 'start' every time we see an '@'
> >> sign.
> >> 
> >>  builtin/clone.c | 11 +++++------
> >>  1 file changed, 5 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/builtin/clone.c b/builtin/clone.c
> >> index cae288f..5d86439 100644
> >> --- a/builtin/clone.c
> >> +++ b/builtin/clone.c
> >> @@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
> >>  		start += 3;
> >>  
> >>  	/*
> >> -	 * Skip authentication data.
> >> +	 * Skip authentication data, if exists.
> >>  	 */
> >> -	ptr = start;
> >> -	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
> >> -		ptr++;
> >> -	if (*ptr == '@')
> >> -		start = ptr + 1;
> >> +	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
> >> +		if (*ptr == '@')
> >> +			start = ptr + 1;
> >> +	}
> >>  
> >>  	/*
> >>  	 * Strip trailing spaces, slashes and /.git
> >
> > I guess it makes sense to skip over @-signs greedily. Is there
> > anything I need to do here or will you squash those changes in?
> 
> Sorry but I won't have patience to split the "these need squashing
> in" into multiple pieces and add them to appropriate commits in the
> 5 patch series (counting Peff's two fixes on top of which you built
> your 3 patch series) correctly.  You as the original author are much
> better equipped to do so than I am, so I'd appreciate if you can
> reroll them as a 5-patch series.
> 
> Among the changes, the fix to builtin/clone.c needs to be squashed
> into your "clone: do not include authentication data in guessed
> dir", and the remainder of the patch to t5603 should go to Peff's
> "clone: add tests for output directory", but with most of them
> marked initially as failing.  And then as you fix them in your
> patches, some of the "fail" mark should be dropped.
> 
> Thanks.

No problem, just wanted to make sure on how to proceed.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v5 0/5] Improve guessing of repository names

[Patrick Steinhardt]

This is version 5 of my patch series which aims to improve
guessed directory names in several cases.

This version now includes the patches from Jeff which were
previously a separate patch series. Besides fixing behavior when
cloning a repository with trailing '.git' suffix they also add a
new test suite that verifies guessed directory names. I've
amended 'clone: add tests for output directory' to add additional
tests (as proposed by Junio).

Changes to my own patches include improved commit messages
detailing why certain changes are done the way they are done and
a change to authentication-data-stripping, such that we now strip
greedily until the last '@' sign in the host part (proposed by
Junio, as well).

Jeff King (2):
  clone: add tests for output directory
  clone: use computed length in guess_dir_name

Patrick Steinhardt (3):
  clone: do not include authentication data in guessed dir
  clone: do not use port number as dir name
  clone: abort if no dir name could be guessed

 builtin/clone.c          |  65 +++++++++++++++++++++++++-----
 t/t5603-clone-dirname.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 157 insertions(+), 11 deletions(-)
 create mode 100755 t/t5603-clone-dirname.sh

-- 
2.5.0

[PATCH v5 1/5] clone: add tests for output directory

[Patrick Steinhardt]

From: Jeff King <peff@peff.net>

When we run "git clone $url", clone guesses from the $url
what to name the local output directory. We don't have any
test coverage of this, so let's add some basic tests.

This reveals a few problems:

  - cloning "foo.git/" does not properly remove the ".git";
    this is a recent regression from 7e837c6 (clone:
    simplify string handling in guess_dir_name(), 2015-07-09)

  - likewise, cloning foo/.git does not seem to handle the
    bare case (we should end up in foo.git, but we try to
    use foo/.git on the local end), which also comes from
    7e837c6.

  - cloning the root is not very smart about URL parsing,
    and usernames and port numbers may end up in the
    directory name

All of these tests are marked as failures.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5603-clone-dirname.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100755 t/t5603-clone-dirname.sh

diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
new file mode 100755
index 0000000..b009a87
--- /dev/null
+++ b/t/t5603-clone-dirname.sh
@@ -0,0 +1,103 @@
+#!/bin/sh
+
+test_description='check output directory names used by git-clone'
+. ./test-lib.sh
+
+# we use a fake ssh wrapper that ignores the arguments
+# entirely; we really only care that we get _some_ repo,
+# as the real test is what clone does on the local side
+test_expect_success 'setup ssh wrapper' '
+	write_script "$TRASH_DIRECTORY/ssh-wrapper" <<-\EOF &&
+	git upload-pack "$TRASH_DIRECTORY"
+	EOF
+	GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" &&
+	export GIT_SSH &&
+	export TRASH_DIRECTORY
+'
+
+# make sure that cloning $1 results in local directory $2
+test_clone_dir () {
+	url=$1; shift
+	dir=$1; shift
+	expect=success
+	bare=non-bare
+	clone_opts=
+	for i in "$@"; do
+		case "$i" in
+		fail)
+			expect=failure
+			;;
+		bare)
+			bare=bare
+			clone_opts=--bare
+			;;
+		esac
+	done
+	test_expect_$expect "clone of $url goes to $dir ($bare)" "
+		rm -rf $dir &&
+		git clone $clone_opts $url &&
+		test_path_is_dir $dir
+	"
+}
+
+# basic syntax with bare and non-bare variants
+test_clone_dir host:foo foo
+test_clone_dir host:foo foo.git bare
+test_clone_dir host:foo.git foo
+test_clone_dir host:foo.git foo.git bare
+test_clone_dir host:foo/.git foo
+test_clone_dir host:foo/.git foo.git bare fail
+
+# similar, but using ssh URL rather than host:path syntax
+test_clone_dir ssh://host/foo foo
+test_clone_dir ssh://host/foo foo.git bare
+test_clone_dir ssh://host/foo.git foo
+test_clone_dir ssh://host/foo.git foo.git bare
+test_clone_dir ssh://host/foo/.git foo
+test_clone_dir ssh://host/foo/.git foo.git bare fail
+
+# we should remove trailing slashes and .git suffixes
+test_clone_dir ssh://host/foo/ foo
+test_clone_dir ssh://host/foo/// foo
+test_clone_dir ssh://host/foo.git/ foo fail
+test_clone_dir ssh://host/foo.git/// foo fail
+test_clone_dir ssh://host/foo///.git/ foo
+test_clone_dir ssh://host/foo/.git/// foo
+
+test_clone_dir host:foo/ foo
+test_clone_dir host:foo/// foo
+test_clone_dir host:foo.git/ foo fail
+test_clone_dir host:foo.git/// foo fail
+test_clone_dir host:foo///.git/ foo
+test_clone_dir host:foo/.git/// foo
+
+# omitting the path should default to the hostname
+test_clone_dir ssh://host/ host
+test_clone_dir ssh://host:1234/ host fail
+test_clone_dir ssh://user@host/ host fail
+test_clone_dir host:/ host fail
+
+# auth materials should be redacted
+test_clone_dir ssh://user:password@host/ host fail
+test_clone_dir ssh://user:password@host:1234/ host fail
+test_clone_dir ssh://user:passw@rd@host:1234/ host fail
+test_clone_dir user@host:/ host fail
+test_clone_dir user:password@host:/ host fail
+test_clone_dir user:pass@wrd@host:/ host fail
+
+# auth-like material should not be dropped
+test_clone_dir ssh://host/foo@bar foo@bar
+test_clone_dir ssh://host/foo@bar.git foo@bar
+test_clone_dir ssh://user:password@host/foo@bar foo@bar
+test_clone_dir ssh://user:passw@rd@host/foo@bar.git foo@bar
+
+test_clone_dir host:/foo@bar foo@bar
+test_clone_dir host:/foo@bar.git foo@bar
+test_clone_dir user:password@host:/foo@bar foo@bar
+test_clone_dir user:passw@rd@host:/foo@bar.git foo@bar
+
+# trailing port-like numbers should not be stripped for paths
+test_clone_dir ssh://user:password@host/test:1234 1234
+test_clone_dir ssh://user:password@host/test:1234.git 1234
+
+test_done
-- 
2.5.0

[PATCH v5 2/5] clone: use computed length in guess_dir_name

[Patrick Steinhardt]

From: Jeff King <peff@peff.net>

Commit 7e837c6 (clone: simplify string handling in
guess_dir_name(), 2015-07-09) changed clone to use
strip_suffix instead of hand-rolled pointer manipulation.
However, strip_suffix will strip from the end of a
NUL-terminated string, and we may have already stripped some
characters (like directory separators, or "/.git"). This
leads to commands like:

  git clone host:foo.git/

failing to strip the ".git".

We must instead convert our pointer arithmetic into a
computed length and feed that to strip_suffix_mem, which will
then reduce the length further for us.

It would be nicer if we could drop the pointer manipulation
entirely, and just continually strip using strip_suffix. But
that doesn't quite work for two reasons:

  1. The early suffixes we're stripping are not constant; we
     need to look for is_dir_sep, which could be one of
     several characters.

  2. Mid-way through the stripping we compute the pointer
     "start", which shows us the beginning of the pathname.
     Which really give us two lengths to work with: the
     offset from the start of the string, and from the start
     of the path. By using pointers for the early part, we
     can just compute the length from "start" when we need
     it.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c          |  3 ++-
 t/t5603-clone-dirname.sh | 12 ++++++------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 303a3a7..bf45199 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -174,7 +174,8 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	/*
 	 * Strip .{bundle,git}.
 	 */
-	strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len);
+	len = end - start;
+	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");
 
 	if (is_bare)
 		dir = xstrfmt("%.*s.git", (int)len, start);
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index b009a87..10f5d09 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -46,7 +46,7 @@ test_clone_dir host:foo foo.git bare
 test_clone_dir host:foo.git foo
 test_clone_dir host:foo.git foo.git bare
 test_clone_dir host:foo/.git foo
-test_clone_dir host:foo/.git foo.git bare fail
+test_clone_dir host:foo/.git foo.git bare
 
 # similar, but using ssh URL rather than host:path syntax
 test_clone_dir ssh://host/foo foo
@@ -54,20 +54,20 @@ test_clone_dir ssh://host/foo foo.git bare
 test_clone_dir ssh://host/foo.git foo
 test_clone_dir ssh://host/foo.git foo.git bare
 test_clone_dir ssh://host/foo/.git foo
-test_clone_dir ssh://host/foo/.git foo.git bare fail
+test_clone_dir ssh://host/foo/.git foo.git bare
 
 # we should remove trailing slashes and .git suffixes
 test_clone_dir ssh://host/foo/ foo
 test_clone_dir ssh://host/foo/// foo
-test_clone_dir ssh://host/foo.git/ foo fail
-test_clone_dir ssh://host/foo.git/// foo fail
+test_clone_dir ssh://host/foo.git/ foo
+test_clone_dir ssh://host/foo.git/// foo
 test_clone_dir ssh://host/foo///.git/ foo
 test_clone_dir ssh://host/foo/.git/// foo
 
 test_clone_dir host:foo/ foo
 test_clone_dir host:foo/// foo
-test_clone_dir host:foo.git/ foo fail
-test_clone_dir host:foo.git/// foo fail
+test_clone_dir host:foo.git/ foo
+test_clone_dir host:foo.git/// foo
 test_clone_dir host:foo///.git/ foo
 test_clone_dir host:foo/.git/// foo
 
-- 
2.5.0

[PATCH v5 3/5] clone: do not include authentication data in guessed dir

[Patrick Steinhardt]

If the URI contains authentication data and the URI's path
component is empty we fail to guess a sensible directory name.
E.g. cloning a repository 'ssh://user:password@example.com/' we
guess a directory name 'password@example.com' where we would want
the hostname only, e.g. 'example.com'.

The naive way of just adding '@' as a path separator would break
cloning repositories like 'foo/bar@baz.git' (which would
currently become 'bar@baz' but would then become 'baz' only).
Instead fix this by first dropping the scheme and then greedily
scanning for an '@' sign until we find the first path separator.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c          | 41 +++++++++++++++++++++++++++++++----------
 t/t5603-clone-dirname.sh |  4 ++--
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index bf45199..da51792 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -146,30 +146,51 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
-	const char *end = repo + strlen(repo), *start;
+	const char *end = repo + strlen(repo), *start, *ptr;
 	size_t len;
 	char *dir;
 
 	/*
+	 * Skip scheme.
+	 */
+	start = strstr(repo, "://");
+	if (start == NULL)
+		start = repo;
+	else
+		start += 3;
+
+	/*
+	 * Skip authentication data. The stripping does happen
+	 * greedily, such that we strip up to the last '@' inside
+	 * the host part.
+	 */
+	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
+		if (*ptr == '@')
+			start = ptr + 1;
+	}
+
+	/*
 	 * Strip trailing spaces, slashes and /.git
 	 */
-	while (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
+	while (start < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
 		end--;
-	if (end - repo > 5 && is_dir_sep(end[-5]) &&
+	if (end - start > 5 && is_dir_sep(end[-5]) &&
 	    !strncmp(end - 4, ".git", 4)) {
 		end -= 5;
-		while (repo < end && is_dir_sep(end[-1]))
+		while (start < end && is_dir_sep(end[-1]))
 			end--;
 	}
 
 	/*
-	 * 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.
+	 * Find last component. To remain backwards compatible we
+	 * also regard colons as path separators, such that
+	 * cloning a repository 'foo:bar.git' would result in a
+	 * directory 'bar' being guessed.
 	 */
-	start = end;
-	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
-		start--;
+	ptr = end;
+	while (start < ptr && !is_dir_sep(ptr[-1]) && ptr[-1] != ':')
+		ptr--;
+	start = ptr;
 
 	/*
 	 * Strip .{bundle,git}.
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index 10f5d09..a9aba72 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -74,11 +74,11 @@ test_clone_dir host:foo/.git/// foo
 # omitting the path should default to the hostname
 test_clone_dir ssh://host/ host
 test_clone_dir ssh://host:1234/ host fail
-test_clone_dir ssh://user@host/ host fail
+test_clone_dir ssh://user@host/ host
 test_clone_dir host:/ host fail
 
 # auth materials should be redacted
-test_clone_dir ssh://user:password@host/ host fail
+test_clone_dir ssh://user:password@host/ host
 test_clone_dir ssh://user:password@host:1234/ host fail
 test_clone_dir ssh://user:passw@rd@host:1234/ host fail
 test_clone_dir user@host:/ host fail
-- 
2.5.0

[PATCH v5 4/5] clone: do not use port number as dir name

[Patrick Steinhardt]

If the URI contains a port number and the URI's path component is
empty we fail to guess a sensible directory name. E.g. cloning a
repository 'ssh://example.com:2222/' we guess a directory name
'2222' where we would want the hostname only, e.g. 'example.com'.

We need to take care to not drop trailing port-like numbers in
certain cases. E.g. when cloning a repository 'foo/bar:2222.git'
we want to guess the directory name '2222' instead of 'bar'.
Thus, we have to first check the stripped URI for path separators
and only strip port numbers if there are path separators present.
This heuristic breaks when cloning a repository 'bar:2222.git',
though.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c          | 17 +++++++++++++++++
 t/t5603-clone-dirname.sh | 14 +++++++-------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index da51792..e7f16ff 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -182,6 +182,23 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	}
 
 	/*
+	 * Strip trailing port number if we've got only a
+	 * hostname (that is, there is no dir separator but a
+	 * colon). This check is required such that we do not
+	 * strip URI's like '/foo/bar:2222.git', which should
+	 * result in a dir '2222' being guessed due to backwards
+	 * compatibility.
+	 */
+	if (memchr(start, '/', end - start) == NULL
+	    && memchr(start, ':', end - start) != NULL) {
+		ptr = end;
+		while (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')
+			ptr--;
+		if (start < ptr && ptr[-1] == ':')
+			end = ptr - 1;
+	}
+
+	/*
 	 * Find last component. To remain backwards compatible we
 	 * also regard colons as path separators, such that
 	 * cloning a repository 'foo:bar.git' would result in a
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index a9aba72..664d0ab 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -73,17 +73,17 @@ test_clone_dir host:foo/.git/// foo
 
 # omitting the path should default to the hostname
 test_clone_dir ssh://host/ host
-test_clone_dir ssh://host:1234/ host fail
+test_clone_dir ssh://host:1234/ host
 test_clone_dir ssh://user@host/ host
-test_clone_dir host:/ host fail
+test_clone_dir host:/ host
 
 # auth materials should be redacted
 test_clone_dir ssh://user:password@host/ host
-test_clone_dir ssh://user:password@host:1234/ host fail
-test_clone_dir ssh://user:passw@rd@host:1234/ host fail
-test_clone_dir user@host:/ host fail
-test_clone_dir user:password@host:/ host fail
-test_clone_dir user:pass@wrd@host:/ host fail
+test_clone_dir ssh://user:password@host:1234/ host
+test_clone_dir ssh://user:passw@rd@host:1234/ host
+test_clone_dir user@host:/ host
+test_clone_dir user:password@host:/ host
+test_clone_dir user:pass@wrd@host:/ host
 
 # auth-like material should not be dropped
 test_clone_dir ssh://host/foo@bar foo@bar
-- 
2.5.0

[PATCH v5 5/5] clone: abort if no dir name could be guessed

[Patrick Steinhardt]

Due to various components of the URI being stripped off it may
happen that we fail to guess a directory name. We currently error
out with a message that it is impossible to create the working
tree '' in such cases. Instead, error out early with a sensible
error message hinting that a directory name should be specified
manually on the command line.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index e7f16ff..5169746 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -215,6 +215,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	len = end - start;
 	strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git");
 
+	if (!len || (len == 1 && *start == '/'))
+	    die("No directory name could be guessed.\n"
+		"Please specify a directory on the command line");
+
 	if (is_bare)
 		dir = xstrfmt("%.*s.git", (int)len, start);
 	else
-- 
2.5.0

Re: [PATCH v5 0/5] Improve guessing of repository names

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> This is version 5 of my patch series which aims to improve
> guessed directory names in several cases.
>
> This version now includes the patches from Jeff which were
> previously a separate patch series. Besides fixing behavior when
> cloning a repository with trailing '.git' suffix they also add a
> new test suite that verifies guessed directory names. I've
> amended 'clone: add tests for output directory' to add additional
> tests (as proposed by Junio).
>
> Changes to my own patches include improved commit messages
> detailing why certain changes are done the way they are done and
> a change to authentication-data-stripping, such that we now strip
> greedily until the last '@' sign in the host part (proposed by
> Junio, as well).
>
> Jeff King (2):
>   clone: add tests for output directory
>   clone: use computed length in guess_dir_name
>
> Patrick Steinhardt (3):
>   clone: do not include authentication data in guessed dir
>   clone: do not use port number as dir name
>   clone: abort if no dir name could be guessed
>
>  builtin/clone.c          |  65 +++++++++++++++++++++++++-----
>  t/t5603-clone-dirname.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 157 insertions(+), 11 deletions(-)
>  create mode 100755 t/t5603-clone-dirname.sh

Looks good.

I'll forge your sign-off for two patches from Peff and queue the
whole thing.

Thanks.