Re: [PATCH 2/2] Teach "git-read-tree -u" to check out submodules as a directory

Subsystems: the rest

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 2/2] Teach "git-read-tree -u" to check out submodules as a directory

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:04

Linus Torvalds [off-list ref] writes:

quoted hunk

@@ -136,8 +147,13 @@ static int write_entry(struct cache_entry *ce, char *path, struct checkout *stat
 						 "symlink %s (%s)", path, strerror(errno));
 		}
 		break;
+	case S_IFDIRLNK:
+		if (to_tempfile)
+			return error("git-checkout-index: cannot create temporary subproject %s", path);
+		if (mkdir(path, 0777) < 0)
+			return error("git-checkout-index: cannot create subproject directory %s", path);
+		break;
 	default:
 		return error("git-checkout-index: unknown file mode for %s", path);
 	}
 

Hmm.  Perhaps something like this on top?

diff --git a/entry.c b/entry.c
index 9545e89..0874d61 100644
--- a/entry.c
+++ b/entry.c

@@ -1,6 +1,23 @@
 #include "cache.h"
 #include "blob.h"
 
+static int make_directory(const char *path, int unlink_as_needed)
+{
+	if (mkdir(path, 0777) < 0) {
+		if (errno == EEXIST) {
+			struct stat st;
+			if (unlink_as_needed &&
+			    !unlink(path) &&
+			    !mkdir(path, 0777))
+				return 0; /* ok */
+			if (!stat(path, &st) && S_ISDIR(st.st_mode))
+				return 0; /* ok */
+		}
+		return -1;
+	}
+	return 0;
+}
+
 static void create_directories(const char *path, struct checkout *state)
 {
 	int len = strlen(path);

@@ -8,19 +25,14 @@ static void create_directories(const char *path, struct checkout *state)
 	const char *slash = path;
 
 	while ((slash = strchr(slash+1, '/')) != NULL) {
+		int unlink_as_needed;
+
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
-		if (mkdir(buf, 0777)) {
-			if (errno == EEXIST) {
-				struct stat st;
-				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
-					continue;
-				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
-					continue; /* ok */
-			}
+		unlink_as_needed = (state->base_dir_len < len && state->force);
+		if (make_directory(buf, unlink_as_needed) < 0)
 			die("cannot create directory at %s", buf);
-		}
 	}
 	free(buf);
 }

@@ -150,7 +162,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct checkout *stat
 	case S_IFDIRLNK:
 		if (to_tempfile)
 			return error("git-checkout-index: cannot create temporary subproject %s", path);
-		if (mkdir(path, 0777) < 0)
+		if (make_directory(path, 0))
 			return error("git-checkout-index: cannot create subproject directory %s", path);
 		break;
 	default:

Re: [PATCH 2/2] Teach "git-read-tree -u" to check out submodules as a directory

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:43:04


On Thu, 12 Apr 2007, Junio C Hamano wrote:

quoted hunk

Hmm.  Perhaps something like this on top?

diff --git a/entry.c b/entry.c
index 9545e89..0874d61 100644
--- a/entry.c
+++ b/entry.c

@@ -1,6 +1,23 @@
 #include "cache.h"
 #include "blob.h"
 
+static int make_directory(const char *path, int unlink_as_needed)

I actually tried to think this through, and I don't *think* we should need 
to do this. It should literally be sufficient to just do the "mkdir()" 
call.

Why? Because we've already done the "create_directories()" calls earlier, 
and if you actually look at "create_file()", that one also just does a 
simple "open(path, O_WRONLY | O_CREAT | O_EXCL, mode)" - so doing just the 
mkdir() for creating a subdirectory should be the logically equivalent 
operation.

Similarly, the S_IFLNK case just does a "symlink()" system call. No "try 
to unlink if there was an old entry there before" code.

Basically, once we are inside "write_entry()", we expect to get a success 
or a failure, not a "needs cleanup". If it needed resolving of other state 
before, we shouldn't even have gotten to that stage in the first place.

That said, looking closer, you're right - the cleanup that is done earlier 
is actually wrong for the gitlink case. So I think the *real* problem is 
the cleanup in "checkout_entry()". In particular, note the

	if (!lstat(path, &st)) {
		...
		if (S_ISDIR(st.st_mode)) {
			...
		}
	}

code. If there was a directory there, we should actually leave it alone 
for the gitlink case, because it is up to the *subproject* checkout (not 
the superproject checkout) to handle any issues within that 
subdirectory!

So I think the *right* patch (on top of the one I send out) is actually 
just this:

		Linus

---

diff --git a/entry.c b/entry.c
index 9545e89..50ffae4 100644
--- a/entry.c
+++ b/entry.c

@@ -195,6 +195,9 @@ int checkout_entry(struct cache_entry *ce, struct checkout *state, char *topath)
 		 */
 		unlink(path);
 		if (S_ISDIR(st.st_mode)) {
+			/* If it is a gitlink, leave it alone! */
+			if (S_ISDIRLNK(ntohl(ce->ce_mode)))
+				return 0;
 			if (!state->force)
 				return error("%s is a directory", path);
 			remove_subtree(path);

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help