{"thread":{"id":"39942","subject":"[PATCH] clone: fix repo name when cloning a server's root","startedAt":"2015-07-27T11:48:28Z","lastAt":"2015-08-10T18:07:16Z","messageCount":53,"participants":["Patrick Steinhardt","Duy Nguyen","Junio C Hamano","Eric Sunshine","Jeff King","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"266822","messageId":"1437997708-10732-1-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":null,"subject":"[PATCH] clone: fix repo name when cloning a server's root","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-27T11:48:28Z","receivedAt":"2015-07-27T11:48:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When cloning a repository from a server's root, that is the URL's\npath component is a '/' only, we fail to generate a sensible\nrepository name when the URL contains authentication data. This\nis especially bad when cloning URLs like\n'ssh://user:passwd@example.com/', which results in a repository\n'passwd@example.com' being created.\n\nImprove the behavior by also regarding '@'-signs as a separator\nwhen scanning the URL. In the mentioned case this would instead\nresult in a directory 'example.com' being created.\n\nSigned-off-by: Patrick Steinhardt \n---\nI was not able to come by with a useful test as that would\nrequire being able to clone a root directory. I couldn't find\nanything in the current tests that looks like what I want to do.\nDoes anybody have an idea on how to achieve this?\n\n builtin/clone.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a72ff7e..aaf38b2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -164,11 +164,13 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \n \t/*\n \t * Find last component, but be prepared that repo could have\n-\t * the form \"remote.example.com:foo.git\", i.e. no slash\n-\t * in the directory part.\n+\t * the form \"remote.example.com:foo.git\", i.e. no slash in\n+\t * the directory part, or \"user:password@remote.example.com/\",\n+\t * that is the path component may be empty.\n \t */\n \tstart = end;\n-\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n+\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':'\n+\t\t\t&& start[-1] != '@')\n \t\tstart--;\n \n \t/*\n-- \n2.4.6\n"},{"id":"266828","messageId":"CACsJy8C0xzTvxMo7=pLFJyu8Yaqm_nz1qVXMFFo9ubQE-dYYSw@mail.gmail.com","threadId":"39942","inReplyTo":"1437997708-10732-1-git-send-email-ps@pks.im","subject":"Re: [PATCH] clone: fix repo name when cloning a server's root","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-07-27T12:51:30Z","receivedAt":"2015-07-27T12:51:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 27, 2015 at 6:48 PM, Patrick Steinhardt wrote:\n> When cloning a repository from a server's root, that is the URL's\n> path component is a '/' only, we fail to generate a sensible\n> repository name when the URL contains authentication data. This\n> is especially bad when cloning URLs like\n> 'ssh://user:passwd@example.com/', which results in a repository\n> 'passwd@example.com' being created.\n>\n> Improve the behavior by also regarding '@'-signs as a separator\n> when scanning the URL. In the mentioned case this would instead\n> result in a directory 'example.com' being created.\n\nMy initial reaction was, if you put password on the command line, you\ndeserve it. However, as we improve this heuristics, perhaps it's\nbetter to export parse_connect_url() from connect.c and use it here?\nWe would have more robust parsing. You can create a repo named\nexample.com given the url ssh://user:pass@example.com:123/. Maybe it's\noverkill?\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> I was not able to come by with a useful test as that would\n> require being able to clone a root directory. I couldn't find\n> anything in the current tests that looks like what I want to do.\n> Does anybody have an idea on how to achieve this?\n\nThere's t/t1509/prepare-chroot.sh that will prepare a chroot for this\npurpose. You'll need linux, busybox and chroot permission.\n-- \nDuy\n"},{"id":"266830","messageId":"20150727125923.GA800@pks-xps","threadId":"39942","inReplyTo":"CACsJy8C0xzTvxMo7=pLFJyu8Yaqm_nz1qVXMFFo9ubQE-dYYSw@mail.gmail.com","subject":"Re: [PATCH] clone: fix repo name when cloning a server's root","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-27T12:59:23Z","receivedAt":"2015-07-27T12:59:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 27, 2015 at 07:51:30PM +0700, Duy Nguyen wrote:\n> On Mon, Jul 27, 2015 at 6:48 PM, Patrick Steinhardt wrote:\n> > When cloning a repository from a server's root, that is the URL's\n> > path component is a '/' only, we fail to generate a sensible\n> > repository name when the URL contains authentication data. This\n> > is especially bad when cloning URLs like\n> > 'ssh://user:passwd@example.com/', which results in a repository\n> > 'passwd@example.com' being created.\n> >\n> > Improve the behavior by also regarding '@'-signs as a separator\n> > when scanning the URL. In the mentioned case this would instead\n> > result in a directory 'example.com' being created.\n> \n> My initial reaction was, if you put password on the command line, you\n> deserve it. However, as we improve this heuristics, perhaps it's\n> better to export parse_connect_url() from connect.c and use it here?\n> We would have more robust parsing. You can create a repo named\n> example.com given the url ssh://user:pass@example.com:123/. Maybe it's\n> overkill?\n\nSure, specifying passwords on command line should not be done\neasily. Still those heuristics fail for everything that does\nnot include an additional [:/] when the URL's path is empty. So I\nguess using parse_connect_url() would be the most sane solution\nfor this, as it will also fix the case when you specify a port,\nwhich would currently use the port as directory name. I'll whip\nup a new version that uses parse_connect_url().\n\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > I was not able to come by with a useful test as that would\n> > require being able to clone a root directory. I couldn't find\n> > anything in the current tests that looks like what I want to do.\n> > Does anybody have an idea on how to achieve this?\n> \n> There's t/t1509/prepare-chroot.sh that will prepare a chroot for this\n> purpose. You'll need linux, busybox and chroot permission.\n\nThanks for the hint.\n\nPatrick\n"},{"id":"266834","messageId":"xmqqzj2hish3.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"CACsJy8C0xzTvxMo7=pLFJyu8Yaqm_nz1qVXMFFo9ubQE-dYYSw@mail.gmail.com","subject":"Re: [PATCH] clone: fix repo name when cloning a server's root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-27T14:29:44Z","receivedAt":"2015-07-27T14:29:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> I was not able to come by with a useful test as that would\n>> require being able to clone a root directory. I couldn't find\n>> anything in the current tests that looks like what I want to do.\n>> Does anybody have an idea on how to achieve this?\n>\n> There's t/t1509/prepare-chroot.sh that will prepare a chroot for this\n> purpose. You'll need linux, busybox and chroot permission.\n\nIf you do not use ssh:// or file://, it should be trivial to\narrange, no? http://site/repo will typically not be serving\n/repo from the root of the filesystem.\n"},{"id":"267051","messageId":"1438185076-28870-1-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1437997708-10732-1-git-send-email-ps@pks.im","subject":"[PATCH v2 0/6] clone: fix repo name when cloning a server's root","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-29T15:51:10Z","receivedAt":"2015-07-29T15:51:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"As proposed the previous patch has been changed such that we now\nutilize parse_connect_url(), which splits a connect URL by host\nand path components. If the path component is empty we now try to\nfall back to the host component, stripping it of its port and\nauthentication information.\n\nThe first two patches fix the t1509-root-worktree tests. They\ncurrently were not working due to broken &&-chains and are\nrequired to test cloning from /.\n\nThe next two patches expose parse_connect_url(), which is then\nused in the fifth patch. The last patch provides tests for the\nbehavior. To keep it simple we just use file:// URIs, which have\nthe option of including a host in there, e.g.\nfile://127.0.0.1/foobar.\n\nPatrick Steinhardt (6):\n tests: fix broken && chains in t1509-root-worktree\n tests: fix cleanup after tests in t1509-root-worktree\n connect: expose parse_connect_url()\n connect: move error check to caller of parse_connect_url\n clone: fix hostname parsing when guessing dir\n clone: add tests for cloning / without workdir name\n\n builtin/clone.c | 73 +++++++++++++++++++++++++++++++++++-------------\n connect.c | 19 +++----------\n connect.h | 13 +++++++++\n t/t1509-root-worktree.sh | 43 +++++++++++++++++++++++++---\n 4 files changed, 109 insertions(+), 39 deletions(-)\n\n-- \n2.5.0\n"},{"id":"267054","messageId":"1438185076-28870-2-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438185076-28870-1-git-send-email-ps@pks.im","subject":"[PATCH v2 1/6] tests: fix broken && chains in t1509-root-worktree","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-29T15:51:11Z","receivedAt":"2015-07-29T15:51:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Signed-off-by: Patrick Steinhardt \n---\n t/t1509-root-worktree.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\nindex b6977d4..0c80129 100755\n--- a/t/t1509-root-worktree.sh\n+++ b/t/t1509-root-worktree.sh\n@@ -125,7 +125,7 @@ fi\n ONE_SHA1=d00491fd7e5bb6fa28c517a0bb32b8b506539d4d\n \n test_expect_success 'setup' '\n-\trm -rf /foo\n+\trm -rf /foo &&\n \tmkdir /foo &&\n \tmkdir /foo/bar &&\n \techo 1 > /foo/foome &&\n@@ -218,7 +218,7 @@ unset GIT_WORK_TREE\n \n test_expect_success 'go to /' 'cd /'\n test_expect_success 'setup' '\n-\trm -rf /.git\n+\trm -rf /.git &&\n \techo \"Initialized empty Git repository in /.git/\" > expected &&\n \tgit init > result &&\n \ttest_cmp expected result\n@@ -241,8 +241,8 @@ say \"auto bare gitdir\"\n \n # DESTROYYYYY!!!!!\n test_expect_success 'setup' '\n-\trm -rf /refs /objects /info /hooks\n-\trm /*\n+\trm -rf /refs /objects /info /hooks &&\n+\trm /* &&\n \tcd / &&\n \techo \"Initialized empty Git repository in /\" > expected &&\n \tgit init --bare > result &&\n-- \n2.5.0\n"},{"id":"267056","messageId":"1438185076-28870-3-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438185076-28870-1-git-send-email-ps@pks.im","subject":"[PATCH v2 2/6] tests: fix cleanup after tests in t1509-root-worktree","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-29T15:51:12Z","receivedAt":"2015-07-29T15:51:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"During cleanup we do a simple 'rm /*' to remove leftover files\nfrom previous tests. As 'rm' errors out when there is anything it\ncannot delete and there are directories present at '/' it will\nthrow an error, causing the '&&' chain to fail.\n\nFix this by explicitly removing the files.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1509-root-worktree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\nindex 0c80129..553a3f6 100755\n--- a/t/t1509-root-worktree.sh\n+++ b/t/t1509-root-worktree.sh\n@@ -242,7 +242,7 @@ say \"auto bare gitdir\"\n # DESTROYYYYY!!!!!\n test_expect_success 'setup' '\n \trm -rf /refs /objects /info /hooks &&\n-\trm /* &&\n+\trm -f /expected /ls.expected /me /result &&\n \tcd / &&\n \techo \"Initialized empty Git repository in /\" > expected &&\n \tgit init --bare > result &&\n-- \n2.5.0\n"},{"id":"267053","messageId":"1438185076-28870-4-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438185076-28870-1-git-send-email-ps@pks.im","subject":"[PATCH v2 3/6] connect: expose parse_connect_url()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-29T15:51:13Z","receivedAt":"2015-07-29T15:51:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Expose parse_connect_url which is to be used later in this patch\nseries.\n\nSigned-off-by: Patrick Steinhardt \n---\n connect.c | 13 +------------\n connect.h | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c0144d8..bdbcee4 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -228,13 +228,6 @@ int server_supports(const char *feature)\n \treturn !!server_feature_value(feature, NULL);\n }\n \n-enum protocol {\n-\tPROTO_LOCAL = 1,\n-\tPROTO_FILE,\n-\tPROTO_SSH,\n-\tPROTO_GIT\n-};\n-\n int url_is_local_not_ssh(const char *url)\n {\n \tconst char *colon = strchr(url, ':');\n@@ -580,11 +573,7 @@ static char *get_port(char *host)\n \treturn NULL;\n }\n \n-/*\n- * Extract protocol and relevant parts from the specified connection URL.\n- * The caller must free() the returned strings.\n- */\n-static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n+enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \t\t\t\t char **ret_path)\n {\n \tchar *url;\ndiff --git a/connect.h b/connect.h\nindex c41a685..245890f 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -11,4 +11,17 @@ extern int parse_feature_request(const char *features, const char *feature);\n extern const char *server_feature_value(const char *feature, int *len_ret);\n extern int url_is_local_not_ssh(const char *url);\n \n+enum protocol {\n+\tPROTO_LOCAL = 1,\n+\tPROTO_FILE,\n+\tPROTO_SSH,\n+\tPROTO_GIT\n+};\n+\n+/*\n+ * Extract protocol and relevant parts from the specified connection URL.\n+ * The caller must free() the returned strings.\n+ */\n+extern enum protocol parse_connect_url(const char *url_orig, char **ret_host, char **ret_path);\n+\n #endif\n-- \n2.5.0\n"},{"id":"267057","messageId":"1438185076-28870-5-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438185076-28870-1-git-send-email-ps@pks.im","subject":"[PATCH v2 4/6] connect: move error check to caller of parse_connect_url","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-29T15:51:14Z","receivedAt":"2015-07-29T15:51:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"parse_connect_url() checks if the path component of the URL is\nempty and if so causes the program to die. As the function is to\nbe used at other call sites which do not require this check, move\nup the error checking to the existing caller.\n\nSigned-off-by: Patrick Steinhardt \n---\n connect.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex bdbcee4..e8b813d 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -613,9 +613,6 @@ enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \telse\n \t\tpath = strchr(end, separator);\n \n-\tif (!path || !*path)\n-\t\tdie(\"No path specified. See 'man git-pull' for valid url syntax\");\n-\n \t/*\n \t * null-terminate hostname and point path to ~ for URL's like this:\n \t * ssh://host.xz/~user/repo\n@@ -665,6 +662,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tsignal(SIGCHLD, SIG_DFL);\n \n \tprotocol = parse_connect_url(url, &hostandport, &path);\n+\tif (!path || !*path)\n+\t\tdie(\"No path specified. See 'man git-pull' for valid url syntax\");\n+\n \tif ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\n \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n-- \n2.5.0\n"},{"id":"267052","messageId":"1438185076-28870-6-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438185076-28870-1-git-send-email-ps@pks.im","subject":"[PATCH v2 5/6] clone: fix hostname parsing when guessing dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-29T15:51:15Z","receivedAt":"2015-07-29T15:51:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We fail to guess a sensible directory name for a newly cloned\nrepository when the path component of the URL is empty. E.g.\ncloning a repository 'ssh://user:password@example.com/' we create\na directory 'password@example.com' for the clone.\n\nFix this by using parse_connect_url to split host and path\ncomponents and explicitly checking whether we need to fall back\nto the hostname for guessing a directory name.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 73 +++++++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 53 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a72ff7e..4547729 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -9,6 +9,7 @@\n */\n \n #include \"builtin.h\"\n+#include \"connect.h\"\n #include \"lockfile.h\"\n #include \"parse-options.h\"\n #include \"fetch-pack.h\"\n@@ -147,34 +148,62 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n \tconst char *end = repo + strlen(repo), *start;\n+\tchar *host, *path;\n \tsize_t len;\n \tchar *dir;\n \n+\tparse_connect_url(repo, &host, &path);\n \t/*\n-\t * Strip trailing spaces, slashes and /.git\n+\t * If the path component of the URL is empty (e.g. it is\n+\t * empty or only contains a '/') we fall back to the host\n+\t * name.\n \t */\n-\twhile (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n-\t\tend--;\n-\tif (end - repo > 5 && is_dir_sep(end[-5]) &&\n-\t !strncmp(end - 4, \".git\", 4)) {\n-\t\tend -= 5;\n-\t\twhile (repo < end && is_dir_sep(end[-1]))\n+\tif (!path || !*path || !strcmp(path, \"/\")) {\n+\t\tif (*host == '\\0')\n+\t\t\tdie(\"No directory name could be guessed.\\n\"\n+\t\t\t\t\"Please specify a directory on the command line\");\n+\t\t/*\n+\t\t * Strip authentication information if it exists.\n+\t\t */\n+\t\tstart = strchr(host, '@');\n+\t\tif (start)\n+\t\t\tstart++;\n+\t\telse\n+\t\t\tstart = host;\n+\t\t/*\n+\t\t * Strip port if it exitsts.\n+\t\t */\n+\t\tend = strchr(start, ':');\n+\t\tif (!end)\n+\t\t\tend = start + strlen(start);\n+\t\tlen = end - start;\n+\t} else {\n+\t\t/*\n+\t\t * Strip trailing spaces, slashes and /.git\n+\t\t */\n+\t\twhile (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n \t\t\tend--;\n-\t}\n+\t\tif (end - repo > 5 && is_dir_sep(end[-5]) &&\n+\t\t\t!strncmp(end - 4, \".git\", 4)) {\n+\t\t\tend -= 5;\n+\t\t\twhile (repo < end && is_dir_sep(end[-1]))\n+\t\t\t\tend--;\n+\t\t}\n \n-\t/*\n-\t * Find last component, but be prepared that repo could have\n-\t * the form \"remote.example.com:foo.git\", i.e. no slash\n-\t * in the directory part.\n-\t */\n-\tstart = end;\n-\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n-\t\tstart--;\n+\t\t/*\n+\t\t * Find last component, but be prepared that repo could have\n+\t\t * the form \"remote.example.com:foo.git\", i.e. no slash\n+\t\t * in the directory part.\n+\t\t */\n+\t\tstart = end;\n+\t\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n+\t\t\tstart--;\n \n-\t/*\n-\t * Strip .{bundle,git}.\n-\t */\n-\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n+\t\t/*\n+\t\t * Strip .{bundle,git}.\n+\t\t */\n+\t\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n+\t}\n \n \tif (is_bare)\n \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n@@ -203,6 +232,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t\tif (out > dir && prev_space)\n \t\t\tout[-1] = '\\0';\n \t}\n+\n+\tfree(host);\n+\tfree(path);\n+\n \treturn dir;\n }\n \n-- \n2.5.0\n"},{"id":"267055","messageId":"1438185076-28870-7-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438185076-28870-1-git-send-email-ps@pks.im","subject":"[PATCH v2 6/6] clone: add tests for cloning with empty path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-29T15:51:16Z","receivedAt":"2015-07-29T15:51:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Test behavior of `git clone` when working with an empty path\ncomponent. This may be the case when cloning a file system's root\ndirectory or from a remote server's root.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1509-root-worktree.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\nindex 553a3f6..acfa133 100755\n--- a/t/t1509-root-worktree.sh\n+++ b/t/t1509-root-worktree.sh\n@@ -237,6 +237,45 @@ test_foobar_foobar\n \n test_expect_success 'cleanup' 'rm -rf /.git'\n \n+say \"clone .git at root without reponame\"\n+\n+test_expect_success 'go to /' 'cd /'\n+test_expect_success 'setup' '\n+\techo \"Initialized empty Git repository in /.git/\" > expected &&\n+\tgit init > result &&\n+\ttest_cmp expected result\n+'\n+\n+test_clone_expect_dir() {\n+\tURL=\"$1\"\n+\tDIR=\"$2\"\n+\techo \"Cloning into '$DIR'...\" >expected\n+\techo \"warning: You appear to have cloned an empty repository.\" >>expected\n+\tgit clone \"$URL\" 2>result >result\n+\trm -r \"$DIR\"\n+\ttest_cmp expected result\n+}\n+\n+test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'\n+test_expect_success 'simple clone of /' '\n+\techo \"fatal: No directory name could be guessed.\" > expected &&\n+\techo \"Please specify a directory on the command line\" >> expected &&\n+\ttest_expect_code 128 git clone / 2>result >result &&\n+\ttest_cmp expected result'\n+\n+test_expect_success 'clone with file://' '\n+\ttest_clone_expect_dir file://127.0.0.1/ 127.0.0.1'\n+test_expect_success 'clone with file://user@' '\n+\ttest_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'\n+test_expect_success 'clone with file://user:password@' '\n+\ttest_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'\n+test_expect_success 'clone with file://:port' '\n+\ttest_clone_expect_dir file://127.0.0.1:9999/ 127.0.0.1'\n+test_expect_success 'clone with file://user:password@:port' '\n+\ttest_clone_expect_dir file://user:password@127.0.0.1:9999/ 127.0.0.1'\n+\n+test_expect_success 'cleanup' 'rm -rf /.git /clones'\n+\n say \"auto bare gitdir\"\n \n # DESTROYYYYY!!!!!\n-- \n2.5.0\n"},{"id":"267074","messageId":"xmqq7fpiamiq.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"1438185076-28870-6-git-send-email-ps@pks.im","subject":"Re: [PATCH v2 5/6] clone: fix hostname parsing when guessing dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-29T17:42:21Z","receivedAt":"2015-07-29T17:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We fail to guess a sensible directory name for a newly cloned\n> repository when the path component of the URL is empty. E.g.\n> cloning a repository 'ssh://user:password@example.com/' we create\n> a directory 'password@example.com' for the clone.\n>\n> Fix this by ...\n\nIt is clear that you do not want to have that password in the\nresulting directory name from the problem description, but you\nstarted saying \"Fix this\" without saying what the desired outcome\nis. \"We want to use only the hostname, e.g. 'example.com', in such\na case instead.\" or something, perhaps, at the end of the first\nparagraph? \"Fix this by doing such and such\" becomes understandable\nonly after we know what end result you want to achieve by \"doing\nsuch and such\".\n\n> ... using parse_connect_url to split host and path\n> components and explicitly checking whether we need to fall back\n> to the hostname for guessing a directory name.\n\nI cannot help wonder why this much change (including patches 3 and\n4) is needed. Isn't it just the matter of making this part of the\nexisting code be aware of '@' in addition to ':'?\n\n> -\t/*\n> -\t * Find last component, but be prepared that repo could have\n> -\t * the form \"remote.example.com:foo.git\", i.e. no slash\n> -\t * in the directory part.\n> -\t */\n> -\tstart = end;\n> -\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n> -\t\tstart--;\n\nRegardless of the issue you are trying to address, we may want to\nlimit that \"be prepared for and careful with ':'\" logic in the\nexisting code to the case where the \"last component\" does not have\nany other component before it. That is:\n\n\thttp://example.com/foo:bar.git/\n\nwould be stripped to\n\n\thttp://example.com/foo:bar\n\nand then we scan backwards for ':' or '/' and declare that \"bar\" is\nthe name of the repository, but we would probably want \"foo:bar\"\ninstead (or we may not, as some filesystems do not want to have a\ncolon in its path components).\n\n\n builtin/clone.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a72ff7e..3d6328a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -165,10 +165,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t/*\n \t * Find last component, but be prepared that repo could have\n \t * the form \"remote.example.com:foo.git\", i.e. no slash\n-\t * in the directory part.\n+\t * in the directory part, or \"http://user@example.com/\" with\n+\t * the whole site serving a single repository.\n \t */\n \tstart = end;\n-\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n+\twhile (repo < start &&\n+\t !(is_dir_sep(start[-1]) || start[-1] == ':' || start[-1] == '@'))\n \t\tstart--;\n \n \t/*\n"},{"id":"267103","messageId":"CAPig+cRGw6-QhYzxkqVE-psQLHORioGmNFbf3Q72nrsSiES5GA@mail.gmail.com","threadId":"39942","inReplyTo":"1438185076-28870-5-git-send-email-ps@pks.im","subject":"Re: [PATCH v2 4/6] connect: move error check to caller of parse_connect_url","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-29T20:32:13Z","receivedAt":"2015-07-29T20:32:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt wrote:\n> parse_connect_url() checks if the path component of the URL is\n> empty and if so causes the program to die. As the function is to\n> be used at other call sites which do not require this check, move\n> up the error checking to the existing caller.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/connect.c b/connect.c\n> index bdbcee4..e8b813d 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -613,9 +613,6 @@ enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n> else\n> path = strchr(end, separator);\n>\n> - if (!path || !*path)\n> - die(\"No path specified. See 'man git-pull' for valid url syntax\");\n> -\n\nGiven that there are several dereferences of 'path' following this bit\nof code, won't this change lead to a crash when path==NULL?\n\n> /*\n> * null-terminate hostname and point path to ~ for URL's like this:\n> * ssh://host.xz/~user/repo\n> @@ -665,6 +662,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n> signal(SIGCHLD, SIG_DFL);\n>\n> protocol = parse_connect_url(url, &hostandport, &path);\n> + if (!path || !*path)\n> + die(\"No path specified. See 'man git-pull' for valid url syntax\");\n> +\n> if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\n> printf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n> printf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n> --\n> 2.5.0\n"},{"id":"267136","messageId":"20150730121811.GA24635@pks-pc.localdomain","threadId":"39942","inReplyTo":"xmqq7fpiamiq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 5/6] clone: fix hostname parsing when guessing dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-30T12:18:11Z","receivedAt":"2015-07-30T12:18:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 29, 2015 at 10:42:21AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > We fail to guess a sensible directory name for a newly cloned\n> > repository when the path component of the URL is empty. E.g.\n> > cloning a repository 'ssh://user:password@example.com/' we create\n> > a directory 'password@example.com' for the clone.\n> >\n> > Fix this by ...\n> \n> It is clear that you do not want to have that password in the\n> resulting directory name from the problem description, but you\n> started saying \"Fix this\" without saying what the desired outcome\n> is. \"We want to use only the hostname, e.g. 'example.com', in such\n> a case instead.\" or something, perhaps, at the end of the first\n> paragraph? \"Fix this by doing such and such\" becomes understandable\n> only after we know what end result you want to achieve by \"doing\n> such and such\".\n\nAgreed, will fix with the next iteration.\n\n> > ... using parse_connect_url to split host and path\n> > components and explicitly checking whether we need to fall back\n> > to the hostname for guessing a directory name.\n> \n> I cannot help wonder why this much change (including patches 3 and\n> 4) is needed. Isn't it just the matter of making this part of the\n> existing code be aware of '@' in addition to ':'?\n\nActually no, as host and path components need to be treated\ndifferently. See below.\n\n> > -\t/*\n> > -\t * Find last component, but be prepared that repo could have\n> > -\t * the form \"remote.example.com:foo.git\", i.e. no slash\n> > -\t * in the directory part.\n> > -\t */\n> > -\tstart = end;\n> > -\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n> > -\t\tstart--;\n> \n> Regardless of the issue you are trying to address, we may want to\n> limit that \"be prepared for and careful with ':'\" logic in the\n> existing code to the case where the \"last component\" does not have\n> any other component before it. That is:\n> \n> \thttp://example.com/foo:bar.git/\n> \n> would be stripped to\n> \n> \thttp://example.com/foo:bar\n> \n> and then we scan backwards for ':' or '/' and declare that \"bar\" is\n> the name of the repository, but we would probably want \"foo:bar\"\n> instead (or we may not, as some filesystems do not want to have a\n> colon in its path components).\n\nThis case is exactly why I did include patches 3 and 4. We've got\ntwo cases that need to be distinguished:\n\n1. we've got a non-empty path component (that is it contains more\n than just a '/'). In this case we want to take its last part\n and strip it of things like '.git'. We should only honor ':'\n as a path separator if it is the first character in the path\n component, otherwise only honor '/'.\n\n2. we've got an empty path component. In this case we want to\n inspect the host part. If it is empty we have to error out,\n otherwise we want to strip it of authentication information\n (everythin up to and including '@') and port information\n (everything following the ':').\n\nSo both cases are treated entirely different. For your example\nwe'd first split up 'http://example.com/foo:bar.git' into the\nhost 'example.com' and the path '/foo:bar.git'. As\n'parse_connect_url()' does exactly what we need, e.g. split up\nhost and path, I think it is only natural to reuse it.\n\nBut actually you are right, currently I still have the old logic\nin place that splits on colons in the path component. In my case\nit would be 'parse_connect_url()'s responsibility to detect if\nhost and path are not separated by '/' but by ':' and thus we'd\nnot run into the problem with 'foo:bar.git'. I'll verify that\nbehavior though and write some tests.\n\nPatrick\n"},{"id":"267137","messageId":"20150730121936.GB24635@pks-pc.localdomain","threadId":"39942","inReplyTo":"CAPig+cRGw6-QhYzxkqVE-psQLHORioGmNFbf3Q72nrsSiES5GA@mail.gmail.com","subject":"Re: [PATCH v2 4/6] connect: move error check to caller of parse_connect_url","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-30T12:19:36Z","receivedAt":"2015-07-30T12:19:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 29, 2015 at 04:32:13PM -0400, Eric Sunshine wrote:\n> On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt wrote:\n> > parse_connect_url() checks if the path component of the URL is\n> > empty and if so causes the program to die. As the function is to\n> > be used at other call sites which do not require this check, move\n> > up the error checking to the existing caller.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > diff --git a/connect.c b/connect.c\n> > index bdbcee4..e8b813d 100644\n> > --- a/connect.c\n> > +++ b/connect.c\n> > @@ -613,9 +613,6 @@ enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n> > else\n> > path = strchr(end, separator);\n> >\n> > - if (!path || !*path)\n> > - die(\"No path specified. See 'man git-pull' for valid url syntax\");\n> > -\n> \n> Given that there are several dereferences of 'path' following this bit\n> of code, won't this change lead to a crash when path==NULL?\n\nCorrect, will fix, thanks.\n\nPatrick\n"},{"id":"267169","messageId":"xmqqk2th7gmq.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"20150730121811.GA24635@pks-pc.localdomain","subject":"Re: [PATCH v2 5/6] clone: fix hostname parsing when guessing dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-30T16:30:05Z","receivedAt":"2015-07-30T16:30:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Regardless of the issue you are trying to address, we may want to\n>> limit that \"be prepared for and careful with ':'\" logic in the\n>> existing code to the case where the \"last component\" does not have\n>> any other component before it. That is:\n>> \n>> \thttp://example.com/foo:bar.git/\n>> \n>> would be stripped to\n>> \n>> \thttp://example.com/foo:bar\n>> \n>> and then we scan backwards for ':' or '/' and declare that \"bar\" is\n>> the name of the repository, but we would probably want \"foo:bar\"\n>> instead (or we may not, as some filesystems do not want to have a\n>> colon in its path components).\n>\n> This case is exactly why I did include patches 3 and 4.\n\nWell, but there is the above \"or we may not\" ;-)\n\n> But actually you are right, currently I still have the old logic\n> in place that splits on colons in the path component. \n\nYes. The reason why I suggested the simple route was exactly\nbecause I noticed that you didn't seem to care about the above\n\"$site/foo:bar.git/\" => \"$site/foo:bar\" => \"bar\" transform.\n\nAnd I think people might depend on that behaviour. \"Fixing\" that\nmay even be seen as a regression.\n\nWhen was the last time you created a foo@bar.git repository? Is it\nreally worth the code churn to do patches 3/4?\n"},{"id":"267170","messageId":"xmqqfv457fkc.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"xmqqk2th7gmq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 5/6] clone: fix hostname parsing when guessing dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-30T16:53:07Z","receivedAt":"2015-07-30T16:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Well, but there is the above \"or we may not\" ;-)\n>\n>> But actually you are right, currently I still have the old logic\n>> in place that splits on colons in the path component. \n>\n> Yes. The reason why I suggested the simple route was exactly\n> because I noticed that you didn't seem to care about the above\n> \"$site/foo:bar.git/\" => \"$site/foo:bar\" => \"bar\" transform.\n>\n> And I think people might depend on that behaviour. \"Fixing\" that\n> may even be seen as a regression.\n>\n> When was the last time you created a foo@bar.git repository?\n\nActually, this was an unrelated question and a wrong one to ask at\nthat.\n\nEven though I personally haven't created foo:bar.git repository,\nbecause it is no longer 2005, it is highly likely that somewhere\nthere is such a person who depends on the current behaviour of\nturning that to \"bar\" on the cloned side. Similarly, even if we the\npeople who read the Git mailing list collectively do not know\nanybody who has foo@bar.git repository, it is highly likely that\nsomewhere there is such a person who depends on the current\nbehaviour of turning that to \"foo@bar\" on the cloned side.\n\nSo the ideal would be to keep turning $site/foo:bar.git to bar,\n$site/foo@bar.git to foo@bar, and $scheme//u@p:$host/ to $host.\n\nAnd it would be ideal if we do so without much code churn. \"The\nwhole site is dedicated to host a single repository at the root\" is\na highly unlikely set-up and it feels wasteful to spend too much\neffort on.\n"},{"id":"267173","messageId":"CAPig+cQ5wR8xp8erAHBzEMEs62ddULrrML53GNVuuie3kd9EvA@mail.gmail.com","threadId":"39942","inReplyTo":"1438185076-28870-7-git-send-email-ps@pks.im","subject":"Re: [PATCH v2 6/6] clone: add tests for cloning with empty path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-30T18:18:02Z","receivedAt":"2015-07-30T18:18:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt wrote:\n> Test behavior of `git clone` when working with an empty path\n> component. This may be the case when cloning a file system's root\n> directory or from a remote server's root.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\n> index 553a3f6..acfa133 100755\n> --- a/t/t1509-root-worktree.sh\n> +++ b/t/t1509-root-worktree.sh\n> @@ -237,6 +237,45 @@ test_foobar_foobar\n>\n> test_expect_success 'cleanup' 'rm -rf /.git'\n>\n> +say \"clone .git at root without reponame\"\n> +\n> +test_expect_success 'go to /' 'cd /'\n> +test_expect_success 'setup' '\n> + echo \"Initialized empty Git repository in /.git/\" > expected &&\n> + git init > result &&\n> + test_cmp expected result\n> +'\n\nI'd say something here about current style omitting the space after\nthe redirection operator (>), however, this script uniformly includes\nthe space, and being consistent with the existing style is important,\nso I won't mention it. ;-)\n\n> +test_clone_expect_dir() {\n> + URL=\"$1\"\n> + DIR=\"$2\"\n> + echo \"Cloning into '$DIR'...\" >expected\n> + echo \"warning: You appear to have cloned an empty repository.\" >>expected\n\n echo >expected <<-\\EOF\n Cloning into...\n warning: You appear...\n EOF\n\nis more readable and maintainable.\n\n> + git clone \"$URL\" 2>result >result\n\n git clone \"$URL\" >result 2>&1\n\n> + rm -r \"$DIR\"\n> + test_cmp expected result\n> +}\n\nWhile not mandatory since it works as expected in its current form, it\nwould be nice to see a fully intact &&-chain in this function. That\nway, if someone some day adds code which doesn't impact 'result' but\nwhich might somehow fail, then the failure will be noticed.\n\n> +\n> +test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'\n> +test_expect_success 'simple clone of /' '\n> + echo \"fatal: No directory name could be guessed.\" > expected &&\n> + echo \"Please specify a directory on the command line\" >> expected &&\n\n cat >expected <<-\\EOF\n fatal: No directory...\n Please specify...\n EOF\n\n> + test_expect_code 128 git clone / 2>result >result &&\n\n test_expect_code 128 git clone / >result 2>&1 &&\n\n> + test_cmp expected result'\n> +\n> +test_expect_success 'clone with file://' '\n> + test_clone_expect_dir file://127.0.0.1/ 127.0.0.1'\n> +test_expect_success 'clone with file://user@' '\n> + test_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'\n> +test_expect_success 'clone with file://user:password@' '\n> + test_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'\n> +test_expect_success 'clone with file://:port' '\n> + test_clone_expect_dir file://127.0.0.1:9999/ 127.0.0.1'\n> +test_expect_success 'clone with file://user:password@:port' '\n> + test_clone_expect_dir file://user:password@127.0.0.1:9999/ 127.0.0.1'\n> +\n> +test_expect_success 'cleanup' 'rm -rf /.git /clones'\n> +\n> say \"auto bare gitdir\"\n>\n> # DESTROYYYYY!!!!!\n> --\n> 2.5.0\n"},{"id":"267183","messageId":"xmqqzj2d5ejn.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"CAPig+cQ5wR8xp8erAHBzEMEs62ddULrrML53GNVuuie3kd9EvA@mail.gmail.com","subject":"Re: [PATCH v2 6/6] clone: add tests for cloning with empty path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-31T00:58:04Z","receivedAt":"2015-07-31T00:58:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt wrote:\n>> Test behavior of `git clone` when working with an empty path\n>> component. This may be the case when cloning a file system's root\n>> directory or from a remote server's root.\n>>\n>> Signed-off-by: Patrick Steinhardt \n>> ---\n>> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\n>> index 553a3f6..acfa133 100755\n>> --- a/t/t1509-root-worktree.sh\n>> +++ b/t/t1509-root-worktree.sh\n>> @@ -237,6 +237,45 @@ test_foobar_foobar\n\nAll true, but a more interesting question is why add more to this\ntest, which is known to be skipped by everybody? The issue being\ncorrected is that any \"://@:/\" that says\n\"the whole site serves a single repository\" is problematic.\n\nSurely, file:// and ssh:// may be examples of schemes that require\nthe filesystem root to be usable as the trash directory to test,\nrequiring a dedicated VM (causing most people to skip t1509), but\nwouldn't \"http://@:/\" be easier to arrange to make\nthe whole site serve a single repository?\n"},{"id":"267191","messageId":"20150731084537.GA698@pks-xps","threadId":"39942","inReplyTo":"xmqqzj2d5ejn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 6/6] clone: add tests for cloning with empty path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-07-31T08:45:37Z","receivedAt":"2015-07-31T08:45:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jul 30, 2015 at 05:58:04PM -0700, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n> > On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt wrote:\n> >> Test behavior of `git clone` when working with an empty path\n> >> component. This may be the case when cloning a file system's root\n> >> directory or from a remote server's root.\n> >>\n> >> Signed-off-by: Patrick Steinhardt \n> >> ---\n> >> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\n> >> index 553a3f6..acfa133 100755\n> >> --- a/t/t1509-root-worktree.sh\n> >> +++ b/t/t1509-root-worktree.sh\n> >> @@ -237,6 +237,45 @@ test_foobar_foobar\n> \n> All true, but a more interesting question is why add more to this\n> test, which is known to be skipped by everybody? The issue being\n> corrected is that any \"://@:/\" that says\n> \"the whole site serves a single repository\" is problematic.\n> \n> Surely, file:// and ssh:// may be examples of schemes that require\n> the filesystem root to be usable as the trash directory to test,\n> requiring a dedicated VM (causing most people to skip t1509), but\n> wouldn't \"http://@:/\" be easier to arrange to make\n> the whole site serve a single repository?\n\nSure it would be. But unfortunately I haven't been able to get\nt/lib-httpd working at my end, so that's why I then chose to\nimplement the tests with t1509. I agree though that the other\nsolution would be preferable, but I currently am not able to\nprovide those.\n"},{"id":"267302","messageId":"20150803083414.GA23997@pks-xps.Speedport_W_723V_1_37_000","threadId":"39942","inReplyTo":"xmqqfv457fkc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 5/6] clone: fix hostname parsing when guessing dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-03T08:34:14Z","receivedAt":"2015-08-03T08:34:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jul 30, 2015 at 09:53:07AM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Well, but there is the above \"or we may not\" ;-)\n> >\n> >> But actually you are right, currently I still have the old logic\n> >> in place that splits on colons in the path component. \n> >\n> > Yes. The reason why I suggested the simple route was exactly\n> > because I noticed that you didn't seem to care about the above\n> > \"$site/foo:bar.git/\" => \"$site/foo:bar\" => \"bar\" transform.\n> >\n> > And I think people might depend on that behaviour. \"Fixing\" that\n> > may even be seen as a regression.\n> >\n> > When was the last time you created a foo@bar.git repository?\n> \n> Actually, this was an unrelated question and a wrong one to ask at\n> that.\n> \n> Even though I personally haven't created foo:bar.git repository,\n> because it is no longer 2005, it is highly likely that somewhere\n> there is such a person who depends on the current behaviour of\n> turning that to \"bar\" on the cloned side. Similarly, even if we the\n> people who read the Git mailing list collectively do not know\n> anybody who has foo@bar.git repository, it is highly likely that\n> somewhere there is such a person who depends on the current\n> behaviour of turning that to \"foo@bar\" on the cloned side.\n> \n> So the ideal would be to keep turning $site/foo:bar.git to bar,\n> $site/foo@bar.git to foo@bar, and $scheme//u@p:$host/ to $host.\n> \n> And it would be ideal if we do so without much code churn. \"The\n> whole site is dedicated to host a single repository at the root\" is\n> a highly unlikely set-up and it feels wasteful to spend too much\n> effort on.\n\nOne more question for backwards compatibility remains then.\nCurrently when we clone something like 'http://example.com:2222/'\nwe'd create a git repository '2222' as we'd split on the first\noccurrence of ':'. Should we remain backwards compatible here, as\nwell, or change the behavior to use 'example.com' as repository\nname?\n"},{"id":"267317","messageId":"20150803163727.GA2602@sigill.intra.peff.net","threadId":"39942","inReplyTo":"20150803083414.GA23997@pks-xps.Speedport_W_723V_1_37_000","subject":"Re: [PATCH v2 5/6] clone: fix hostname parsing when guessing dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-03T16:37:28Z","receivedAt":"2015-08-03T16:37:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 03, 2015 at 10:34:14AM +0200, Patrick Steinhardt wrote:\n\n> One more question for backwards compatibility remains then.\n> Currently when we clone something like 'http://example.com:2222/'\n> we'd create a git repository '2222' as we'd split on the first\n> occurrence of ':'. Should we remain backwards compatible here, as\n> well, or change the behavior to use 'example.com' as repository\n> name?\n\nI don't think naming the repo \"2222\" makes much sense; I'd consider it a\nbug. The only sensible names are \"example.com\" or \"example.com:2222\"\n(the latter is more specific if you are going to clone the root off of\nseveral different ports, but that seems rather unlikely; the former is\nprobably what I'd expect).\n\n-Peff\n"},{"id":"267334","messageId":"xmqqsi8088fh.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"20150803163727.GA2602@sigill.intra.peff.net","subject":"Re: [PATCH v2 5/6] clone: fix hostname parsing when guessing dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-03T19:43:14Z","receivedAt":"2015-08-03T19:43:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 03, 2015 at 10:34:14AM +0200, Patrick Steinhardt wrote:\n>\n>> One more question for backwards compatibility remains then.\n>> Currently when we clone something like 'http://example.com:2222/'\n>> we'd create a git repository '2222' as we'd split on the first\n>> occurrence of ':'. Should we remain backwards compatible here, as\n>> well, or change the behavior to use 'example.com' as repository\n>> name?\n>\n> I don't think naming the repo \"2222\" makes much sense; I'd consider it a\n> bug. The only sensible names are \"example.com\" or \"example.com:2222\"\n> (the latter is more specific if you are going to clone the root off of\n> several different ports, but that seems rather unlikely; the former is\n> probably what I'd expect).\n\nYeah, I tend to agree.\n"},{"id":"267387","messageId":"1438687797-14254-1-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1437997708-10732-1-git-send-email-ps@pks.im","subject":"[PATCH v3 0/6] fix repo name when cloning a server's root","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-04T11:29:51Z","receivedAt":"2015-08-04T11:29:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"This is the third version of this patch series. It aims to\nimprove guessing directory names such that we do not include\nauthentication data and port numbers in them.\n\nThis version drops the patches exposing 'parse_connect_url()' and\ninstead does the stripping of authentification data and port\ninside the URI in 'guess_dir_name()'.\n\nActually I'm not that happy with the patch as it currently stands\nas it requires a lot of complexity to correctly strip the URI\nsuch that we do not mishandle several corner cases. At least I\ndidn't find any shorter way of doing what I want without breaking\nbackwards compatibility. I'll try to explain why the more complex\nways of handling the URI are required:\n\n - The naive way of just adding '@' as path separator would break\n cloning repositories like '/foo/bar@baz.git' (which would\n currently become 'bar@baz' but would become 'baz' only).\n\n - Skipping the scheme initially is required because without it we\n wouldn't be able to scan until the next dir separator in the\n host part when stripping authentication information.\n\n - First checking for '/' in the current stripped URI when we\n want to remove the port is required because we do not want to\n strip port numbers when cloning from something like\n '/foo/bar:2222.git' (which would currently become '2222' but\n would then be stripped of the ':2222' part and instead become\n 'bar'). Still, this breaks on cloning a bare repository in the\n current dir (e.g. cloning 'bar:2222.git', which should become\n '2222' because it is not a port number but would become\n 'bar').\n\nAs you can see, there is a lot of complexity in there and I'm not\nconvinced this is better than just exposing\n'parse_connect_url()', which already handles everything for us.\nMaybe I'm just being blind for the obvious solution here, though.\n\nPatrick Steinhardt (6):\n tests: fix broken && chains in t1509-root-worktree\n tests: fix cleanup after tests in t1509-root-worktree\n clone: do not include authentication data in guessed dir\n clone: do not use port number as dir name\n clone: abort if no dir name could be guessed\n clone: add tests for cloning with empty path\n\n builtin/clone.c | 67 ++++++++++++++++++++++++++++++++++++++++--------\n t/t1509-root-worktree.sh | 51 +++++++++++++++++++++++++++++++++---\n 2 files changed, 103 insertions(+), 15 deletions(-)\n\n-- \n2.5.0\n"},{"id":"267391","messageId":"1438687797-14254-2-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438687797-14254-1-git-send-email-ps@pks.im","subject":"[PATCH v3 1/6] tests: fix broken && chains in t1509-root-worktree","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-04T11:29:52Z","receivedAt":"2015-08-04T11:29:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Signed-off-by: Patrick Steinhardt \n---\n t/t1509-root-worktree.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\nindex b6977d4..0c80129 100755\n--- a/t/t1509-root-worktree.sh\n+++ b/t/t1509-root-worktree.sh\n@@ -125,7 +125,7 @@ fi\n ONE_SHA1=d00491fd7e5bb6fa28c517a0bb32b8b506539d4d\n \n test_expect_success 'setup' '\n-\trm -rf /foo\n+\trm -rf /foo &&\n \tmkdir /foo &&\n \tmkdir /foo/bar &&\n \techo 1 > /foo/foome &&\n@@ -218,7 +218,7 @@ unset GIT_WORK_TREE\n \n test_expect_success 'go to /' 'cd /'\n test_expect_success 'setup' '\n-\trm -rf /.git\n+\trm -rf /.git &&\n \techo \"Initialized empty Git repository in /.git/\" > expected &&\n \tgit init > result &&\n \ttest_cmp expected result\n@@ -241,8 +241,8 @@ say \"auto bare gitdir\"\n \n # DESTROYYYYY!!!!!\n test_expect_success 'setup' '\n-\trm -rf /refs /objects /info /hooks\n-\trm /*\n+\trm -rf /refs /objects /info /hooks &&\n+\trm /* &&\n \tcd / &&\n \techo \"Initialized empty Git repository in /\" > expected &&\n \tgit init --bare > result &&\n-- \n2.5.0\n"},{"id":"267389","messageId":"1438687797-14254-3-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438687797-14254-1-git-send-email-ps@pks.im","subject":"[PATCH v3 2/6] tests: fix cleanup after tests in t1509-root-worktree","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-04T11:29:53Z","receivedAt":"2015-08-04T11:29:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"During cleanup we do a simple 'rm /*' to remove leftover files\nfrom previous tests. As 'rm' errors out when there is anything it\ncannot delete and there are directories present at '/' it will\nthrow an error, causing the '&&' chain to fail.\n\nFix this by explicitly removing the files.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1509-root-worktree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\nindex 0c80129..553a3f6 100755\n--- a/t/t1509-root-worktree.sh\n+++ b/t/t1509-root-worktree.sh\n@@ -242,7 +242,7 @@ say \"auto bare gitdir\"\n # DESTROYYYYY!!!!!\n test_expect_success 'setup' '\n \trm -rf /refs /objects /info /hooks &&\n-\trm /* &&\n+\trm -f /expected /ls.expected /me /result &&\n \tcd / &&\n \techo \"Initialized empty Git repository in /\" > expected &&\n \tgit init --bare > result &&\n-- \n2.5.0\n"},{"id":"267388","messageId":"1438687797-14254-4-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438687797-14254-1-git-send-email-ps@pks.im","subject":"[PATCH v3 3/6] clone: do not include authentication data in guessed dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-04T11:29:54Z","receivedAt":"2015-08-04T11:29:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If the URI contains authentication data and the URI's path\ncomponent is empty we fail to guess a sensible directory name.\nE.g. cloning a repository 'ssh://user:password@example.com/' we\nguess a directory name 'password@example.com' where we would want\nthe hostname only, e.g. 'example.com'.\n\nFix this by first skipping authentication data.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 46 +++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 35 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a72ff7e..794a933 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -146,35 +146,59 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n \n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n-\tconst char *end = repo + strlen(repo), *start;\n+\tconst char *end = repo + strlen(repo), *start, *ptr;\n \tsize_t len;\n \tchar *dir;\n \n \t/*\n+\t * Skip scheme.\n+\t */\n+\tstart = strstr(repo, \"://\");\n+\tif (start == NULL)\n+\t\tstart = repo;\n+\telse\n+\t\tstart += 3;\n+\n+\t/*\n+\t * Skip authentication data.\n+\t */\n+\tptr = start;\n+\twhile (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')\n+\t\tptr++;\n+\tif (*ptr == '@')\n+\t\tstart = ptr + 1;\n+\n+\t/*\n \t * Strip trailing spaces, slashes and /.git\n \t */\n-\twhile (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n+\twhile (start < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n \t\tend--;\n-\tif (end - repo > 5 && is_dir_sep(end[-5]) &&\n+\tif (end - start > 5 && is_dir_sep(end[-5]) &&\n \t !strncmp(end - 4, \".git\", 4)) {\n \t\tend -= 5;\n-\t\twhile (repo < end && is_dir_sep(end[-1]))\n+\t\twhile (start < end && is_dir_sep(end[-1]))\n \t\t\tend--;\n \t}\n \n \t/*\n-\t * Find last component, but be prepared that repo could have\n-\t * the form \"remote.example.com:foo.git\", i.e. no slash\n-\t * in the directory part.\n+\t * Find last component. To remain backwards compatible we\n+\t * also regard colons as path separators, such that\n+\t * cloning a repository 'foo:bar.git' would result in a\n+\t * directory 'bar' being guessed. This is skipped if the\n+\t * current path does not contain any path separators.\n \t */\n-\tstart = end;\n-\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n-\t\tstart--;\n+\tif (memchr(start, '/', end - start) != NULL\n+\t || memchr(start, ':', end - start) != NULL) {\n+\t\tstart = end;\n+\t\twhile (!is_dir_sep(start[-1]) && start[-1] != ':')\n+\t\t\tstart--;\n+\t}\n+\tlen = end - start;\n \n \t/*\n \t * Strip .{bundle,git}.\n \t */\n-\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n+\tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n \n \tif (is_bare)\n \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n-- \n2.5.0\n"},{"id":"267393","messageId":"1438687797-14254-5-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438687797-14254-1-git-send-email-ps@pks.im","subject":"[PATCH v3 4/6] clone: do not use port number as dir name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-04T11:29:55Z","receivedAt":"2015-08-04T11:29:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If the URI contains a port number and the URI's path component is\nempty we fail to guess a sensible directory name. E.g. cloning a\nrepository 'ssh://example.com:2222/' we guess a directory name\n'2222' where we would want the hostname only, e.g. 'example.com'.\n\nFix this by stripping trailing port numbers.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 794a933..a163797 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -181,6 +181,23 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t}\n \n \t/*\n+\t * Strip trailing port number if we've got only a\n+\t * hostname (that is, there is no dir separator but a\n+\t * colon). This check is required such that we do not\n+\t * strip URI's like '/foo/bar:2222.git', which should\n+\t * result in a dir '2222' being guessed due to backwards\n+\t * compatibility.\n+\t */\n+\tif (memchr(start, '/', end - start) == NULL\n+\t && memchr(start, ':', end - start) != NULL) {\n+\t\tptr = end;\n+\t\twhile (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')\n+\t\t\tptr--;\n+\t\tif (start < ptr && ptr[-1] == ':')\n+\t\t\tend = ptr - 1;\n+\t}\n+\n+\t/*\n \t * Find last component. To remain backwards compatible we\n \t * also regard colons as path separators, such that\n \t * cloning a repository 'foo:bar.git' would result in a\n-- \n2.5.0\n"},{"id":"267390","messageId":"1438687797-14254-6-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438687797-14254-1-git-send-email-ps@pks.im","subject":"[PATCH v3 5/6] clone: abort if no dir name could be guessed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-04T11:29:56Z","receivedAt":"2015-08-04T11:29:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Due to various components of the URI being stripped off it may\nhappen that we fail to guess a directory name. We currently error\nout with a message that it is impossible to create the working\ntree '' in such cases. Instead, error out early with a sensible\nerror message hinting that a directory name should be specified\nmanually on the command line.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a163797..2adc712 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -217,6 +217,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t */\n \tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n \n+\tif (!len || (len == 1 && *start == '/'))\n+\t die(\"No directory name could be guessed.\\n\"\n+\t\t\"Please specify a directory on the command line\");\n+\n \tif (is_bare)\n \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n \telse\n-- \n2.5.0\n"},{"id":"267392","messageId":"1438687797-14254-7-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438687797-14254-1-git-send-email-ps@pks.im","subject":"[PATCH v3 6/6] clone: add tests for cloning with empty path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-04T11:29:57Z","receivedAt":"2015-08-04T11:29:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Test behavior of `git clone` when working with an empty path\ncomponent. This may be the case when cloning a file system's root\ndirectory or from a remote server's root.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1509-root-worktree.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n\ndiff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\nindex 553a3f6..d521ca3 100755\n--- a/t/t1509-root-worktree.sh\n+++ b/t/t1509-root-worktree.sh\n@@ -237,6 +237,49 @@ test_foobar_foobar\n \n test_expect_success 'cleanup' 'rm -rf /.git'\n \n+say \"clone .git at root without reponame\"\n+\n+test_expect_success 'go to /' 'cd /'\n+test_expect_success 'setup' '\n+\techo \"Initialized empty Git repository in /.git/\" > expected &&\n+\tgit init > result &&\n+\ttest_cmp expected result\n+'\n+\n+test_clone_expect_dir() {\n+\tURL=\"$1\"\n+\tDIR=\"$2\"\n+\tcat <<-EOF >expected &&\n+\t\tCloning into '$DIR'...\n+\t\twarning: You appear to have cloned an empty repository.\n+\t\tEOF\n+\tgit clone \"$URL\" >result 2>&1 &&\n+\trm -rf \"$DIR\" &&\n+\ttest_cmp expected result\n+}\n+\n+test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'\n+test_expect_success 'simple clone of /' '\n+\tcat <<-EOF >expected &&\n+\t\tfatal: No directory name could be guessed.\n+\t\tPlease specify a directory on the command line\n+\t\tEOF\n+\ttest_expect_code 128 git clone / >result 2>&1 &&\n+\ttest_cmp expected result'\n+\n+test_expect_success 'clone with file://host/' '\n+\ttest_clone_expect_dir file://127.0.0.1/ 127.0.0.1'\n+test_expect_success 'clone with file://user@host/' '\n+\ttest_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'\n+test_expect_success 'clone with file://user:password@host/' '\n+\ttest_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'\n+test_expect_success 'clone with file://host:port/' '\n+\ttest_clone_expect_dir file://127.0.0.1:9999/ 127.0.0.1'\n+test_expect_success 'clone with file://user:password@host:port' '\n+\ttest_clone_expect_dir file://user:password@127.0.0.1:9999/ 127.0.0.1'\n+\n+test_expect_success 'cleanup' 'rm -rf /.git /clones'\n+\n say \"auto bare gitdir\"\n \n # DESTROYYYYY!!!!!\n-- \n2.5.0\n"},{"id":"267476","messageId":"CAPig+cT1utggwL_E2+Mmp7LqscywhrWx_Dr0XOSbTJU5oq7NLQ@mail.gmail.com","threadId":"39942","inReplyTo":"1438687797-14254-7-git-send-email-ps@pks.im","subject":"Re: [PATCH v3 6/6] clone: add tests for cloning with empty path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-04T18:37:51Z","receivedAt":"2015-08-04T18:37:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 4, 2015 at 7:29 AM, Patrick Steinhardt wrote:\n> Test behavior of `git clone` when working with an empty path\n> component. This may be the case when cloning a file system's root\n> directory or from a remote server's root.\n\nA few minor, mostly style-related, comments below...\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh\n> index 553a3f6..d521ca3 100755\n> --- a/t/t1509-root-worktree.sh\n> +++ b/t/t1509-root-worktree.sh\n> @@ -237,6 +237,49 @@ test_foobar_foobar\n>\n> test_expect_success 'cleanup' 'rm -rf /.git'\n>\n> +say \"clone .git at root without reponame\"\n> +\n> +test_expect_success 'go to /' 'cd /'\n> +test_expect_success 'setup' '\n> + echo \"Initialized empty Git repository in /.git/\" > expected &&\n> + git init > result &&\n> + test_cmp expected result\n> +'\n> +\n> +test_clone_expect_dir() {\n> + URL=\"$1\"\n> + DIR=\"$2\"\n\nIt would be nice for the &&-chain to be intact for these two lines, as\nwell, since you never know where someone may insert code in the\nfuture. If code is inserted above or between these lines, and the code\nfails, its failure will go unnoticed due to the broken &&-chain.\n\n> + cat <<-EOF >expected &&\n\nIn this codebase, it's customary to say:\n\n cat >expected <<-EOF &&\n\n> + Cloning into '$DIR'...\n> + warning: You appear to have cloned an empty repository.\n> + EOF\n\nIn this codebase, it's customary to place the content and EOF at the\nsame indentation level as the opening 'cat < + git clone \"$URL\" >result 2>&1 &&\n> + rm -rf \"$DIR\" &&\n> + test_cmp expected result\n> +}\n> +\n> +test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'\n> +test_expect_success 'simple clone of /' '\n> + cat <<-EOF >expected &&\n\nSince you don't expect any variable interpolation in this case, you\ncan telegraph that intent more clearly via:\n\n cat >expected <<-\\EOF &&\n\n> + fatal: No directory name could be guessed.\n> + Please specify a directory on the command line\n> + EOF\n> + test_expect_code 128 git clone / >result 2>&1 &&\n> + test_cmp expected result'\n> +\n> +test_expect_success 'clone with file://host/' '\n> + test_clone_expect_dir file://127.0.0.1/ 127.0.0.1'\n> +test_expect_success 'clone with file://user@host/' '\n> + test_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'\n> +test_expect_success 'clone with file://user:password@host/' '\n> + test_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'\n> +test_expect_success 'clone with file://host:port/' '\n> + test_clone_expect_dir file://127.0.0.1:9999/ 127.0.0.1'\n> +test_expect_success 'clone with file://user:password@host:port' '\n> + test_clone_expect_dir file://user:password@127.0.0.1:9999/ 127.0.0.1'\n> +\n> +test_expect_success 'cleanup' 'rm -rf /.git /clones'\n> +\n> say \"auto bare gitdir\"\n>\n> # DESTROYYYYY!!!!!\n> --\n> 2.5.0\n"},{"id":"267507","messageId":"1438769179-5943-1-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1437997708-10732-1-git-send-email-ps@pks.im","subject":"[PATCH v4 0/3] fix repo name when cloning a server's root","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-05T10:06:16Z","receivedAt":"2015-08-05T10:06:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"This is version 4 of my patch series, which aims to improve\nguessed directory names when we clone a server's root, that is we\nhave empty path components.\n\nThis version is still preliminary as it is based upon the patches\nby Peff ([PATCH 0/2] fix clone guess_dir_name regression in\nv2.4.8) which have not been merged yet.\n\nAs his patches include newly added tests that allow for cloning\nfrom a server's root without using chroot, the patches regarding\nt1509 have been dropped. I've posted the fixes to them as a\nseparate patch series instead.\n\nPreviously I've included a guard such that we only try to find\nthe last component if we've got a '/' or ':' in the path. This is\nnot really required, though, as in the case where we ain't got\none of those characters we'll simply skip to the beginning again,\ncausing this to be a no-op, due to the port already being\nstripped. So I've simply dropped the guard to minimize code\nchurn.\n\nPatrick Steinhardt (3):\n clone: do not include authentication data in guessed dir\n clone: do not use port number as dir name\n clone: abort if no dir name could be guessed\n\n builtin/clone.c | 61 ++++++++++++++++++++++++++++++++++++++++--------\n t/t5603-clone-dirname.sh | 10 ++++++--\n 2 files changed, 59 insertions(+), 12 deletions(-)\n\n-- \n2.5.0\n"},{"id":"267509","messageId":"1438769179-5943-2-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438769179-5943-1-git-send-email-ps@pks.im","subject":"[PATCH v4 1/3] clone: do not include authentication data in guessed dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-05T10:06:17Z","receivedAt":"2015-08-05T10:06:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If the URI contains authentication data and the URI's path\ncomponent is empty we fail to guess a sensible directory name.\nE.g. cloning a repository 'ssh://user:password@example.com/' we\nguess a directory name 'password@example.com' where we would want\nthe hostname only, e.g. 'example.com'.\n\nFix this by first skipping authentication data.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 40 ++++++++++++++++++++++++++++++----------\n t/t5603-clone-dirname.sh | 3 ++-\n 2 files changed, 32 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex bf45199..3ddf8b9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -146,30 +146,50 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n \n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n-\tconst char *end = repo + strlen(repo), *start;\n+\tconst char *end = repo + strlen(repo), *start, *ptr;\n \tsize_t len;\n \tchar *dir;\n \n \t/*\n+\t * Skip scheme.\n+\t */\n+\tstart = strstr(repo, \"://\");\n+\tif (start == NULL)\n+\t\tstart = repo;\n+\telse\n+\t\tstart += 3;\n+\n+\t/*\n+\t * Skip authentication data.\n+\t */\n+\tptr = start;\n+\twhile (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')\n+\t\tptr++;\n+\tif (*ptr == '@')\n+\t\tstart = ptr + 1;\n+\n+\t/*\n \t * Strip trailing spaces, slashes and /.git\n \t */\n-\twhile (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n+\twhile (start < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n \t\tend--;\n-\tif (end - repo > 5 && is_dir_sep(end[-5]) &&\n+\tif (end - start > 5 && is_dir_sep(end[-5]) &&\n \t !strncmp(end - 4, \".git\", 4)) {\n \t\tend -= 5;\n-\t\twhile (repo < end && is_dir_sep(end[-1]))\n+\t\twhile (start < end && is_dir_sep(end[-1]))\n \t\t\tend--;\n \t}\n \n \t/*\n-\t * Find last component, but be prepared that repo could have\n-\t * the form \"remote.example.com:foo.git\", i.e. no slash\n-\t * in the directory part.\n+\t * Find last component. To remain backwards compatible we\n+\t * also regard colons as path separators, such that\n+\t * cloning a repository 'foo:bar.git' would result in a\n+\t * directory 'bar' being guessed.\n \t */\n-\tstart = end;\n-\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n-\t\tstart--;\n+\tptr = end;\n+\twhile (start < ptr && !is_dir_sep(ptr[-1]) && ptr[-1] != ':')\n+\t\tptr--;\n+\tstart = ptr;\n \n \t/*\n \t * Strip .{bundle,git}.\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex 46725b9..3a454f9 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -64,6 +64,7 @@ test_clone_dir ssh://host/foo/.git/ foo\n # omitting the path should default to the hostname\n test_clone_dir ssh://host/ host\n test_clone_dir ssh://host:1234/ host fail\n-test_clone_dir ssh://user@host/ host fail\n+test_clone_dir ssh://user@host/ host\n+test_clone_dir ssh://user:password@host/ host\n \n test_done\n-- \n2.5.0\n"},{"id":"267510","messageId":"1438769179-5943-3-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438769179-5943-1-git-send-email-ps@pks.im","subject":"[PATCH v4 2/3] clone: do not use port number as dir name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-05T10:06:18Z","receivedAt":"2015-08-05T10:06:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If the URI contains a port number and the URI's path component is\nempty we fail to guess a sensible directory name. E.g. cloning a\nrepository 'ssh://example.com:2222/' we guess a directory name\n'2222' where we would want the hostname only, e.g. 'example.com'.\n\nFix this by stripping trailing port numbers.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 17 +++++++++++++++++\n t/t5603-clone-dirname.sh | 7 ++++++-\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 3ddf8b9..7d93e13 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -181,6 +181,23 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t}\n \n \t/*\n+\t * Strip trailing port number if we've got only a\n+\t * hostname (that is, there is no dir separator but a\n+\t * colon). This check is required such that we do not\n+\t * strip URI's like '/foo/bar:2222.git', which should\n+\t * result in a dir '2222' being guessed due to backwards\n+\t * compatibility.\n+\t */\n+\tif (memchr(start, '/', end - start) == NULL\n+\t && memchr(start, ':', end - start) != NULL) {\n+\t\tptr = end;\n+\t\twhile (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')\n+\t\t\tptr--;\n+\t\tif (start < ptr && ptr[-1] == ':')\n+\t\t\tend = ptr - 1;\n+\t}\n+\n+\t/*\n \t * Find last component. To remain backwards compatible we\n \t * also regard colons as path separators, such that\n \t * cloning a repository 'foo:bar.git' would result in a\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex 3a454f9..27dbd6c 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -63,8 +63,13 @@ test_clone_dir ssh://host/foo/.git/ foo\n \n # omitting the path should default to the hostname\n test_clone_dir ssh://host/ host\n-test_clone_dir ssh://host:1234/ host fail\n+test_clone_dir ssh://host:1234/ host\n test_clone_dir ssh://user@host/ host\n test_clone_dir ssh://user:password@host/ host\n+test_clone_dir ssh://user:password@host:1234/ host\n+\n+# trailing port-like numbers should not be stripped for paths\n+test_clone_dir ssh://user:password@host/test:1234 1234\n+test_clone_dir ssh://user:password@host/test:1234.git 1234\n \n test_done\n-- \n2.5.0\n"},{"id":"267508","messageId":"1438769179-5943-4-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1438769179-5943-1-git-send-email-ps@pks.im","subject":"[PATCH v4 3/3] clone: abort if no dir name could be guessed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-05T10:06:19Z","receivedAt":"2015-08-05T10:06:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Due to various components of the URI being stripped off it may\nhappen that we fail to guess a directory name. We currently error\nout with a message that it is impossible to create the working\ntree '' in such cases. Instead, error out early with a sensible\nerror message hinting that a directory name should be specified\nmanually on the command line.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 7d93e13..5834978 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -214,6 +214,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \tlen = end - start;\n \tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n \n+\tif (!len || (len == 1 && *start == '/'))\n+\t die(\"No directory name could be guessed.\\n\"\n+\t\t\"Please specify a directory on the command line\");\n+\n \tif (is_bare)\n \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n \telse\n-- \n2.5.0\n"},{"id":"267524","messageId":"xmqq4mkd4p1x.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"1438687797-14254-1-git-send-email-ps@pks.im","subject":"Re: [PATCH v3 0/6] fix repo name when cloning a server's root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-05T17:34:34Z","receivedAt":"2015-08-05T17:34:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> - The naive way of just adding '@' as path separator would break\n> cloning repositories like '/foo/bar@baz.git' (which would\n> currently become 'bar@baz' but would become 'baz' only).\n>\n> - Skipping the scheme initially is required because without it we\n> wouldn't be able to scan until the next dir separator in the\n> host part when stripping authentication information.\n>\n> - First checking for '/' in the current stripped URI when we\n> want to remove the port is required because we do not want to\n> strip port numbers when cloning from something like\n> '/foo/bar:2222.git' (which would currently become '2222' but\n> would then be stripped of the ':2222' part and instead become\n> 'bar'). Still, this breaks on cloning a bare repository in the\n> current dir (e.g. cloning 'bar:2222.git', which should become\n> '2222' because it is not a port number but would become\n> 'bar').\n\nThis is a very good write-up.\n\nPlease make sure all of the above appears in the commit log message\nsomewhere.\n\n> As you can see, there is a lot of complexity in there and I'm not\n> convinced this is better than just exposing\n> 'parse_connect_url()', which already handles everything for us.\n\nIf the function \"handles everything for us\", that's fine, but the\nprimary reason I am hesitant is because parse_connect_url() was\ndesigned specifically not to have to worry about some protocols\n(e.g. I think feeding it a \"http://\" would fail, and more\nimportantly, its current callers want such a call to fail). Also it\nis meant to handle some non-protocols (e.g. scp style host:path that\ndoes not follow ://...).\n\nAlso does it handle the \"2222\" case above? I do not think\nparse_connect_url() even calls get_host_and_port() to be able to\ntell what \"2222\" means in these examples.\n\n> Maybe I'm just being blind for the obvious solution here, though.\n>\n> Patrick Steinhardt (6):\n> tests: fix broken && chains in t1509-root-worktree\n> tests: fix cleanup after tests in t1509-root-worktree\n> clone: do not include authentication data in guessed dir\n> clone: do not use port number as dir name\n> clone: abort if no dir name could be guessed\n> clone: add tests for cloning with empty path\n>\n> builtin/clone.c | 67 ++++++++++++++++++++++++++++++++++++++++--------\n> t/t1509-root-worktree.sh | 51 +++++++++++++++++++++++++++++++++---\n> 2 files changed, 103 insertions(+), 15 deletions(-)\n"},{"id":"267525","messageId":"xmqqzj253a39.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"1438769179-5943-2-git-send-email-ps@pks.im","subject":"Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-05T17:43:06Z","receivedAt":"2015-08-05T17:43:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> If the URI contains authentication data and the URI's path\n> component is empty we fail to guess a sensible directory name.\n> E.g. cloning a repository 'ssh://user:password@example.com/' we\n> guess a directory name 'password@example.com' where we would want\n> the hostname only, e.g. 'example.com'.\n>\n> ...\n> +\tptr = start;\n> +\twhile (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')\n> +\t\tptr++;\n\nHmm....\n\n> +\tif (*ptr == '@')\n> +\t\tstart = ptr + 1;\n> +\n> +\t * Find last component. To remain backwards compatible we\n> +\t * also regard colons as path separators, such that\n> +\t * cloning a repository 'foo:bar.git' would result in a\n> +\t * directory 'bar' being guessed.\n> \t */\n\nI think this is a reasonable thing to do (besides, I think some\npeople cannot have colon in their filenames, so keeping this aspect\nthe same as before would avoid unintended regressions).\n\n> -\tstart = end;\n> -\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n> -\t\tstart--;\n> +\tptr = end;\n> +\twhile (start < ptr && !is_dir_sep(ptr[-1]) && ptr[-1] != ':')\n> +\t\tptr--;\n> +\tstart = ptr;\n> \n> \t/*\n> \t * Strip .{bundle,git}.\n> diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\n> index 46725b9..3a454f9 100755\n> --- a/t/t5603-clone-dirname.sh\n> +++ b/t/t5603-clone-dirname.sh\n> @@ -64,6 +64,7 @@ test_clone_dir ssh://host/foo/.git/ foo\n> # omitting the path should default to the hostname\n> test_clone_dir ssh://host/ host\n> test_clone_dir ssh://host:1234/ host fail\n> -test_clone_dir ssh://user@host/ host fail\n> +test_clone_dir ssh://user@host/ host\n> +test_clone_dir ssh://user:password@host/ host\n\nPerhaps add\n\n\"test_clone_dir ssh://user:passw@rd@host/ host\"\n\nhere? How is this expected to be parsed?\n\n> test_done\n"},{"id":"267526","messageId":"xmqqvbct3a15.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"1438769179-5943-4-git-send-email-ps@pks.im","subject":"Re: [PATCH v4 3/3] clone: abort if no dir name could be guessed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-05T17:44:22Z","receivedAt":"2015-08-05T17:44:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Due to various components of the URI being stripped off it may\n> happen that we fail to guess a directory name. We currently error\n> out with a message that it is impossible to create the working\n> tree '' in such cases. Instead, error out early with a sensible\n> error message hinting that a directory name should be specified\n> manually on the command line.\n\nSounds like a sensible thing to do.\n\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/clone.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 7d93e13..5834978 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -214,6 +214,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> \tlen = end - start;\n> \tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n> \n> +\tif (!len || (len == 1 && *start == '/'))\n> +\t die(\"No directory name could be guessed.\\n\"\n> +\t\t\"Please specify a directory on the command line\");\n> +\n> \tif (is_bare)\n> \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n> \telse\n"},{"id":"267532","messageId":"xmqqr3nh34tv.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"xmqqzj253a39.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-05T19:36:44Z","receivedAt":"2015-08-05T19:36:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps add\n>\n> \"test_clone_dir ssh://user:passw@rd@host/ host\"\n>\n> here? How is this expected to be parsed?\n\nFor completeness, here is what I think the end result (together with\nPeff's series) of the test should look like.\n\nThe first hunk is merely style. We could drop 'in \"$@\"' from there\nand some people may argue that it would be more obvious, but I think\nbeing explict is fine.\n\nAs to the second hunk:\n\n - the first batch is for \"trailing slash removal\" for scp-like\n syntax;\n\n - the second batch is for \"omitting path should default to host\" for\n the same;\n\n - the third batch is for \"omitting authentication material\" for the\n same.\n\nNote that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/\ntests fail for the same reason (finding @ should be greedy, I think).\n\n t/t5603-clone-dirname.sh | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex 27dbd6c..4897ea8 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -22,7 +22,8 @@ test_clone_dir () {\n \texpect=success\n \tbare=non-bare\n \tclone_opts=\n-\tfor i in \"$@\"; do\n+\tfor i in \"$@\"\n+\tdo\n \t\tcase \"$i\" in\n \t\tfail)\n \t\t\texpect=failure\n@@ -61,12 +62,23 @@ test_clone_dir ssh://host/foo/ foo\n test_clone_dir ssh://host/foo.git/ foo\n test_clone_dir ssh://host/foo/.git/ foo\n \n+test_clone_dir host:foo/ foo\n+test_clone_dir host:foo.git/ foo\n+test_clone_dir host:foo/.git/ foo\n+\n # omitting the path should default to the hostname\n test_clone_dir ssh://host/ host\n test_clone_dir ssh://host:1234/ host\n test_clone_dir ssh://user@host/ host\n+test_clone_dir host:/ host\n+\n+# auth materials should be redacted\n test_clone_dir ssh://user:password@host/ host\n test_clone_dir ssh://user:password@host:1234/ host\n+test_clone_dir ssh://user:passw@rd@host:1234/ host\n+test_clone_dir user@host:/ host\n+test_clone_dir user:password@host:/ host\n+test_clone_dir user:passw@rd@host:/ host\n \n # trailing port-like numbers should not be stripped for paths\n test_clone_dir ssh://user:password@host/test:1234 1234\n"},{"id":"267533","messageId":"xmqqmvy534m0.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"xmqqr3nh34tv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-05T19:41:27Z","receivedAt":"2015-08-05T19:41:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> For completeness, here is what I think the end result (together with\n> Peff's series) of the test should look like.\n> ...\n> Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/\n> tests fail for the same reason (finding @ should be greedy, I think).\n\nAnd I think this should make it pass. Just remember the last\noccurrence of '@' by moving the 'start' every time we see an '@'\nsign.\n\n builtin/clone.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex cae288f..5d86439 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t\tstart += 3;\n \n \t/*\n-\t * Skip authentication data.\n+\t * Skip authentication data, if exists.\n \t */\n-\tptr = start;\n-\twhile (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')\n-\t\tptr++;\n-\tif (*ptr == '@')\n-\t\tstart = ptr + 1;\n+\tfor (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {\n+\t\tif (*ptr == '@')\n+\t\t\tstart = ptr + 1;\n+\t}\n \n \t/*\n \t * Strip trailing spaces, slashes and /.git\n"},{"id":"267543","messageId":"20150805211947.GC21134@sigill.intra.peff.net","threadId":"39942","inReplyTo":"xmqq4mkd4p1x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 0/6] fix repo name when cloning a server's root","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-05T21:19:47Z","receivedAt":"2015-08-05T21:19:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 05, 2015 at 10:34:34AM -0700, Junio C Hamano wrote:\n\n> > As you can see, there is a lot of complexity in there and I'm not\n> > convinced this is better than just exposing\n> > 'parse_connect_url()', which already handles everything for us.\n> \n> If the function \"handles everything for us\", that's fine, but the\n> primary reason I am hesitant is because parse_connect_url() was\n> designed specifically not to have to worry about some protocols\n> (e.g. I think feeding it a \"http://\" would fail, and more\n> importantly, its current callers want such a call to fail). Also it\n> is meant to handle some non-protocols (e.g. scp style host:path that\n> does not follow ://...).\n\nTrue, but the transport code _is_ handling that at some point. It makes\nme wonder if it would be possible to push the call to transport_get\nfurther up inside cmd_clone(), and then provide some way to query the\nremote path and hostname from the transport code. Then guess_dir_name\ncould just go away entirely, in favor of something like:\n\n dir_name = transport_get_path(transport);\n if (!*dir_name)\n\tdir_name = transport_get_host(transport);\n\nThat may be overly simplistic or unworkable, though. I haven't dug into\nthe code.\n\n> Also does it handle the \"2222\" case above? I do not think\n> parse_connect_url() even calls get_host_and_port() to be able to\n> tell what \"2222\" means in these examples.\n\nSpeaking of which, has anyone tested whether the old or new code handles\nexternal remote helpers? Certainly:\n\n foo::https://host/repo.git\n\nshould still use repo.git. But technically the string handed to\ngit-remote-foo does not have to look anything like a URL. In those cases\nneither guess_dir_name nor the transport code have any idea what anything\nto the right of the \"::\" means; we probably have to resort to blind\nguessing based on characters like colon and slash.\n\n-Peff\n"},{"id":"267561","messageId":"55C30B2D.5000308@web.de","threadId":"39942","inReplyTo":"20150805211947.GC21134@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/6] fix repo name when cloning a server's root","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-08-06T07:22:21Z","receivedAt":"2015-08-06T07:22:21Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-08-05 23.19, Jeff King wrote:\n> On Wed, Aug 05, 2015 at 10:34:34AM -0700, Junio C Hamano wrote:\n> \n>>> As you can see, there is a lot of complexity in there and I'm not\n>>> convinced this is better than just exposing\n>>> 'parse_connect_url()', which already handles everything for us.\nI try expose and use parse_connect_url():\nIt handles the scp-like syntax \"host:/path,\nliterall IPV6 addresses, port numbers,\n':' without a port number and all other Git specific parsing,\nwhich is inside and outside the RFC 3986.\n(I should know, because I managed to break the parser twice,\nand fix it)\n\nI added a diagnostics to connect.c, and if you run the a simply test,\nwe can see that the colon slash logic is often unsufficient:\n\ntb@mypc:~/projects/git/tb.150731_connect> ./git fetch-pack --diag-url ssh://host/\nDiag: url=ssh://host/\nDiag: protocol=ssh\nDiag: userandhost=host\nDiag: port=NONE\nDiag: path=/\nDiag: guesseddir=host/\ntb@macce:~/projects/git/tb.150731_connect> ./git fetch-pack --diag-url ssh://host:/\nDiag: url=ssh://host:/\nDiag: protocol=ssh\nDiag: userandhost=host\nDiag: port=NONE\nDiag: path=/\nDiag: guesseddir=/\n\n\nOn top of that, you can easily write test cases in t5601, as many as you want.\nThe (minor) drawback is that it doesn't handle http:// or https://,\nbut that is easy to add in the parser, and doesn't break existing code.\n\nThe major which remains is to search for '@' in userandhost,\nand strip that off.\n(Or when there is a '@', search for a ':' before the '@', and strip that off)\nAfter that, all non-printable characters should be %-escaped.\nIf we replace ':' as non-printable as well, we can make Windows users 1% more happy.\n\n\n>>\n>> If the function \"handles everything for us\", that's fine, but the\n>> primary reason I am hesitant is because parse_connect_url() was\n>> designed specifically not to have to worry about some protocols\n>> (e.g. I think feeding it a \"http://\" would fail, and more\n>> importantly, its current callers want such a call to fail). Also it\n>> is meant to handle some non-protocols (e.g. scp style host:path that\n>> does not follow ://...).\n> \n> True, but the transport code _is_ handling that at some point. It makes\n> me wonder if it would be possible to push the call to transport_get\n> further up inside cmd_clone(), and then provide some way to query the\n> remote path and hostname from the transport code. Then guess_dir_name\n> could just go away entirely, in favor of something like:\n> \n> dir_name = transport_get_path(transport);\n> if (!*dir_name)\n> \tdir_name = transport_get_host(transport);\n> \n> That may be overly simplistic or unworkable, though. I haven't dug into\n> the code.\n> \n>> Also does it handle the \"2222\" case above? I do not think\n>> parse_connect_url() even calls get_host_and_port() to be able to\n>> tell what \"2222\" means in these examples.\n> \n> Speaking of which, has anyone tested whether the old or new code handles\n> external remote helpers? Certainly:\n> \n> foo::https://host/repo.git\n> \n> should still use repo.git. But technically the string handed to\n> git-remote-foo does not have to look anything like a URL. In those cases\n> neither guess_dir_name nor the transport code have any idea what anything\n> to the right of the \"::\" means; we probably have to resort to blind\n> guessing based on characters like colon and slash.\n> \nIt is easy to strip the foo:: part of the url, assume that\nthe remote helper uses a RFC 3986 similar url syntax, so that we\ncan feed the reminding https://host/repo.git into the parser (see above).\n\nIf the remote helper doesn't do this, we can't guess anything, can we ?\nSo error out and tell the user seems the right thing to do.\n\nIn the hope that this is useful, pushed my prototype branch to\nhttps://github.com/tboegi/git/tree/150731_connect_diag_guess_name\n"},{"id":"267564","messageId":"xmqqlhdozw1f.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"55C30B2D.5000308@web.de","subject":"Re: [PATCH v3 0/6] fix repo name when cloning a server's root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-06T08:00:12Z","receivedAt":"2015-08-06T08:00:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> It is easy to strip the foo:: part of the url, assume that\n> the remote helper uses a RFC 3986 similar url syntax, so that we\n> can feed the reminding https://host/repo.git into the parser (see above).\n\nThe thing that worries me is that foo:: syntax and external helper\ninterface was invented by Daniel Barkalow primarily because he\nwanted to allow things that do *not* fit in that URL-like scheme,\nfor example see:\n\n http://thread.gmane.org/gmane.comp.version-control.git/125374/focus=125405\n\n> If the remote helper doesn't do this, we can't guess anything, can we ?\n> So error out and tell the user seems the right thing to do.\n\nYes. A blind guess that fails spectacularly is far worse than an\noutright rejection that is cautious.\n"},{"id":"267565","messageId":"20150806094732.GA746@pks-xps","threadId":"39942","inReplyTo":"xmqqmvy534m0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-06T09:47:32Z","receivedAt":"2015-08-06T09:47:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > For completeness, here is what I think the end result (together with\n> > Peff's series) of the test should look like.\n> > ...\n> > Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/\n> > tests fail for the same reason (finding @ should be greedy, I think).\n> \n> And I think this should make it pass. Just remember the last\n> occurrence of '@' by moving the 'start' every time we see an '@'\n> sign.\n> \n> builtin/clone.c | 11 +++++------\n> 1 file changed, 5 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index cae288f..5d86439 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> \t\tstart += 3;\n> \n> \t/*\n> -\t * Skip authentication data.\n> +\t * Skip authentication data, if exists.\n> \t */\n> -\tptr = start;\n> -\twhile (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')\n> -\t\tptr++;\n> -\tif (*ptr == '@')\n> -\t\tstart = ptr + 1;\n> +\tfor (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {\n> +\t\tif (*ptr == '@')\n> +\t\t\tstart = ptr + 1;\n> +\t}\n> \n> \t/*\n> \t * Strip trailing spaces, slashes and /.git\n\nI guess it makes sense to skip over @-signs greedily. Is there\nanything I need to do here or will you squash those changes in?\n"},{"id":"267630","messageId":"xmqqh9oayghp.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"20150806094732.GA746@pks-xps","subject":"Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T20:45:54Z","receivedAt":"2015-08-07T20:45:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> \n>> > For completeness, here is what I think the end result (together with\n>> > Peff's series) of the test should look like.\n>> > ...\n>> > Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/\n>> > tests fail for the same reason (finding @ should be greedy, I think).\n>> \n>> And I think this should make it pass. Just remember the last\n>> occurrence of '@' by moving the 'start' every time we see an '@'\n>> sign.\n>> \n>> builtin/clone.c | 11 +++++------\n>> 1 file changed, 5 insertions(+), 6 deletions(-)\n>> \n>> diff --git a/builtin/clone.c b/builtin/clone.c\n>> index cae288f..5d86439 100644\n>> --- a/builtin/clone.c\n>> +++ b/builtin/clone.c\n>> @@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n>> \t\tstart += 3;\n>> \n>> \t/*\n>> -\t * Skip authentication data.\n>> +\t * Skip authentication data, if exists.\n>> \t */\n>> -\tptr = start;\n>> -\twhile (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')\n>> -\t\tptr++;\n>> -\tif (*ptr == '@')\n>> -\t\tstart = ptr + 1;\n>> +\tfor (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {\n>> +\t\tif (*ptr == '@')\n>> +\t\t\tstart = ptr + 1;\n>> +\t}\n>> \n>> \t/*\n>> \t * Strip trailing spaces, slashes and /.git\n>\n> I guess it makes sense to skip over @-signs greedily. Is there\n> anything I need to do here or will you squash those changes in?\n\nSorry but I won't have patience to split the \"these need squashing\nin\" into multiple pieces and add them to appropriate commits in the\n5 patch series (counting Peff's two fixes on top of which you built\nyour 3 patch series) correctly. You as the original author are much\nbetter equipped to do so than I am, so I'd appreciate if you can\nreroll them as a 5-patch series.\n\nAmong the changes, the fix to builtin/clone.c needs to be squashed\ninto your \"clone: do not include authentication data in guessed\ndir\", and the remainder of the patch to t5603 should go to Peff's\n\"clone: add tests for output directory\", but with most of them\nmarked initially as failing. And then as you fix them in your\npatches, some of the \"fail\" mark should be dropped.\n\nThanks.\n\n builtin/clone.c | 11 +++++------\n t/t5603-clone-dirname.sh | 14 +++++++++++++-\n 2 files changed, 18 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex cae288f..5d86439 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t\tstart += 3;\n \n \t/*\n-\t * Skip authentication data.\n+\t * Skip authentication data, if exists.\n \t */\n-\tptr = start;\n-\twhile (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')\n-\t\tptr++;\n-\tif (*ptr == '@')\n-\t\tstart = ptr + 1;\n+\tfor (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {\n+\t\tif (*ptr == '@')\n+\t\t\tstart = ptr + 1;\n+\t}\n \n \t/*\n \t * Strip trailing spaces, slashes and /.git\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex 27dbd6c..4897ea8 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -22,7 +22,8 @@ test_clone_dir () {\n \texpect=success\n \tbare=non-bare\n \tclone_opts=\n-\tfor i in \"$@\"; do\n+\tfor i in \"$@\"\n+\tdo\n \t\tcase \"$i\" in\n \t\tfail)\n \t\t\texpect=failure\n@@ -61,12 +62,23 @@ test_clone_dir ssh://host/foo/ foo\n test_clone_dir ssh://host/foo.git/ foo\n test_clone_dir ssh://host/foo/.git/ foo\n \n+test_clone_dir host:foo/ foo\n+test_clone_dir host:foo.git/ foo\n+test_clone_dir host:foo/.git/ foo\n+\n # omitting the path should default to the hostname\n test_clone_dir ssh://host/ host\n test_clone_dir ssh://host:1234/ host\n test_clone_dir ssh://user@host/ host\n+test_clone_dir host:/ host\n+\n+# auth materials should be redacted\n test_clone_dir ssh://user:password@host/ host\n test_clone_dir ssh://user:password@host:1234/ host\n+test_clone_dir ssh://user:passw@rd@host:1234/ host\n+test_clone_dir user@host:/ host\n+test_clone_dir user:password@host:/ host\n+test_clone_dir user:passw@rd@host:/ host\n \n # trailing port-like numbers should not be stripped for paths\n test_clone_dir ssh://user:password@host/test:1234 1234\n-- \n2.5.0-423-ga363579\n"},{"id":"267656","messageId":"20150808173737.GA949@pks-pc.localdomain","threadId":"39942","inReplyTo":"xmqqh9oayghp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-08T17:37:37Z","receivedAt":"2015-08-08T17:37:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 07, 2015 at 01:45:54PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:\n> >> Junio C Hamano writes:\n> >> \n> >> > For completeness, here is what I think the end result (together with\n> >> > Peff's series) of the test should look like.\n> >> > ...\n> >> > Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/\n> >> > tests fail for the same reason (finding @ should be greedy, I think).\n> >> \n> >> And I think this should make it pass. Just remember the last\n> >> occurrence of '@' by moving the 'start' every time we see an '@'\n> >> sign.\n> >> \n> >> builtin/clone.c | 11 +++++------\n> >> 1 file changed, 5 insertions(+), 6 deletions(-)\n> >> \n> >> diff --git a/builtin/clone.c b/builtin/clone.c\n> >> index cae288f..5d86439 100644\n> >> --- a/builtin/clone.c\n> >> +++ b/builtin/clone.c\n> >> @@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n> >> \t\tstart += 3;\n> >> \n> >> \t/*\n> >> -\t * Skip authentication data.\n> >> +\t * Skip authentication data, if exists.\n> >> \t */\n> >> -\tptr = start;\n> >> -\twhile (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')\n> >> -\t\tptr++;\n> >> -\tif (*ptr == '@')\n> >> -\t\tstart = ptr + 1;\n> >> +\tfor (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {\n> >> +\t\tif (*ptr == '@')\n> >> +\t\t\tstart = ptr + 1;\n> >> +\t}\n> >> \n> >> \t/*\n> >> \t * Strip trailing spaces, slashes and /.git\n> >\n> > I guess it makes sense to skip over @-signs greedily. Is there\n> > anything I need to do here or will you squash those changes in?\n> \n> Sorry but I won't have patience to split the \"these need squashing\n> in\" into multiple pieces and add them to appropriate commits in the\n> 5 patch series (counting Peff's two fixes on top of which you built\n> your 3 patch series) correctly. You as the original author are much\n> better equipped to do so than I am, so I'd appreciate if you can\n> reroll them as a 5-patch series.\n> \n> Among the changes, the fix to builtin/clone.c needs to be squashed\n> into your \"clone: do not include authentication data in guessed\n> dir\", and the remainder of the patch to t5603 should go to Peff's\n> \"clone: add tests for output directory\", but with most of them\n> marked initially as failing. And then as you fix them in your\n> patches, some of the \"fail\" mark should be dropped.\n> \n> Thanks.\n\nNo problem, just wanted to make sure on how to proceed.\n"},{"id":"267766","messageId":"1439221705-20336-1-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1437997708-10732-1-git-send-email-ps@pks.im","subject":"[PATCH v5 0/5] Improve guessing of repository names","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-10T15:48:20Z","receivedAt":"2015-08-10T15:48:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"This is version 5 of my patch series which aims to improve\nguessed directory names in several cases.\n\nThis version now includes the patches from Jeff which were\npreviously a separate patch series. Besides fixing behavior when\ncloning a repository with trailing '.git' suffix they also add a\nnew test suite that verifies guessed directory names. I've\namended 'clone: add tests for output directory' to add additional\ntests (as proposed by Junio).\n\nChanges to my own patches include improved commit messages\ndetailing why certain changes are done the way they are done and\na change to authentication-data-stripping, such that we now strip\ngreedily until the last '@' sign in the host part (proposed by\nJunio, as well).\n\nJeff King (2):\n clone: add tests for output directory\n clone: use computed length in guess_dir_name\n\nPatrick Steinhardt (3):\n clone: do not include authentication data in guessed dir\n clone: do not use port number as dir name\n clone: abort if no dir name could be guessed\n\n builtin/clone.c | 65 +++++++++++++++++++++++++-----\n t/t5603-clone-dirname.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 157 insertions(+), 11 deletions(-)\n create mode 100755 t/t5603-clone-dirname.sh\n\n-- \n2.5.0\n"},{"id":"267770","messageId":"1439221705-20336-2-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1439221705-20336-1-git-send-email-ps@pks.im","subject":"[PATCH v5 1/5] clone: add tests for output directory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-10T15:48:21Z","receivedAt":"2015-08-10T15:48:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jeff King \n\nWhen we run \"git clone $url\", clone guesses from the $url\nwhat to name the local output directory. We don't have any\ntest coverage of this, so let's add some basic tests.\n\nThis reveals a few problems:\n\n - cloning \"foo.git/\" does not properly remove the \".git\";\n this is a recent regression from 7e837c6 (clone:\n simplify string handling in guess_dir_name(), 2015-07-09)\n\n - likewise, cloning foo/.git does not seem to handle the\n bare case (we should end up in foo.git, but we try to\n use foo/.git on the local end), which also comes from\n 7e837c6.\n\n - cloning the root is not very smart about URL parsing,\n and usernames and port numbers may end up in the\n directory name\n\nAll of these tests are marked as failures.\n\nSigned-off-by: Jeff King \n---\n t/t5603-clone-dirname.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 103 insertions(+)\n create mode 100755 t/t5603-clone-dirname.sh\n\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nnew file mode 100755\nindex 0000000..b009a87\n--- /dev/null\n+++ b/t/t5603-clone-dirname.sh\n@@ -0,0 +1,103 @@\n+#!/bin/sh\n+\n+test_description='check output directory names used by git-clone'\n+. ./test-lib.sh\n+\n+# we use a fake ssh wrapper that ignores the arguments\n+# entirely; we really only care that we get _some_ repo,\n+# as the real test is what clone does on the local side\n+test_expect_success 'setup ssh wrapper' '\n+\twrite_script \"$TRASH_DIRECTORY/ssh-wrapper\" <<-\\EOF &&\n+\tgit upload-pack \"$TRASH_DIRECTORY\"\n+\tEOF\n+\tGIT_SSH=\"$TRASH_DIRECTORY/ssh-wrapper\" &&\n+\texport GIT_SSH &&\n+\texport TRASH_DIRECTORY\n+'\n+\n+# make sure that cloning $1 results in local directory $2\n+test_clone_dir () {\n+\turl=$1; shift\n+\tdir=$1; shift\n+\texpect=success\n+\tbare=non-bare\n+\tclone_opts=\n+\tfor i in \"$@\"; do\n+\t\tcase \"$i\" in\n+\t\tfail)\n+\t\t\texpect=failure\n+\t\t\t;;\n+\t\tbare)\n+\t\t\tbare=bare\n+\t\t\tclone_opts=--bare\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+\ttest_expect_$expect \"clone of $url goes to $dir ($bare)\" \"\n+\t\trm -rf $dir &&\n+\t\tgit clone $clone_opts $url &&\n+\t\ttest_path_is_dir $dir\n+\t\"\n+}\n+\n+# basic syntax with bare and non-bare variants\n+test_clone_dir host:foo foo\n+test_clone_dir host:foo foo.git bare\n+test_clone_dir host:foo.git foo\n+test_clone_dir host:foo.git foo.git bare\n+test_clone_dir host:foo/.git foo\n+test_clone_dir host:foo/.git foo.git bare fail\n+\n+# similar, but using ssh URL rather than host:path syntax\n+test_clone_dir ssh://host/foo foo\n+test_clone_dir ssh://host/foo foo.git bare\n+test_clone_dir ssh://host/foo.git foo\n+test_clone_dir ssh://host/foo.git foo.git bare\n+test_clone_dir ssh://host/foo/.git foo\n+test_clone_dir ssh://host/foo/.git foo.git bare fail\n+\n+# we should remove trailing slashes and .git suffixes\n+test_clone_dir ssh://host/foo/ foo\n+test_clone_dir ssh://host/foo/// foo\n+test_clone_dir ssh://host/foo.git/ foo fail\n+test_clone_dir ssh://host/foo.git/// foo fail\n+test_clone_dir ssh://host/foo///.git/ foo\n+test_clone_dir ssh://host/foo/.git/// foo\n+\n+test_clone_dir host:foo/ foo\n+test_clone_dir host:foo/// foo\n+test_clone_dir host:foo.git/ foo fail\n+test_clone_dir host:foo.git/// foo fail\n+test_clone_dir host:foo///.git/ foo\n+test_clone_dir host:foo/.git/// foo\n+\n+# omitting the path should default to the hostname\n+test_clone_dir ssh://host/ host\n+test_clone_dir ssh://host:1234/ host fail\n+test_clone_dir ssh://user@host/ host fail\n+test_clone_dir host:/ host fail\n+\n+# auth materials should be redacted\n+test_clone_dir ssh://user:password@host/ host fail\n+test_clone_dir ssh://user:password@host:1234/ host fail\n+test_clone_dir ssh://user:passw@rd@host:1234/ host fail\n+test_clone_dir user@host:/ host fail\n+test_clone_dir user:password@host:/ host fail\n+test_clone_dir user:pass@wrd@host:/ host fail\n+\n+# auth-like material should not be dropped\n+test_clone_dir ssh://host/foo@bar foo@bar\n+test_clone_dir ssh://host/foo@bar.git foo@bar\n+test_clone_dir ssh://user:password@host/foo@bar foo@bar\n+test_clone_dir ssh://user:passw@rd@host/foo@bar.git foo@bar\n+\n+test_clone_dir host:/foo@bar foo@bar\n+test_clone_dir host:/foo@bar.git foo@bar\n+test_clone_dir user:password@host:/foo@bar foo@bar\n+test_clone_dir user:passw@rd@host:/foo@bar.git foo@bar\n+\n+# trailing port-like numbers should not be stripped for paths\n+test_clone_dir ssh://user:password@host/test:1234 1234\n+test_clone_dir ssh://user:password@host/test:1234.git 1234\n+\n+test_done\n-- \n2.5.0\n"},{"id":"267767","messageId":"1439221705-20336-3-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1439221705-20336-1-git-send-email-ps@pks.im","subject":"[PATCH v5 2/5] clone: use computed length in guess_dir_name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-10T15:48:22Z","receivedAt":"2015-08-10T15:48:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"From: Jeff King \n\nCommit 7e837c6 (clone: simplify string handling in\nguess_dir_name(), 2015-07-09) changed clone to use\nstrip_suffix instead of hand-rolled pointer manipulation.\nHowever, strip_suffix will strip from the end of a\nNUL-terminated string, and we may have already stripped some\ncharacters (like directory separators, or \"/.git\"). This\nleads to commands like:\n\n git clone host:foo.git/\n\nfailing to strip the \".git\".\n\nWe must instead convert our pointer arithmetic into a\ncomputed length and feed that to strip_suffix_mem, which will\nthen reduce the length further for us.\n\nIt would be nicer if we could drop the pointer manipulation\nentirely, and just continually strip using strip_suffix. But\nthat doesn't quite work for two reasons:\n\n 1. The early suffixes we're stripping are not constant; we\n need to look for is_dir_sep, which could be one of\n several characters.\n\n 2. Mid-way through the stripping we compute the pointer\n \"start\", which shows us the beginning of the pathname.\n Which really give us two lengths to work with: the\n offset from the start of the string, and from the start\n of the path. By using pointers for the early part, we\n can just compute the length from \"start\" when we need\n it.\n\nSigned-off-by: Jeff King \n---\n builtin/clone.c | 3 ++-\n t/t5603-clone-dirname.sh | 12 ++++++------\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 303a3a7..bf45199 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -174,7 +174,8 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t/*\n \t * Strip .{bundle,git}.\n \t */\n-\tstrip_suffix(start, is_bundle ? \".bundle\" : \".git\" , &len);\n+\tlen = end - start;\n+\tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n \n \tif (is_bare)\n \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex b009a87..10f5d09 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -46,7 +46,7 @@ test_clone_dir host:foo foo.git bare\n test_clone_dir host:foo.git foo\n test_clone_dir host:foo.git foo.git bare\n test_clone_dir host:foo/.git foo\n-test_clone_dir host:foo/.git foo.git bare fail\n+test_clone_dir host:foo/.git foo.git bare\n \n # similar, but using ssh URL rather than host:path syntax\n test_clone_dir ssh://host/foo foo\n@@ -54,20 +54,20 @@ test_clone_dir ssh://host/foo foo.git bare\n test_clone_dir ssh://host/foo.git foo\n test_clone_dir ssh://host/foo.git foo.git bare\n test_clone_dir ssh://host/foo/.git foo\n-test_clone_dir ssh://host/foo/.git foo.git bare fail\n+test_clone_dir ssh://host/foo/.git foo.git bare\n \n # we should remove trailing slashes and .git suffixes\n test_clone_dir ssh://host/foo/ foo\n test_clone_dir ssh://host/foo/// foo\n-test_clone_dir ssh://host/foo.git/ foo fail\n-test_clone_dir ssh://host/foo.git/// foo fail\n+test_clone_dir ssh://host/foo.git/ foo\n+test_clone_dir ssh://host/foo.git/// foo\n test_clone_dir ssh://host/foo///.git/ foo\n test_clone_dir ssh://host/foo/.git/// foo\n \n test_clone_dir host:foo/ foo\n test_clone_dir host:foo/// foo\n-test_clone_dir host:foo.git/ foo fail\n-test_clone_dir host:foo.git/// foo fail\n+test_clone_dir host:foo.git/ foo\n+test_clone_dir host:foo.git/// foo\n test_clone_dir host:foo///.git/ foo\n test_clone_dir host:foo/.git/// foo\n \n-- \n2.5.0\n"},{"id":"267769","messageId":"1439221705-20336-4-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1439221705-20336-1-git-send-email-ps@pks.im","subject":"[PATCH v5 3/5] clone: do not include authentication data in guessed dir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-10T15:48:23Z","receivedAt":"2015-08-10T15:48:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If the URI contains authentication data and the URI's path\ncomponent is empty we fail to guess a sensible directory name.\nE.g. cloning a repository 'ssh://user:password@example.com/' we\nguess a directory name 'password@example.com' where we would want\nthe hostname only, e.g. 'example.com'.\n\nThe naive way of just adding '@' as a path separator would break\ncloning repositories like 'foo/bar@baz.git' (which would\ncurrently become 'bar@baz' but would then become 'baz' only).\nInstead fix this by first dropping the scheme and then greedily\nscanning for an '@' sign until we find the first path separator.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 41 +++++++++++++++++++++++++++++++----------\n t/t5603-clone-dirname.sh | 4 ++--\n 2 files changed, 33 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex bf45199..da51792 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -146,30 +146,51 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n \n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n-\tconst char *end = repo + strlen(repo), *start;\n+\tconst char *end = repo + strlen(repo), *start, *ptr;\n \tsize_t len;\n \tchar *dir;\n \n \t/*\n+\t * Skip scheme.\n+\t */\n+\tstart = strstr(repo, \"://\");\n+\tif (start == NULL)\n+\t\tstart = repo;\n+\telse\n+\t\tstart += 3;\n+\n+\t/*\n+\t * Skip authentication data. The stripping does happen\n+\t * greedily, such that we strip up to the last '@' inside\n+\t * the host part.\n+\t */\n+\tfor (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {\n+\t\tif (*ptr == '@')\n+\t\t\tstart = ptr + 1;\n+\t}\n+\n+\t/*\n \t * Strip trailing spaces, slashes and /.git\n \t */\n-\twhile (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n+\twhile (start < end && (is_dir_sep(end[-1]) || isspace(end[-1])))\n \t\tend--;\n-\tif (end - repo > 5 && is_dir_sep(end[-5]) &&\n+\tif (end - start > 5 && is_dir_sep(end[-5]) &&\n \t !strncmp(end - 4, \".git\", 4)) {\n \t\tend -= 5;\n-\t\twhile (repo < end && is_dir_sep(end[-1]))\n+\t\twhile (start < end && is_dir_sep(end[-1]))\n \t\t\tend--;\n \t}\n \n \t/*\n-\t * Find last component, but be prepared that repo could have\n-\t * the form \"remote.example.com:foo.git\", i.e. no slash\n-\t * in the directory part.\n+\t * Find last component. To remain backwards compatible we\n+\t * also regard colons as path separators, such that\n+\t * cloning a repository 'foo:bar.git' would result in a\n+\t * directory 'bar' being guessed.\n \t */\n-\tstart = end;\n-\twhile (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')\n-\t\tstart--;\n+\tptr = end;\n+\twhile (start < ptr && !is_dir_sep(ptr[-1]) && ptr[-1] != ':')\n+\t\tptr--;\n+\tstart = ptr;\n \n \t/*\n \t * Strip .{bundle,git}.\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex 10f5d09..a9aba72 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -74,11 +74,11 @@ test_clone_dir host:foo/.git/// foo\n # omitting the path should default to the hostname\n test_clone_dir ssh://host/ host\n test_clone_dir ssh://host:1234/ host fail\n-test_clone_dir ssh://user@host/ host fail\n+test_clone_dir ssh://user@host/ host\n test_clone_dir host:/ host fail\n \n # auth materials should be redacted\n-test_clone_dir ssh://user:password@host/ host fail\n+test_clone_dir ssh://user:password@host/ host\n test_clone_dir ssh://user:password@host:1234/ host fail\n test_clone_dir ssh://user:passw@rd@host:1234/ host fail\n test_clone_dir user@host:/ host fail\n-- \n2.5.0\n"},{"id":"267771","messageId":"1439221705-20336-5-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1439221705-20336-1-git-send-email-ps@pks.im","subject":"[PATCH v5 4/5] clone: do not use port number as dir name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-10T15:48:24Z","receivedAt":"2015-08-10T15:48:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If the URI contains a port number and the URI's path component is\nempty we fail to guess a sensible directory name. E.g. cloning a\nrepository 'ssh://example.com:2222/' we guess a directory name\n'2222' where we would want the hostname only, e.g. 'example.com'.\n\nWe need to take care to not drop trailing port-like numbers in\ncertain cases. E.g. when cloning a repository 'foo/bar:2222.git'\nwe want to guess the directory name '2222' instead of 'bar'.\nThus, we have to first check the stripped URI for path separators\nand only strip port numbers if there are path separators present.\nThis heuristic breaks when cloning a repository 'bar:2222.git',\nthough.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 17 +++++++++++++++++\n t/t5603-clone-dirname.sh | 14 +++++++-------\n 2 files changed, 24 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex da51792..e7f16ff 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -182,6 +182,23 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t}\n \n \t/*\n+\t * Strip trailing port number if we've got only a\n+\t * hostname (that is, there is no dir separator but a\n+\t * colon). This check is required such that we do not\n+\t * strip URI's like '/foo/bar:2222.git', which should\n+\t * result in a dir '2222' being guessed due to backwards\n+\t * compatibility.\n+\t */\n+\tif (memchr(start, '/', end - start) == NULL\n+\t && memchr(start, ':', end - start) != NULL) {\n+\t\tptr = end;\n+\t\twhile (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')\n+\t\t\tptr--;\n+\t\tif (start < ptr && ptr[-1] == ':')\n+\t\t\tend = ptr - 1;\n+\t}\n+\n+\t/*\n \t * Find last component. To remain backwards compatible we\n \t * also regard colons as path separators, such that\n \t * cloning a repository 'foo:bar.git' would result in a\ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex a9aba72..664d0ab 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -73,17 +73,17 @@ test_clone_dir host:foo/.git/// foo\n \n # omitting the path should default to the hostname\n test_clone_dir ssh://host/ host\n-test_clone_dir ssh://host:1234/ host fail\n+test_clone_dir ssh://host:1234/ host\n test_clone_dir ssh://user@host/ host\n-test_clone_dir host:/ host fail\n+test_clone_dir host:/ host\n \n # auth materials should be redacted\n test_clone_dir ssh://user:password@host/ host\n-test_clone_dir ssh://user:password@host:1234/ host fail\n-test_clone_dir ssh://user:passw@rd@host:1234/ host fail\n-test_clone_dir user@host:/ host fail\n-test_clone_dir user:password@host:/ host fail\n-test_clone_dir user:pass@wrd@host:/ host fail\n+test_clone_dir ssh://user:password@host:1234/ host\n+test_clone_dir ssh://user:passw@rd@host:1234/ host\n+test_clone_dir user@host:/ host\n+test_clone_dir user:password@host:/ host\n+test_clone_dir user:pass@wrd@host:/ host\n \n # auth-like material should not be dropped\n test_clone_dir ssh://host/foo@bar foo@bar\n-- \n2.5.0\n"},{"id":"267768","messageId":"1439221705-20336-6-git-send-email-ps@pks.im","threadId":"39942","inReplyTo":"1439221705-20336-1-git-send-email-ps@pks.im","subject":"[PATCH v5 5/5] clone: abort if no dir name could be guessed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-08-10T15:48:25Z","receivedAt":"2015-08-10T15:48:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Due to various components of the URI being stripped off it may\nhappen that we fail to guess a directory name. We currently error\nout with a message that it is impossible to create the working\ntree '' in such cases. Instead, error out early with a sensible\nerror message hinting that a directory name should be specified\nmanually on the command line.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex e7f16ff..5169746 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -215,6 +215,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \tlen = end - start;\n \tstrip_suffix_mem(start, &len, is_bundle ? \".bundle\" : \".git\");\n \n+\tif (!len || (len == 1 && *start == '/'))\n+\t die(\"No directory name could be guessed.\\n\"\n+\t\t\"Please specify a directory on the command line\");\n+\n \tif (is_bare)\n \t\tdir = xstrfmt(\"%.*s.git\", (int)len, start);\n \telse\n-- \n2.5.0\n"},{"id":"267785","messageId":"xmqqio8nuiej.fsf@gitster.dls.corp.google.com","threadId":"39942","inReplyTo":"1439221705-20336-1-git-send-email-ps@pks.im","subject":"Re: [PATCH v5 0/5] Improve guessing of repository names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-10T18:07:16Z","receivedAt":"2015-08-10T18:07:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> This is version 5 of my patch series which aims to improve\n> guessed directory names in several cases.\n>\n> This version now includes the patches from Jeff which were\n> previously a separate patch series. Besides fixing behavior when\n> cloning a repository with trailing '.git' suffix they also add a\n> new test suite that verifies guessed directory names. I've\n> amended 'clone: add tests for output directory' to add additional\n> tests (as proposed by Junio).\n>\n> Changes to my own patches include improved commit messages\n> detailing why certain changes are done the way they are done and\n> a change to authentication-data-stripping, such that we now strip\n> greedily until the last '@' sign in the host part (proposed by\n> Junio, as well).\n>\n> Jeff King (2):\n> clone: add tests for output directory\n> clone: use computed length in guess_dir_name\n>\n> Patrick Steinhardt (3):\n> clone: do not include authentication data in guessed dir\n> clone: do not use port number as dir name\n> clone: abort if no dir name could be guessed\n>\n> builtin/clone.c | 65 +++++++++++++++++++++++++-----\n> t/t5603-clone-dirname.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 157 insertions(+), 11 deletions(-)\n> create mode 100755 t/t5603-clone-dirname.sh\n\nLooks good.\n\nI'll forge your sign-off for two patches from Peff and queue the\nwhole thing.\n\nThanks.\n"}]}