Re: [PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory

On Thu, 19 Apr 2007, Alex Riesen wrote:

On 4/19/07, Alex Riesen [off-list ref] wrote:

quoted

quoted

So here's a suggested and totally untested patch. It makes the code more
readable, and probably fixes *two* bugs in the process. It also simply
doesn't really even care what the error actually was - the important part
was not that it was a directory, but that the unlink didn't succeed!

Well, it is a bit tested now. I'll repeat the testing tomorrow on that
windows box.

Tested on windows too. Works.

Good. Junio, I'd really suggest applying it. The old code was literally 
wrong, and depended on an error return that seems to be Linux-specific. It 
was also pretty ugly.

Here's the patch again with proper sign-off and a commentary..

As mentioned, maybe this wants expanding in the future, but regardless, 
the patch not only fixes a git problem on windows (and quite possibly 
other unixes too), any extensions will be much easier on top of this.

		Linus

---
From: Linus Torvalds <torvalds@linux-foundation.org>
Subject: Fix working directory errno handling when unlinking a directory

Alex Riesen noticed that the case where a file replaced a directory entry 
in the working tree was broken on cygwin. It turns out that the code made 
some Linux-specific assumptions, and also ignored errors entirely for the 
case where the entry was a symlink rather than a file.

This cleans it up by separating out the common case into a function of its 
own, so that both regular files and symlinks can share it, and by making 
the error handling more obvious (and not depend on any Linux-specific 
behaviour).

Acked-by: Alex Riesen <redacted>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

---
 merge-recursive.c |   54 ++++++++++++++++++++++++++++------------------------
 1 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 595b022..cea6c87 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -574,6 +574,31 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
 	}
 }
 
+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);
+	if (status) {
+		if (status == -3) {
+			/* something else exists */
+			error(msg, path, ": perhaps a D/F conflict?");
+			return -1;
+		}
+		die(msg, path, "");
+	}
+
+	/* Successful unlink is good.. */
+	if (!unlink(path))
+		return 0;
+	/* .. and so is no existing file */
+	if (errno == ENOENT)
+		return 0;
+	/* .. but not some other error (who really cares what?) */
+	return error(msg, path, ": perhaps a D/F conflict?");
+}
+
 static void update_file_flags(const unsigned char *sha,
 			      unsigned mode,
 			      const char *path,

@@ -594,33 +619,12 @@ static void update_file_flags(const unsigned char *sha,
 		if (type != OBJ_BLOB)
 			die("blob expected for %s '%s'", sha1_to_hex(sha), path);
 
+		if (make_room_for_path(path) < 0) {
+			update_wd = 0;
+			goto update_index;
+		}
 		if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {
 			int fd;
-			int status;
-			const char *msg = "failed to create path '%s'%s";
-
-			status = mkdir_p(path, 0777);
-			if (status) {
-				if (status == -3) {
-					/* something else exists */
-					error(msg, path, ": perhaps a D/F conflict?");
-					update_wd = 0;
-					goto update_index;
-				}
-				die(msg, path, "");
-			}
-			if (unlink(path)) {
-				if (errno == EISDIR) {
-					/* something else exists */
-					error(msg, path, ": perhaps a D/F conflict?");
-					update_wd = 0;
-					goto update_index;
-				}
-				if (errno != ENOENT)
-					die("failed to unlink %s "
-					    "in preparation to update: %s",
-					    path, strerror(errno));
-			}
 			if (mode & 0100)
 				mode = 0777;
 			else