{"thread":{"id":"15329","subject":"[PATCH] ignore trailing slash when creating leading directories","startedAt":"2008-09-02T08:19:09Z","lastAt":"2008-09-03T19:24:31Z","messageCount":9,"participants":["Clemens Buchacher","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"89497","messageId":"20080902081909.GA2059@localhost","threadId":"15329","inReplyTo":null,"subject":"[PATCH] ignore trailing slash when creating leading directories","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-09-02T08:19:09Z","receivedAt":"2008-09-02T08:19:09Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"'git clone <repo> path/' (note the trailing slash) fails, because the\nentire path is interpreted as leading directories. So when mkdir tries to\ncreate the actual path, it already exists.\n\nThis makes sure a trailing slash is ignored.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n builtin-clone.c |    7 ++++---\n sha1_file.c     |    2 +-\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex c0e3086..4f945ad 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -422,10 +422,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (!option_bare) {\n \t\tjunk_work_tree = work_tree;\n \t\tif (safe_create_leading_directories_const(work_tree) < 0)\n-\t\t\tdie(\"could not create leading directories of '%s'\",\n-\t\t\t\t\twork_tree);\n+\t\t\tdie(\"could not create leading directories of '%s': %s\",\n+\t\t\t\t\twork_tree, strerror(errno));\n \t\tif (mkdir(work_tree, 0755))\n-\t\t\tdie(\"could not create work tree dir '%s'.\", work_tree);\n+\t\t\tdie(\"could not create work tree dir '%s': %s.\",\n+\t\t\t\t\twork_tree, strerror(errno));\n \t\tset_git_work_tree(work_tree);\n \t}\n \tjunk_git_dir = git_dir;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9ee1ed1..3cb9414 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -97,7 +97,7 @@ int safe_create_leading_directories(char *path)\n \n \twhile (pos) {\n \t\tpos = strchr(pos, '/');\n-\t\tif (!pos)\n+\t\tif (!pos || !*(pos + 1))\n \t\t\tbreak;\n \t\t*pos = 0;\n \t\tif (!stat(path, &st)) {\n-- \n1.6.0\n"},{"id":"89522","messageId":"7vk5dul7f5.fsf@gitster.siamese.dyndns.org","threadId":"15329","inReplyTo":"20080902081909.GA2059@localhost","subject":"Re: [PATCH] ignore trailing slash when creating leading directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T18:38:38Z","receivedAt":"2008-09-02T18:38:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> 'git clone <repo> path/' (note the trailing slash) fails, because the\n> entire path is interpreted as leading directories. So when mkdir tries to\n> create the actual path, it already exists.\n>\n> This makes sure a trailing slash is ignored.\n>\n> Signed-off-by: Clemens Buchacher <drizzd@aon.at>\n\nThanks.\n\nI have a few comments.\n\n (1) Addition of strerror(errno) is a good thing, but it is a separate\n     topic;\n\n (2) I always thought that it was a clever feature to allow callers that\n     would want to prepare a directory in advance to ask for \"xyzzy/\" and\n     cause the whole path created.  You are breaking it, which may or may\n     not be a bad thing per-se, because I do not think any existing caller\n     depends on this behaviour;\n\n (3) If you *are* to break that feature, then I think you should also\n     handle a user input that is broken in the same fashion as your clone\n     example, namely, \"git clone <repo> path//\".  It does not make much\n     senseto say \"path/\" as the last parameter to clone is not a user\n     error but \"path//\" is.\n\nIf a change in behaviour to strip trailing slashes inside safe_c_l_d() is\nagreed to be a good thing (I do not mind that myself, but there could be\nsome private patches people are using in their trees that depend on the\ncurrent behaviour --- we never know), I think it should go through the\nusual next-master cycle as a feature enhancement / clean-up patch, so that\nwe have better chance to catch breakages this might cause to other people.\n\nAs a \"bugfix\" patch meant to apply to 'maint', I'd prefer a fix to the\ncaller (builtin-clone.c that calls the function), which should be of much\nless impact.  It is fine to include the change to add strerror(errno) in\nthat patch, whose title would be \"clone: fix creation of explicitly named\ntarget directory\".\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 9ee1ed1..3cb9414 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -97,7 +97,7 @@ int safe_create_leading_directories(char *path)\n>  \n>  \twhile (pos) {\n>  \t\tpos = strchr(pos, '/');\n> -\t\tif (!pos)\n> +\t\tif (!pos || !*(pos + 1))\n\n(minor nit) I think\n\n\t\tif (!pos || !pos[1])\n\nis shorter and easier on the eye.\n\n>  \t\t\tbreak;\n>  \t\t*pos = 0;\n>  \t\tif (!stat(path, &st)) {\n> -- \n> 1.6.0\n"},{"id":"89525","messageId":"20080902191322.GA11172@localhost","threadId":"15329","inReplyTo":"7vk5dul7f5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] ignore trailing slash when creating leading directories","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-09-02T19:13:22Z","receivedAt":"2008-09-02T19:13:22Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Tue, Sep 02, 2008 at 11:38:38AM -0700, Junio C Hamano wrote:\n>  (1) Addition of strerror(errno) is a good thing, but it is a separate\n>      topic;\n\nYes indeed. There are many more places that lack proper error reporting. I\nwonder if we should introduce some wrapper functions like mkdir_or_die, so\nthe caller doesn't have check for errors. Such a function could\n(optionally?) create leading directories as well.\n\n>  (2) I always thought that it was a clever feature to allow callers that\n>      would want to prepare a directory in advance to ask for \"xyzzy/\" and\n>      cause the whole path created.  You are breaking it, which may or may\n>      not be a bad thing per-se, because I do not think any existing caller\n>      depends on this behaviour;\n\nYes, I was afraid of that. So I checked all calls to c_l_d and it's not used\nthat way anywhere.\n\n>  (3) If you *are* to break that feature, then I think you should also\n>      handle a user input that is broken in the same fashion as your clone\n>      example, namely, \"git clone <repo> path//\".  It does not make much\n>      senseto say \"path/\" as the last parameter to clone is not a user\n>      error but \"path//\" is.\n\nTrue enough.\n\n> As a \"bugfix\" patch meant to apply to 'maint', I'd prefer a fix to the\n> caller (builtin-clone.c that calls the function), which should be of much\n> less impact.  It is fine to include the change to add strerror(errno) in\n> that patch, whose title would be \"clone: fix creation of explicitly named\n> target directory\".\n\nUnfortunately, if we simply add strerror to the error message, in place of\n\nfatal: could not create work tree dir 'path/'.\n\nthe new version would print\n\nfatal: could not create work tree dir 'path/': File exists.\n\nwhich makes things worse IMO. We could of course strip trailing slashes in\nbuiltin-clone.c for now and revert that as soon as the cleanup patch is in,\nbut I think it's not worth the trouble. I suggest we live with the \"bug\" for\nnow. The error reporting cleanups should be done at a greater scope anyways.\n\n> > -\t\tif (!pos)\n> > +\t\tif (!pos || !*(pos + 1))\n> \n> (minor nit) I think\n> \n> \t\tif (!pos || !pos[1])\n> \n> is shorter and easier on the eye.\n\nWill be fixed in the patch to follow.\n\nClemens\n"},{"id":"89532","messageId":"20080902200759.GA13451@localhost","threadId":"15329","inReplyTo":"20080902191322.GA11172@localhost","subject":"[PATCH v2] ignore trailing slashes when creating leading directories","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-09-02T20:07:59Z","receivedAt":"2008-09-02T20:07:59Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Currently, create_leading_directories() will interpret all parts of\npaths like 'a/b/c/' as \"leading directories\". A subsequent call to mkdir\nfor the tail of the path will fail, because the \"File already exists.\"\n\nThis makes sure trailing slashes are ignored.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nApplies to next. Passes regression tests.\n\n sha1_file.c |    7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9ee1ed1..dcb3d22 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -93,12 +93,19 @@ static inline int offset_1st_component(const char *path)\n int safe_create_leading_directories(char *path)\n {\n \tchar *pos = path + offset_1st_component(path);\n+\tchar *next;\n \tstruct stat st;\n \n \twhile (pos) {\n \t\tpos = strchr(pos, '/');\n \t\tif (!pos)\n \t\t\tbreak;\n+\t\tnext = pos + 1;\n+\t\twhile (*next == '/')\n+\t\t\tnext++;\n+\t\tif (!*next)\n+\t\t\tbreak;\n+\n \t\t*pos = 0;\n \t\tif (!stat(path, &st)) {\n \t\t\t/* path exists */\n-- \n1.6.0.1.275.gc88b6\n"},{"id":"89540","messageId":"7vod36jne1.fsf@gitster.siamese.dyndns.org","threadId":"15329","inReplyTo":"20080902191322.GA11172@localhost","subject":"Re: [PATCH] ignore trailing slash when creating leading directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T20:36:38Z","receivedAt":"2008-09-02T20:36:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> Unfortunately, if we simply add strerror to the error message, in place of\n>\n> fatal: could not create work tree dir 'path/'.\n>\n> the new version would print\n>\n> fatal: could not create work tree dir 'path/': File exists.\n>\n> which makes things worse IMO. We could of course strip trailing slashes in\n> builtin-clone.c for now and revert that as soon as the cleanup patch is in,\n\nI think the above statement is a wrong way to think about it.\n\nThis particular caller (and nobody else), even when it does not want the\n\"create all directories\" behaviour, is giving such an instruction to the\nexisting function, without validating the user input.  That is a bug that\nneeds to be fixed, regardless of what the final semantics of c-l-d should\nbe, isn't it?\n\n> but I think it's not worth the trouble.\n\nWhat I think is not worth the trouble is, after fixing this caller, to\nrevert that fix, when/if we decide that c-l-d should strip trailing\nslashes for all callers.\n\nI am not saying the current semantics of c-l-d is optimal.  I am just\nworried about breaking people's private patches that may depend on the\ncurrent behaviour.  It would be nice if we can clean it up without\nbreaking people, and after doing so, if somebody really want to have\n\"create all directories, the last one is also a directory\", they can\ninvent a cleaner function that takes that insn as a separate paremeter,\nnot as an obscure \"trailing slash means create all\", which may be cute but\nnot clean nor clear at all, which is what c-l-d does.\n"},{"id":"89547","messageId":"7v1w02jlu0.fsf@gitster.siamese.dyndns.org","threadId":"15329","inReplyTo":"7vod36jne1.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] ignore trailing slash when creating leading directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T21:10:15Z","receivedAt":"2008-09-02T21:10:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I am not saying the current semantics of c-l-d is optimal.  I am just\n> worried about breaking people's private patches that may depend on the\n> current behaviour.  It would be nice if we can clean it up without\n> breaking people, and after doing so, if somebody really want to have\n> \"create all directories, the last one is also a directory\", they can\n> invent a cleaner function that takes that insn as a separate paremeter,\n> not as an obscure \"trailing slash means create all\", which may be cute but\n> not clean nor clear at all, which is what c-l-d does.\n\nAnd just so that you do not misunderstand that I am opposed to the change\nto c-l-d, here is a replacement to your original patch.  I didn't fix the\ncaller, as I didn't want to do an overlapping fix with you.\n\n-- >8 --\nsafe_create_leadign_directories(): make it about \"leading\" directories\n\nWe used to allow callers to pass \"foo/bar/\" to make sure both \"foo\" and\n\"foo/bar\" exist and have good permissions, but this interface is too error\nprone.  If a caller mistakenly passes a path with trailing slashes\n(because it forgets to verify the user input, perhaps) even when it wants\nto later mkdir \"bar\" itself, it will find that it cannot mkdir \"bar\".  If\nsuch a caller does not bother to check the error for EEXIST, it may even\nerrorneously die().\n\nBecause we have no existing callers to use that obscure feature, this\npatch removes it to avoid confusion.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sha1_file.c |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git i/sha1_file.c w/sha1_file.c\nindex 9ee1ed1..bc6176e 100644\n--- i/sha1_file.c\n+++ w/sha1_file.c\n@@ -99,7 +99,11 @@ int safe_create_leading_directories(char *path)\n \t\tpos = strchr(pos, '/');\n \t\tif (!pos)\n \t\t\tbreak;\n-\t\t*pos = 0;\n+\t\twhile (*++pos == '/')\n+\t\t\t;\n+\t\tif (!*pos)\n+\t\t\tbreak;\n+\t\t*--pos = '\\0';\n \t\tif (!stat(path, &st)) {\n \t\t\t/* path exists */\n \t\t\tif (!S_ISDIR(st.st_mode)) {\n"},{"id":"89668","messageId":"20080903185554.GA6738@localhost","threadId":"15329","inReplyTo":"7vk5dul7f5.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] clone: fix creation of explicitly named target directory","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-09-03T18:55:55Z","receivedAt":"2008-09-03T18:55:55Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"'git clone <repo> path/' (note the trailing slash) fails, because the\nentire path is interpreted as leading directories. So when mkdir tries to\ncreate the actual path, it already exists.\n\nThis makes sure trailing slashes are removed.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nOn Tue, Sep 02, 2008 at 11:38:38AM -0700, Junio C Hamano wrote:\n> As a \"bugfix\" patch meant to apply to 'maint', I'd prefer a fix to the\n> caller (builtin-clone.c that calls the function), which should be of much\n> less impact.  It is fine to include the change to add strerror(errno) in\n> that patch, whose title would be \"clone: fix creation of explicitly named\n> target directory\".\n\n builtin-clone.c |   20 ++++++++++++++++----\n 1 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex f44ecea..bbfa7d1 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -147,6 +147,17 @@ static int is_directory(const char *path)\n \treturn !stat(path, &buf) && S_ISDIR(buf.st_mode);\n }\n \n+static char *strip_dir_sep(char *dir)\n+{\n+\tchar *end = dir + strlen(dir);\n+\n+\twhile (dir < end && is_dir_sep(end[-1]))\n+\t\tend--;\n+\t*end = '\\0';\n+\n+\treturn dir;\n+}\n+\n static void setup_reference(const char *repo)\n {\n \tconst char *ref_git;\n@@ -394,7 +405,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\trepo = repo_name;\n \n \tif (argc == 2)\n-\t\tdir = xstrdup(argv[1]);\n+\t\tdir = strip_dir_sep(xstrdup(argv[1]));\n \telse\n \t\tdir = guess_dir_name(repo_name, is_bundle, option_bare);\n \n@@ -422,10 +433,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (!option_bare) {\n \t\tjunk_work_tree = work_tree;\n \t\tif (safe_create_leading_directories_const(work_tree) < 0)\n-\t\t\tdie(\"could not create leading directories of '%s'\",\n-\t\t\t\t\twork_tree);\n+\t\t\tdie(\"could not create leading directories of '%s': %s\",\n+\t\t\t\t\twork_tree, strerror(errno));\n \t\tif (mkdir(work_tree, 0755))\n-\t\t\tdie(\"could not create work tree dir '%s'.\", work_tree);\n+\t\t\tdie(\"could not create work tree dir '%s': %s.\",\n+\t\t\t\t\twork_tree, strerror(errno));\n \t\tset_git_work_tree(work_tree);\n \t}\n \tjunk_git_dir = git_dir;\n-- \n1.6.0\n"},{"id":"89669","messageId":"20080903190230.GA12990@localhost","threadId":"15329","inReplyTo":"7v1w02jlu0.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] ignore trailing slash when creating leading directories","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-09-03T19:02:31Z","receivedAt":"2008-09-03T19:02:31Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Tue, Sep 02, 2008 at 02:10:15PM -0700, Junio C Hamano wrote:\n> And just so that you do not misunderstand that I am opposed to the change\n> to c-l-d, here is a replacement to your original patch.  I didn't fix the\n> caller, as I didn't want to do an overlapping fix with you.\n\nYou beat me to it - again. :-) But I like yours much better.\n\nClemens\n"},{"id":"89674","messageId":"7vprnlav80.fsf@gitster.siamese.dyndns.org","threadId":"15329","inReplyTo":"20080903185554.GA6738@localhost","subject":"Re: [PATCH] clone: fix creation of explicitly named target directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-03T19:24:31Z","receivedAt":"2008-09-03T19:24:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> 'git clone <repo> path/' (note the trailing slash) fails, because the\n> entire path is interpreted as leading directories. So when mkdir tries to\n> create the actual path, it already exists.\n>\n> This makes sure trailing slashes are removed.\n\nThanks.\n\n> +static char *strip_dir_sep(char *dir)\n> +{\n> +\tchar *end = dir + strlen(dir);\n> +\n> +\twhile (dir < end && is_dir_sep(end[-1]))\n> +\t\tend--;\n> +\t*end = '\\0';\n\nIt does not matter in this particular context, but I'd do \"dir < end - 1\"\nto avoid returning the root directory as an empty string, just as a\ndisciplined style; also I'd rename this to strip_trailing_slashes(), make\nit of type void to make it more clear that it munges the string that is\ngiven as the input parameter.\n\n> @@ -394,7 +405,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\trepo = repo_name;\n>  \n>  \tif (argc == 2)\n> -\t\tdir = xstrdup(argv[1]);\n> +\t\tdir = strip_dir_sep(xstrdup(argv[1]));\n>  \telse\n>  \t\tdir = guess_dir_name(repo_name, is_bundle, option_bare);\n\nMade me wonder if guess_dir_name() can return something with trailing\nslashes; it turns out that it doesn't, but not very nice.  As people's\nbraincycle is more precious, I'd rather say:\n\n\tif (argc == 2)\n        \tdir = xstrdup(argv[1]);\n\telse\n        \tdir = guess_dir_name(repo_name, is_bundle, option_bare);\n\tstrip_trailing_slashes(dir);\n\nI'll queue with the above changes to reduce one round of back-and-forth,\nbut if you see any flaws in my above reasoning, please say so.\n"}]}