threads / patch / 15329

patchignore trailing slash when creating leading directories

Subject: [PATCH] ignore trailing slash when creating leading directories

## tl;dr

9 messages between Sep 2, 2008 and Sep 3, 2008. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Clemens Buchacher· Sep 2, 2008, 08:19 UTC · lore

'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 <drizzd@aon.at>
---
 builtin-clone.c |    7 ++++---
 sha1_file.c     |    2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)
Show changes to 2 files +5 −4

builtin-clone.c, sha1_file.c

diff --git a/builtin-clone.c b/builtin-clone.c
index c0e3086..4f945ad 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -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;
diff --git a/sha1_file.c b/sha1_file.c
index 9ee1ed1..3cb9414 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -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)) {

-- 
1.6.0
Junio C Hamano· Sep 2, 2008, 18:38 UTC · re: Clemens Buchacher · lore

Re: [PATCH] ignore trailing slash when creating leading directories

Clemens Buchacher <drizzd@aon.at> writes:
Show 7 quoted lines
> '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 <drizzd@aon.at>
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".

Show 10 quoted lines
> diff --git a/sha1_file.c b/sha1_file.c
> index 9ee1ed1..3cb9414 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -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.
Show 5 quoted lines
>  			break;
>  		*pos = 0;
>  		if (!stat(path, &st)) {
> -- 
> 1.6.0
Clemens Buchacher· Sep 2, 2008, 19:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] ignore trailing slash when creating leading directories

On Tue, Sep 02, 2008 at 11:38:38AM -0700, Junio C Hamano wrote:
>  (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.

Show 5 quoted 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.

Show 5 quoted 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.
Show 5 quoted lines
> 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".
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.

Show 8 quoted lines
> > -		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
Clemens Buchacher· Sep 2, 2008, 20:07 UTC · re: Clemens Buchacher · lore

[PATCH v2] 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.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
Applies to next. Passes regression tests.
 sha1_file.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
Show changes to sha1_file.c +7 −0
diff --git a/sha1_file.c b/sha1_file.c
index 9ee1ed1..dcb3d22 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -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 */

-- 
1.6.0.1.275.gc88b6
Junio C Hamano· Sep 2, 2008, 20:36 UTC · re: Clemens Buchacher · lore

Re: [PATCH] ignore trailing slash when creating leading directories

Clemens Buchacher <drizzd@aon.at> writes:
Show 10 quoted lines
> 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,
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?

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

Junio C Hamano· Sep 2, 2008, 21:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] ignore trailing slash when creating leading directories

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> 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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_file.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Show changes to diff +5 −1
diff --git i/sha1_file.c w/sha1_file.c
index 9ee1ed1..bc6176e 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -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)) {
Clemens Buchacher· Sep 3, 2008, 19:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] ignore trailing slash when creating leading directories

On Tue, Sep 02, 2008 at 02:10:15PM -0700, Junio C Hamano wrote:
> 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
Clemens Buchacher· Sep 3, 2008, 18:55 UTC · re: Junio C Hamano · lore

[PATCH] clone: fix creation of explicitly named target directory

'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.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Tue, Sep 02, 2008 at 11:38:38AM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> 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".
 builtin-clone.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
Show changes to builtin-clone.c +16 −4
diff --git a/builtin-clone.c b/builtin-clone.c
index f44ecea..bbfa7d1 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -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;

-- 
1.6.0
Junio C Hamano· Sep 3, 2008, 19:24 UTC · re: Clemens Buchacher · lore

Re: [PATCH] clone: fix creation of explicitly named target directory

Clemens Buchacher <drizzd@aon.at> writes:
Show 5 quoted lines
> '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.
Show 7 quoted lines
> +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.

Show 8 quoted lines
> @@ -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.

← back to recent threads