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

Clemens Buchacher [off-list ref] 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 <redacted>

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

(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

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.

quoted

-		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

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

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

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

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