Git clone behaviour change in 1.5.6 (vs 1.5.5.1)

Git clone behaviour change in 1.5.6 (vs 1.5.5.1)

[Kalle Vahlman]

Hi!

Switching to the 1.5.6 release from 1.5.5.1, I found out that the
rewritten git-clone command changed its behaviour wrt cloning to a
non-existing destination directory structure. In the shell version the
destination (work tree) is created with 'mkdir -p' but in the C
version with just the mkdir() call which doesn't create the parent
directories.

I can't find any indication that this would be intended in the repo
history nor in the mailing list, so I'm left thinking that this is an
unwanted regression. Could someone confirm this?

-- 
Kalle Vahlman, zuh@iki.fi
Powered by http://movial.fi
Interesting stuff at http://syslog.movial.fi

Re: Git clone behaviour change in 1.5.6 (vs 1.5.5.1)

[Daniel Barkalow]

On Mon, 23 Jun 2008, Kalle Vahlman wrote:

> Hi!
> 
> Switching to the 1.5.6 release from 1.5.5.1, I found out that the
> rewritten git-clone command changed its behaviour wrt cloning to a
> non-existing destination directory structure. In the shell version the
> destination (work tree) is created with 'mkdir -p' but in the C
> version with just the mkdir() call which doesn't create the parent
> directories.
> 
> I can't find any indication that this would be intended in the repo
> history nor in the mailing list, so I'm left thinking that this is an
> unwanted regression. Could someone confirm this?

It wasn't an intentional change, anyway.

	-Daniel
*This .sig left intentionally blank*

Re: Git clone behaviour change in 1.5.6 (vs 1.5.5.1)

[Jeff King]

On Mon, Jun 23, 2008 at 03:56:41PM -0400, Daniel Barkalow wrote:

> > Switching to the 1.5.6 release from 1.5.5.1, I found out that the
> > rewritten git-clone command changed its behaviour wrt cloning to a
> > non-existing destination directory structure. In the shell version the
> > destination (work tree) is created with 'mkdir -p' but in the C
> > version with just the mkdir() call which doesn't create the parent
> > directories.
> > 
> > I can't find any indication that this would be intended in the repo
> > history nor in the mailing list, so I'm left thinking that this is an
> > unwanted regression. Could someone confirm this?
> 
> It wasn't an intentional change, anyway.

Here is a partial fix. For --bare repo creation, we follow a different
codepath, and I think the fix is likely to be a bit tricky. We actually
fail in make_absolute_path, before we try creating the directory. But I
suspect we can't just create the directory ourselves because init_db
expects it not to exist.

-- >8 --
clone: create intermediate directories of repo destination

The shell version used to use "mkdir -p" to create the repo
path, but the C version just calls "mkdir". Let's replicate
the old behavior.
---
 builtin-clone.c  |   40 +++++++++++++++++++++++++++++++++++++++-
 t/t5601-clone.sh |   14 ++++++++++++++
 2 files changed, 53 insertions(+), 1 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 5c5acb4..758d02c 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -168,6 +168,44 @@ static void setup_reference(const char *repo)
 	free(ref_git_copy);
 }
 
+static int mkdirp_recurse(char *path, int mode)
+{
+	int n;
+	char tmp;
+	int r;
+
+	if (mkdir(path, mode) == 0)
+		return 0;
+	if (errno != ENOENT)
+		return -1;
+
+	for (n = strlen(path); n > 0 && path[n] != '/'; n--)
+		;
+	if (n == 0)
+		return -1;
+	tmp = path[n];
+	path[n] = '\0';
+	r = mkdirp_recurse(path, mode);
+	path[n] = tmp;
+	if (r < 0)
+		return -1;
+	return mkdir(path, mode);
+}
+
+static int mkdirp(const char *path, int mode)
+{
+	char buf[PATH_MAX];
+	int n;
+
+	n = strlen(path);
+	if (n >= PATH_MAX) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+	memcpy(buf, path, n+1);
+	return mkdirp_recurse(buf, mode);
+}
+
 static void copy_or_link_directory(char *src, char *dest)
 {
 	struct dirent *de;
@@ -404,7 +442,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	if (!option_bare) {
 		junk_work_tree = work_tree;
-		if (mkdir(work_tree, 0755))
+		if (mkdirp(work_tree, 0755))
 			die("could not create work tree dir '%s'.", work_tree);
 		set_git_work_tree(work_tree);
 	}
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 593d1a3..c2c83f0 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -30,4 +30,18 @@ test_expect_success 'clone checks out files' '
 
 '
 
+test_expect_success 'clone creates intermediate directories' '
+
+	git clone src long/path/to/dst &&
+	test -f long/path/to/dst/file
+
+'
+
+test_expect_failure 'clone creates intermediate directories for bare repo' '
+
+	git clone --bare src long/path/to/bare/dst &&
+	test -f long/path/to/bare/dst/config
+
+'
+
 test_done
-- 
1.5.6.105.gceec6.dirty

Re: Git clone behaviour change in 1.5.6 (vs 1.5.5.1)

[Brandon Casey]

Jeff King wrote:
> On Mon, Jun 23, 2008 at 03:56:41PM -0400, Daniel Barkalow wrote:
> 
>>> Switching to the 1.5.6 release from 1.5.5.1, I found out that the
>>> rewritten git-clone command changed its behaviour wrt cloning to a
>>> non-existing destination directory structure. In the shell version the
>>> destination (work tree) is created with 'mkdir -p' but in the C
>>> version with just the mkdir() call which doesn't create the parent
>>> directories.

> Here is a partial fix.

There is a mkdir_p() in builtin-merge-recursive.c which calls
safe_create_leading_directories() in sha1_file.c

Can these functions be reused?


-brandon

[PATCH] clone: create intermediate directories of destination repo

[Jeff King]

The shell version used to use "mkdir -p" to create the repo
path, but the C version just calls "mkdir". Let's replicate
the old behavior. In this case we can simply create the
directories leading up to the git dir. If it's a bare repo,
then that is everything that init_db wants ahead of time. If
it isn't bare, then the worktree contains the git dir, so we
create the worktree.

We can reuse safe_create_leading_directories, but we need to
make a copy of our const buffer to do so. Since
merge-recursive uses the same pattern, we can factor this
out into a global function. This has two other cleanup
advantages for merge-recursive:

  1. mkdir_p wasn't a very good name. "mkdir -p foo/bar" actually
     creates bar, but this function just creates the leading
     directories.

  2. mkdir_p took a mode argument, but it was completely
     ignored.
---
On Mon, Jun 23, 2008 at 04:03:47PM -0500, Brandon Casey wrote:

> There is a mkdir_p() in builtin-merge-recursive.c which calls
> safe_create_leading_directories() in sha1_file.c
> 
> Can these functions be reused?

Thanks, I hadn't noticed those for some reason. This version uses the
existing code, and correctly handles both the bare and worktree cases.

The big difference is that safe_create_leading_directories will do an
adjust_shared_perm on the result. I don't think that should be a
problem, but it is a difference.

 builtin-clone.c           |    5 +++--
 builtin-merge-recursive.c |   13 ++-----------
 cache.h                   |    1 +
 sha1_file.c               |    9 +++++++++
 t/t5601-clone.sh          |   14 ++++++++++++++
 5 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 7190952..e951911 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -398,10 +398,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		git_dir = xstrdup(mkpath("%s/.git", dir));
 	}
 
+	if (safe_create_leading_directories_const(git_dir) < 0)
+		die("could not create leading directories of '%s'", git_dir);
+
 	if (!option_bare) {
 		junk_work_tree = work_tree;
-		if (mkdir(work_tree, 0755))
-			die("could not create work tree dir '%s'.", work_tree);
 		set_git_work_tree(work_tree);
 	}
 	junk_git_dir = git_dir;
diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 4aa28a1..43bf6aa 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c
@@ -481,15 +481,6 @@ static char *unique_path(const char *path, const char *branch)
 	return newpath;
 }
 
-static int mkdir_p(const char *path, unsigned long mode)
-{
-	/* path points to cache entries, so xstrdup before messing with it */
-	char *buf = xstrdup(path);
-	int result = safe_create_leading_directories(buf);
-	free(buf);
-	return result;
-}
-
 static void flush_buffer(int fd, const char *buf, unsigned long size)
 {
 	while (size > 0) {
@@ -512,7 +503,7 @@ static int make_room_for_path(const char *path)
 	int status;
 	const char *msg = "failed to create path '%s'%s";
 
-	status = mkdir_p(path, 0777);
+	status = safe_create_leading_directories_const(path);
 	if (status) {
 		if (status == -3) {
 			/* something else exists */
@@ -583,7 +574,7 @@ static void update_file_flags(const unsigned char *sha,
 			close(fd);
 		} else if (S_ISLNK(mode)) {
 			char *lnk = xmemdupz(buf, size);
-			mkdir_p(path, 0777);
+			safe_create_leading_directories_const(path);
 			unlink(path);
 			symlink(lnk, path);
 			free(lnk);
diff --git a/cache.h b/cache.h
index d12ee7d..49f20dd 100644
--- a/cache.h
+++ b/cache.h
@@ -517,6 +517,7 @@ enum sharedrepo {
 int git_config_perm(const char *var, const char *value);
 int adjust_shared_perm(const char *path);
 int safe_create_leading_directories(char *path);
+int safe_create_leading_directories_const(const char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
diff --git a/sha1_file.c b/sha1_file.c
index 191f814..71be0c9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -116,6 +116,15 @@ int safe_create_leading_directories(char *path)
 	return 0;
 }
 
+int safe_create_leading_directories_const(const char *path)
+{
+	/* path points to cache entries, so xstrdup before messing with it */
+	char *buf = xstrdup(path);
+	int result = safe_create_leading_directories(buf);
+	free(buf);
+	return result;
+}
+
 char *sha1_to_hex(const unsigned char *sha1)
 {
 	static int bufno;
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 593d1a3..c2c83f0 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -30,4 +30,18 @@ test_expect_success 'clone checks out files' '
 
 '
 
+test_expect_success 'clone creates intermediate directories (non-bare)' '
+
+	git clone src long/path/to/dst &&
+	test -f long/path/to/dst/file
+
+'
+
+test_expect_success 'clone creates intermediate directories (bare)' '
+
+	git clone --bare src long/path/to/bare/dst &&
+	test -f long/path/to/bare/dst/config
+
+'
+
 test_done
-- 
1.5.6.50.gae760.dirty

Re: [PATCH] clone: create intermediate directories of destination repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The shell version used to use "mkdir -p" to create the repo path, but
> the C version just calls "mkdir". Let's replicate the old behavior. In
> this case we can simply create the directories leading up to the git
> dir. If it's a bare repo, then that is everything that init_db wants
> ahead of time. If it isn't bare, then the worktree contains the git dir,
> so we create the worktree.

Clever ;-)

> The big difference is that safe_create_leading_directories will do an
> adjust_shared_perm on the result. I don't think that should be a
> problem, but it is a difference.

This early in the code you would not have read anything that triggers
"shared" so it probably is not a problem, I would think.

Re: [PATCH] clone: create intermediate directories of destination repo

[Jeff King]

On Tue, Jun 24, 2008 at 12:39:40AM -0700, Junio C Hamano wrote:

> > The shell version used to use "mkdir -p" to create the repo path, but
> > the C version just calls "mkdir". Let's replicate the old behavior. In
> > this case we can simply create the directories leading up to the git
> > dir. If it's a bare repo, then that is everything that init_db wants
> > ahead of time. If it isn't bare, then the worktree contains the git dir,
> > so we create the worktree.
> 
> Clever ;-)

I am worried that it is too clever. I didn't see an obvious way for
work_tree and git_dir to not have that property, but I think it is still
worth somebody double-checking.

-Peff

Re: [PATCH] clone: create intermediate directories of destination repo

[Daniel Barkalow]

On Tue, 24 Jun 2008, Jeff King wrote:

> On Tue, Jun 24, 2008 at 12:39:40AM -0700, Junio C Hamano wrote:
> 
> > > The shell version used to use "mkdir -p" to create the repo path, but
> > > the C version just calls "mkdir". Let's replicate the old behavior. In
> > > this case we can simply create the directories leading up to the git
> > > dir. If it's a bare repo, then that is everything that init_db wants
> > > ahead of time. If it isn't bare, then the worktree contains the git dir,
> > > so we create the worktree.
> > 
> > Clever ;-)
> 
> I am worried that it is too clever. I didn't see an obvious way for
> work_tree and git_dir to not have that property, but I think it is still
> worth somebody double-checking.

I think you can specify git_dir and work_tree on the command line, and set 
them to whatever you want, although I now don't remember whether they're 
actually both followed for clone (I tried to match the shell version, 
whose behavior didn't make too much sense to me).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] clone: create intermediate directories of destination repo

[Jeff King]

On Tue, Jun 24, 2008 at 11:20:07AM -0400, Daniel Barkalow wrote:

> > I am worried that it is too clever. I didn't see an obvious way for
> > work_tree and git_dir to not have that property, but I think it is still
> > worth somebody double-checking.
> 
> I think you can specify git_dir and work_tree on the command line, and set 
> them to whatever you want, although I now don't remember whether they're 
> actually both followed for clone (I tried to match the shell version, 
> whose behavior didn't make too much sense to me).

The git_dir variable is always set from the directory provided on the
command line. However, the work_tree can be overridden by the
environment. For some reason I missed the giant 'work_tree =
getenv("GIT_WORK_TREE")' when I looked before. But that means that they
are not necessarily related.

So my original patch was too clever. Here is the less clever patch, with
an extra test that would break with the clever version.

-- >8 --
clone: create intermediate directories of destination repo

The shell version used to use "mkdir -p" to create the repo
path, but the C version just calls "mkdir". Let's replicate
the old behavior. We have to create the git and worktree
leading dirs separately; while most of the time, the
worktree dir contains the git dir (as .git), the user can
override this using GIT_WORK_TREE.

We can reuse safe_create_leading_directories, but we need to
make a copy of our const buffer to do so. Since
merge-recursive uses the same pattern, we can factor this
out into a global function. This has two other cleanup
advantages for merge-recursive:

  1. mkdir_p wasn't a very good name. "mkdir -p foo/bar" actually
     creates bar, but this function just creates the leading
     directories.

  2. mkdir_p took a mode argument, but it was completely
     ignored.
---
 builtin-clone.c           |    5 +++++
 builtin-merge-recursive.c |   13 ++-----------
 cache.h                   |    1 +
 sha1_file.c               |    9 +++++++++
 t/t5601-clone.sh          |   22 ++++++++++++++++++++++
 5 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 7190952..b2dfe1a 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -400,6 +400,9 @@ 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);
 		if (mkdir(work_tree, 0755))
 			die("could not create work tree dir '%s'.", work_tree);
 		set_git_work_tree(work_tree);
@@ -410,6 +413,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	setenv(CONFIG_ENVIRONMENT, xstrdup(mkpath("%s/config", git_dir)), 1);
 
+	if (safe_create_leading_directories_const(git_dir) < 0)
+		die("could not create leading directories of '%s'", git_dir);
 	set_git_dir(make_absolute_path(git_dir));
 
 	fprintf(stderr, "Initialize %s\n", git_dir);
diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 4aa28a1..43bf6aa 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c
@@ -481,15 +481,6 @@ static char *unique_path(const char *path, const char *branch)
 	return newpath;
 }
 
-static int mkdir_p(const char *path, unsigned long mode)
-{
-	/* path points to cache entries, so xstrdup before messing with it */
-	char *buf = xstrdup(path);
-	int result = safe_create_leading_directories(buf);
-	free(buf);
-	return result;
-}
-
 static void flush_buffer(int fd, const char *buf, unsigned long size)
 {
 	while (size > 0) {
@@ -512,7 +503,7 @@ static int make_room_for_path(const char *path)
 	int status;
 	const char *msg = "failed to create path '%s'%s";
 
-	status = mkdir_p(path, 0777);
+	status = safe_create_leading_directories_const(path);
 	if (status) {
 		if (status == -3) {
 			/* something else exists */
@@ -583,7 +574,7 @@ static void update_file_flags(const unsigned char *sha,
 			close(fd);
 		} else if (S_ISLNK(mode)) {
 			char *lnk = xmemdupz(buf, size);
-			mkdir_p(path, 0777);
+			safe_create_leading_directories_const(path);
 			unlink(path);
 			symlink(lnk, path);
 			free(lnk);
diff --git a/cache.h b/cache.h
index a68866c..a9c14da 100644
--- a/cache.h
+++ b/cache.h
@@ -517,6 +517,7 @@ enum sharedrepo {
 int git_config_perm(const char *var, const char *value);
 int adjust_shared_perm(const char *path);
 int safe_create_leading_directories(char *path);
+int safe_create_leading_directories_const(const char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
diff --git a/sha1_file.c b/sha1_file.c
index 9330bc4..0d52e59 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -116,6 +116,15 @@ int safe_create_leading_directories(char *path)
 	return 0;
 }
 
+int safe_create_leading_directories_const(const char *path)
+{
+	/* path points to cache entries, so xstrdup before messing with it */
+	char *buf = xstrdup(path);
+	int result = safe_create_leading_directories(buf);
+	free(buf);
+	return result;
+}
+
 char *sha1_to_hex(const unsigned char *sha1)
 {
 	static int bufno;
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 593d1a3..b642fb2 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -30,4 +30,26 @@ test_expect_success 'clone checks out files' '
 
 '
 
+test_expect_success 'clone respects GIT_WORK_TREE' '
+
+	GIT_WORK_TREE=worktree git clone src bare &&
+	test -f bare/config &&
+	test -f worktree/file
+
+'
+
+test_expect_success 'clone creates intermediate directories' '
+
+	git clone src long/path/to/dst &&
+	test -f long/path/to/dst/file
+
+'
+
+test_expect_success 'clone creates intermediate directories for bare repo' '
+
+	git clone --bare src long/path/to/bare/dst &&
+	test -f long/path/to/bare/dst/config
+
+'
+
 test_done
-- 
1.5.6.57.g56b3.dirty

Re: [PATCH] clone: create intermediate directories of destination repo

[Daniel Barkalow]

On Wed, 25 Jun 2008, Jeff King wrote:

> On Tue, Jun 24, 2008 at 11:20:07AM -0400, Daniel Barkalow wrote:
> 
> > > I am worried that it is too clever. I didn't see an obvious way for
> > > work_tree and git_dir to not have that property, but I think it is still
> > > worth somebody double-checking.
> > 
> > I think you can specify git_dir and work_tree on the command line, and set 
> > them to whatever you want, although I now don't remember whether they're 
> > actually both followed for clone (I tried to match the shell version, 
> > whose behavior didn't make too much sense to me).
> 
> The git_dir variable is always set from the directory provided on the
> command line. However, the work_tree can be overridden by the
> environment. For some reason I missed the giant 'work_tree =
> getenv("GIT_WORK_TREE")' when I looked before. But that means that they
> are not necessarily related.
> 
> So my original patch was too clever. Here is the less clever patch, with
> an extra test that would break with the clever version.

Looks good, thanks.

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

	-Daniel
*This .sig left intentionally blank*