[PATCH] ignore trailing slash when creating leading directories

[PATCH] ignore trailing slash when creating leading directories

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

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

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

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

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

>  			break;
>  		*pos = 0;
>  		if (!stat(path, &st)) {
> -- 
> 1.6.0

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

[Clemens Buchacher]

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.

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

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

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

> > -		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] ignore trailing slashes when creating leading directories

[Clemens Buchacher]

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(-)

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

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

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

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

[Junio C Hamano]

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

> 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(-)

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)) {

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

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

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

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

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

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

[Clemens Buchacher]

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

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

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

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

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