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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 17, 2007, 08:28 UTC
Message-ID: <7vtzs3a0xg.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11846059721204-git-send-email-sithglan@stud.uni-erlangen.de>

Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:

Show 25 quoted lines

> Calling unlink on a directory on a Solaris UFS filesystem as root makes it
> inconsistent. Thanks to Johannes Sixt for the obvious fix.
> ---
>  entry.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 82bf725..1f2e34d 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -14,10 +14,10 @@ static void create_directories(const char *path, const struct checkout *state)
>  		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 (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
> +					continue;
>  			}
>  			die("cannot create directory at %s", buf);
>  		}
> -- 
> 1.5.2.1

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 wonder if anybody involved in the discussion has actually tested this patch (or the other one, that has the same problem)?

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.

---
 entry.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/entry.c b/entry.c
index f9e7dc5..42fda36 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 (!stat(buf, &st) && S_ISDIR(st.st_mode))
-					continue; /* ok */
-				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
-					continue;
-			}
+			if (errno == EEXIST && state->force &&
+			    !unlink(buf) && !mkdir(buf, 0777))
+				continue;
 			die("cannot create directory at %s", buf);
 		}
 	}