Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann <thomas@glanzmann.de>]

Hello Junio,

> This is wrong.  If the filesystem has a symlink and we would want a
> directory there, we should unlink().  So at least the stat there needs
> to be lstat().

I see.

> I wonder if anybody involved in the discussion has actually
> tested this patch (or the other one, that has the same problem)?

I tested it. But I did not test it with symlinks.

> Does the following replacement work for you?  It adds far more lines
> than your version, but they are mostly comments to make it clear why
> we do things this way.

Yes, it does. Excuse the delay but my build machine is not the fastest.

	(faui04a) [/var/tmp] git clone ~/work/repositories/public/easix.git test-10
	Initialized empty Git repository in /var/tmp/test-10/.git/
	remote: Generating pack...
	remote: Done counting 317 objects.
	remote: Deltifying 317 objects...
	remote: te: % (317/317) done: ) done
	Indexing 317 objects...
	remote: Total 317 (delta 182), reused 278 (delta 157)
	100% (317/317) done
	Resolving 182 deltas...
	100% (182/182) done
	(faui04a) [/var/tmp] cd test-10
	./test-10
	(faui04a) [/var/tmp/test-10] git status
	# On branch master
	nothing to commit (working directory clean)

I rebased your patch on top of current HEAD (as I can access it on
git.kernel.org) and removed trailing whitspace from one line (git-apply
complained)

	Thomas

>From 3b60b807007507ce5e1f8490f1469dac5bb95917 Mon Sep 17 00:00:00 2001
From: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
Date: Tue, 17 Jul 2007 11:31:07 +0200
Subject: [PATCH] Do _not_ call unlink on a directory

Calling unlink on a directory on a Solaris UFS filesystem as root makes it
inconsistent. Thanks to Junio for the not so obvious fix.
---
 entry.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/entry.c b/entry.c
index c540ae1..0625112 100644
--- a/entry.c
+++ b/entry.c
@@ -8,17 +8,40 @@ static void create_directories(const char *path, const struct checkout *state)
 	const char *slash = path;
 
 	while ((slash = strchr(slash+1, '/')) != NULL) {
+		struct stat st;
+		int stat_status;
+
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
+
+		if (len <= state->base_dir_len)
+			/*
+			 * checkout-index --prefix=<dir>; <dir> is
+			 * allowed to be a symlink to an existing
+			 * directory.
+			 */
+			stat_status = stat(buf, &st);
+		else
+			/*
+			 * if there currently is a symlink, we would
+			 * want to replace it with a real directory.
+			 */
+			stat_status = lstat(buf, &st);
+
+		if (!stat_status && S_ISDIR(st.st_mode))
+			continue; /* ok, it is already a directory. */
+
+		/*
+		 * We know stat_status == 0 means something exists
+		 * there and this mkdir would fail, but that is an
+		 * error codepath; we do not care, as we unlink and
+		 * mkdir again in such a case.
+		 */
 		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 */
-			}
+			if (errno == EEXIST && state->force &&
+			    !unlink(buf) && !mkdir(buf, 0777))
+				continue;
 			die("cannot create directory at %s", buf);
 		}
 	}
-- 
1.5.2.1