ignore trailing slash when creating leading directories
PATCH 1/1 ignore trailing slash when creating leading directories
'git clone <repo> path/' (note the trailing slash) fails, because the entire path is interpreted as leading directories. So when mkdir tries to create the actual path, it already exists.
This makes sure a trailing slash is ignored.
--- builtin-clone.c | 7 ++++--- sha1_file.c | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)
builtin-clone.c +4 -3
@@ -422,10 +422,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)if (!option_bare) { junk_work_tree = work_tree;if (safe_create_leading_directories_const (work_tree) <0 ) -die ("could not create leading directories of '%s'" , - work_tree); +die ("could not create leading directories of '%s': %s" , + work_tree,strerror (errno));if (mkdir (work_tree,0755 )) -die ("could not create work tree dir '%s'." , work_tree); +die ("could not create work tree dir '%s': %s." , + work_tree,strerror (errno));set_git_work_tree (work_tree); } junk_git_dir = git_dir;
sha1_file.c +1 -1
@@ -97,7 +97,7 @@ int safe_create_leading_directories(char *path)while (pos) { pos =strchr (pos,'/' ); -if (!pos) +if (!pos || !*(pos +1 ))break ; *pos =0 ;if (!stat (path, &st)) {
Signature · 1 line
1.6.0
Quoted 7 lines · Clemens Buchacher
'git clone <repo> path/' (note the trailing slash) fails, because the entire path is interpreted as leading directories. So when mkdir tries to create the actual path, it already exists.
This makes sure a trailing slash is ignored.
Signed-off-by Clemens Buchacher <[email protected]>
Thanks.
I have a few comments.
(1) Addition of strerror(errno) is a good thing, but it is a separate
topic;
(2) I always thought that it was a clever feature to allow callers that would want to prepare a directory in advance to ask for "xyzzy/" and cause the whole path created. You are breaking it, which may or may not be a bad thing per-se, because I do not think any existing caller depends on this behaviour;
(3) If you *are* to break that feature, then I think you should also handle a user input that is broken in the same fashion as your clone example, namely, "git clone <repo> path//". It does not make much senseto say "path/" as the last parameter to clone is not a user error but "path//" is.
If a change in behaviour to strip trailing slashes inside safe_c_l_d() is agreed to be a good thing (I do not mind that myself, but there could be some private patches people are using in their trees that depend on the current behaviour --- we never know), I think it should go through the usual next-master cycle as a feature enhancement / clean-up patch, so that we have better chance to catch breakages this might cause to other people.
As a "bugfix" patch meant to apply to 'maint', I'd prefer a fix to the caller (builtin-clone.c that calls the function), which should be of much less impact. It is fine to include the change to add strerror(errno) in that patch, whose title would be "clone: fix creation of explicitly named target directory".
sha1_file.c int safe_create_leading_directories(char *path) · line 100
Quoted 10 lines
sha1_file.c +1
-1@@ -97,7 +97,7 @@ int safe_create_leading_directories(char *path)while (pos) { pos =strchr (pos,'/' ); -if (!pos) +if (!pos || !*(pos +1 ))
(minor nit) I think
if (!pos || !pos[1])
is shorter and easier on the eye.
Quoted 5 lines
sha1_file.c +0
-0break ; *pos =0 ;if (!stat (path, &st)) {Signature · 1 line
1.6.0
Quoted 2 lines · Junio C Hamano
(1) Addition of strerror(errno) is a good thing, but it is a separate
topic;
Yes indeed. There are many more places that lack proper error reporting. I wonder if we should introduce some wrapper functions like mkdir_or_die, so the caller doesn't have check for errors. Such a function could (optionally?) create leading directories as well.
Quoted 5 lines
(2) I always thought that it was a clever feature to allow callers that would want to prepare a directory in advance to ask for "xyzzy/" and cause the whole path created. You are breaking it, which may or may not be a bad thing per-se, because I do not think any existing caller depends on this behaviour;
Yes, I was afraid of that. So I checked all calls to c_l_d and it's not used that way anywhere.
Quoted 5 lines
(3) If you *are* to break that feature, then I think you should also handle a user input that is broken in the same fashion as your clone example, namely, "git clone <repo> path//". It does not make much senseto say "path/" as the last parameter to clone is not a user error but "path//" is.
True enough.
Quoted 5 lines
As a "bugfix" patch meant to apply to 'maint', I'd prefer a fix to the caller (
builtin-clone.cthat calls the function), which should be of much less impact. It is fine to include the change to add strerror(errno) in that patch, whose title would be "clone: fix creation of explicitly named target directory".
Unfortunately, if we simply add strerror to the error message, in place of
fatal: could not create work tree dir 'path/'.
the new version would print
fatal: could not create work tree dir 'path/': File exists.
which makes things worse IMO. We could of course strip trailing slashes in builtin-clone.c for now and revert that as soon as the cleanup patch is in, but I think it's not worth the trouble. I suggest we live with the "bug" for now. The error reporting cleanups should be done at a greater scope anyways.
Quoted 8 lines
sha1_file.c int safe_create_leading_directories(char *path) · line 100
Quoted 2 lines
sha1_file.c +1
-1-if (!pos) +if (!pos || !*(pos +1 ))(minor nit) I think
if (!pos || !pos[1])is shorter and easier on the eye.
Will be fixed in the patch to follow.
Clemens
PATCH v2 1/1 ignore trailing slashes when creating leading directories
Currently, create_leading_directories() will interpret all parts of paths like 'a/b/c/' as "leading directories". A subsequent call to mkdir for the tail of the path will fail, because the "File already exists."
This makes sure trailing slashes are ignored.
---
Applies to next. Passes regression tests.
sha1_file.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
sha1_file.c +7 -0
@@ -93,12 +93,19 @@ static inline int offset_1st_component(const char *path)int safe_create_leading_directories (char *path) {char *pos = path +offset_1st_component (path); +char *next;struct stat st;while (pos) { pos =strchr (pos,'/' );if (!pos)break ; + next = pos +1 ; +while (*next =='/' ) + next++; +if (!*next) +break ; + *pos =0 ;if (!stat (path, &st)) {/* path exists */
Signature · 1 line
1.6.0.1.275.gc88b6
Quoted 10 lines · Clemens Buchacher
Unfortunately, if we simply add strerror to the error message, in place of
fatal: could not create work tree dir 'path/'.
the new version would print
fatal: could not create work tree dir 'path/': File exists.
which makes things worse IMO. We could of course strip trailing slashes in
builtin-clone.cfor now and revert that as soon as the cleanup patch is in,
I think the above statement is a wrong way to think about it.
This particular caller (and nobody else), even when it does not want the "create all directories" behaviour, is giving such an instruction to the existing function, without validating the user input. That is a bug that needs to be fixed, regardless of what the final semantics of c-l-d should be, isn't it?
Quoted 1 line
but I think it's not worth the trouble.
What I think is not worth the trouble is, after fixing this caller, to revert that fix, when/if we decide that c-l-d should strip trailing slashes for all callers.
I am not saying the current semantics of c-l-d is optimal. I am just worried about breaking people's private patches that may depend on the current behaviour. It would be nice if we can clean it up without breaking people, and after doing so, if somebody really want to have "create all directories, the last one is also a directory", they can invent a cleaner function that takes that insn as a separate paremeter, not as an obscure "trailing slash means create all", which may be cute but not clean nor clear at all, which is what c-l-d does.
Quoted 8 lines · Junio C Hamano
I am not saying the current semantics of c-l-d is optimal. I am just worried about breaking people's private patches that may depend on the current behaviour. It would be nice if we can clean it up without breaking people, and after doing so, if somebody really want to have "create all directories, the last one is also a directory", they can invent a cleaner function that takes that insn as a separate paremeter, not as an obscure "trailing slash means create all", which may be cute but not clean nor clear at all, which is what c-l-d does.
And just so that you do not misunderstand that I am opposed to the change to c-l-d, here is a replacement to your original patch. I didn't fix the caller, as I didn't want to do an overlapping fix with you.
-- >8 --safe_create_leadign_directories(): make it about "leading" directories
We used to allow callers to pass "foo/bar/" to make sure both "foo" and "foo/bar" exist and have good permissions, but this interface is too error prone. If a caller mistakenly passes a path with trailing slashes (because it forgets to verify the user input, perhaps) even when it wants to later mkdir "bar" itself, it will find that it cannot mkdir "bar". If such a caller does not bother to check the error for EEXIST, it may even errorneously die().
Because we have no existing callers to use that obscure feature, this patch removes it to avoid confusion.
--- sha1_file.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
sha1_file.c +5 -1
@@ -99,7 +99,11 @@ int safe_create_leading_directories(char *path) pos =strchr (pos,'/' );if (!pos)break ; - *pos =0 ; +while (*++pos =='/' ) + ; +if (!*pos) +break ; + *--pos ='\0' ;if (!stat (path, &st)) {/* path exists */ if (!S_ISDIR (st.st_mode)) {
Quoted 3 lines · Junio C Hamano
And just so that you do not misunderstand that I am opposed to the change to c-l-d, here is a replacement to your original patch. I didn't fix the caller, as I didn't want to do an overlapping fix with you.
You beat me to it - again. :-) But I like yours much better.
Clemens
PATCH 1/1 clone: fix creation of explicitly named target directory
In reply to Junio C Hamano'git clone <repo> path/' (note the trailing slash) fails, because the entire path is interpreted as leading directories. So when mkdir tries to create the actual path, it already exists.
This makes sure trailing slashes are removed.
---
Quoted 5 lines · Junio C Hamano
As a "bugfix" patch meant to apply to 'maint', I'd prefer a fix to the caller (
builtin-clone.cthat calls the function), which should be of much less impact. It is fine to include the change to add strerror(errno) in that patch, whose title would be "clone: fix creation of explicitly named target directory".
builtin-clone.c | 20 ++++++++++++++++---- 1 files changed, 16 insertions(+), 4 deletions(-)
builtin-clone.c +16 -4
@@ -147,6 +147,17 @@ static int is_directory(const char *path)return !stat (path, &buf) &&S_ISDIR (buf.st_mode); } +static char *strip_dir_sep (char *dir) +{ +char *end = dir +strlen (dir); + +while (dir < end &&is_dir_sep (end[-1 ])) + end--; + *end ='\0' ; + +return dir; +} +static void setup_reference (const char *repo) {const char *ref_git;
@@ -394,7 +405,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) repo = repo_name;if (argc ==2 ) - dir =xstrdup (argv[1 ]); + dir =strip_dir_sep (xstrdup (argv[1 ]));else dir =guess_dir_name (repo_name, is_bundle, option_bare);
@@ -422,10 +433,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)if (!option_bare) { junk_work_tree = work_tree;if (safe_create_leading_directories_const (work_tree) <0 ) -die ("could not create leading directories of '%s'" , - work_tree); +die ("could not create leading directories of '%s': %s" , + work_tree,strerror (errno));if (mkdir (work_tree,0755 )) -die ("could not create work tree dir '%s'." , work_tree); +die ("could not create work tree dir '%s': %s." , + work_tree,strerror (errno));set_git_work_tree (work_tree); } junk_git_dir = git_dir;
Signature · 1 line
1.6.0
Re: [PATCH] clone: fix creation of explicitly named target directory
Quoted 5 lines · Clemens Buchacher
'git clone <repo> path/' (note the trailing slash) fails, because the entire path is interpreted as leading directories. So when mkdir tries to create the actual path, it already exists.
This makes sure trailing slashes are removed.
Thanks.
builtin-clone.c static int is_directory(const char *path) · line 156
Quoted 7 lines
builtin-clone.c +7
-0+static char *strip_dir_sep (char *dir) +{ +char *end = dir +strlen (dir); + +while (dir < end &&is_dir_sep (end[-1 ])) + end--; + *end ='\0' ;
It does not matter in this particular context, but I'd do "dir < end - 1" to avoid returning the root directory as an empty string, just as a disciplined style; also I'd rename this to strip_trailing_slashes(), make it of type void to make it more clear that it munges the string that is given as the input parameter.
builtin-clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 410
Quoted 8 lines
builtin-clone.c +1
-1@@ -394,7 +405,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) repo = repo_name;if (argc ==2 ) - dir =xstrdup (argv[1 ]); + dir =strip_dir_sep (xstrdup (argv[1 ]));else dir =guess_dir_name (repo_name, is_bundle, option_bare);
Made me wonder if guess_dir_name() can return something with trailing slashes; it turns out that it doesn't, but not very nice. As people's braincycle is more precious, I'd rather say:
if (argc == 2)
dir = xstrdup(argv[1]);
else
dir = guess_dir_name(repo_name, is_bundle, option_bare);
strip_trailing_slashes(dir);I'll queue with the above changes to reduce one round of back-and-forth, but if you see any flaws in my above reasoning, please say so.