[PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM

[PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM

[Alex Riesen]

... and the directory already exists. I.E. Cygwin is such
a case: mkdir fail for mounts which reference directly
to windows mounts ("drives").

---

The discussion, which ended up with this patch can be read
here: http://www.cygwin.com/ml/cygwin/2006-01/msg01276.html

BTW, there is this:
http://www.cygwin.com/ml/cygwin/2006-01/msg01380.html
So this patch will probably be not needed soon.

Still I post it just for reference, or in case someone
can't afford an upgrade.

 Makefile          |    8 ++++++++
 checkout-index.c  |    2 +-
 compat/mkdir.c    |   23 +++++++++++++++++++++++
 git-compat-util.h |    6 ++++++
 4 files changed, 38 insertions(+), 1 deletions(-)
 create mode 100644 compat/mkdir.c

c9f480f5d1e777cc0f4861e94c2449bd8c7cf73a
diff --git a/Makefile b/Makefile
index 2e95353..596c2d8 100644
--- a/Makefile
+++ b/Makefile
@@ -66,6 +66,9 @@ all:
 # Define USE_STDEV below if you want git to care about the underlying device
 # change being considered an inode change from the update-cache perspective.
 
+# Define FIX_MKDIR_ERRNO if your mkdir(2) does not always set errno to EEXIST
+# if it failed because of existing directory (notably Cygwin).
+
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
@@ -249,6 +252,7 @@ ifeq ($(uname_O),Cygwin)
 	# Try uncommenting this if you see things break -- YMMV.
 	# NO_MMAP = YesPlease
 	NO_IPV6 = YesPlease
+	FIX_MKDIR_ERRNO = YesPlease
 	X = .exe
 endif
 ifeq ($(uname_S),OpenBSD)
@@ -392,6 +396,10 @@ else
 endif
 endif
 endif
+ifdef FIX_MKDIR_ERRNO
+	COMPAT_CFLAGS += -DFIX_MKDIR_ERRNO
+	COMPAT_OBJS += compat/mkdir.o
+endif
 
 ALL_CFLAGS += -DSHA1_HEADER=$(call shellquote,$(SHA1_HEADER)) $(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
diff --git a/checkout-index.c b/checkout-index.c
index 53dd8cb..c5a6d6e 100644
--- a/checkout-index.c
+++ b/checkout-index.c
@@ -199,6 +199,6 @@ int main(int argc, char **argv)
 	if (0 <= newfd &&
 	    (write_cache(newfd, active_cache, active_nr) ||
 	     commit_index_file(&cache_file)))
-		die("Unable to write new cachefile");
+		die("Unable to write new cachefile: %s", strerror(errno));
 	return 0;
 }
diff --git a/compat/mkdir.c b/compat/mkdir.c
new file mode 100644
index 0000000..1c2260c
--- /dev/null
+++ b/compat/mkdir.c
@@ -0,0 +1,23 @@
+#include <unistd.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include "../git-compat-util.h"
+#undef mkdir
+
+int gitmkdir(const char *dir, mode_t mode)
+{
+	int rc = mkdir(dir, mode);
+	if ( rc < 0 ) {
+		int e  = errno;
+		if ( EACCES == e || EPERM == e )
+		{
+			struct stat st;
+			if ( stat(dir, &st) == 0 )
+				errno = EEXIST;
+			else
+				errno = e;
+		}
+	}
+	return rc;
+}
+
diff --git a/git-compat-util.h b/git-compat-util.h
index f982b8e..9cf3aab 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -154,4 +154,10 @@ static inline int sane_case(int x, int h
 #ifndef MAXPATHLEN
 #define MAXPATHLEN 256
 #endif
+
+#ifdef FIX_MKDIR_ERRNO
+#define mkdir gitmkdir
+int gitmkdir(const char *dir, mode_t mode);
+#endif
+
 #endif
-- 
1.1.4.g7397-dirty

Re: [PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> ... and the directory already exists. I.E. Cygwin is such
> a case: mkdir fail for mounts which reference directly
> to windows mounts ("drives").
>
> ---
>
> The discussion, which ended up with this patch can be read
> here: http://www.cygwin.com/ml/cygwin/2006-01/msg01276.html
>
> BTW, there is this:
> http://www.cygwin.com/ml/cygwin/2006-01/msg01380.html
> So this patch will probably be not needed soon.

Thanks for all the background information.  Although it is very
tempting for me to adopt your patch as an easy way out, I would
feel *dirty* if I did so.

Eric Blake is right in his argument in that thread.  Our code
should not depend on the Linux EEXIST behaviour.  The reason
Cygwin folks want to be as close to Linux is to work around
application bugs like this.  Which is a valid concern for them,
but it does not mean the application has license to depend on
Linux behaviour.

We, as an application, should take a different attitude -- we
should fix things to make things portable, not work things
around, if both are equally easily doable.  I do not object to
using a function that has a semantics Linux mkdir() gives us,
but calling that mkdir() does not feel quite right.  

Also the wrapper implies that in our future use of mkdir() we
cannot tell the difference between EEXIST and other errors if we
later wanted to.

So let's look at our existing uses first:

apply.c:1577:		if (mkdir(buf, 0777) < 0) {
apply.c-1578-			if (errno != EEXIST)
apply.c-1579-				break;
apply.c-1580-		}
apply.c-1581-	}

This is "we want to see buf directory exist and we would create
one if there isn't".  Instead of checking errno, we could stat
as your patch does.

entry.c:15:		if (mkdir(buf, 0777)) {
entry.c-16-			if (errno == EEXIST) {
entry.c-17-				struct stat st;
entry.c:18:				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
entry.c-19-					continue;
entry.c-20-				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
entry.c-21-					continue; /* ok */
entry.c-22-			}

This is a bit more involved. "we want to have buf directory and
we would create it, and if force is given to have something
under that directory, we would even unlink the nondirectory that
sits there".

I asked "git-grep" where else we use mkdir(), and all other
users want the semantics of the apply.c use quoted above.

So I'd rather see us create a generic helper function like this:

	int make_directory(const char *path, int force)
        {
		struct stat st;
		int mkdir_errno;
		if (!mkdir(path, 0777))
			return 0;
                mkdir_errno = errno;
		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
                	return 0;
                if (!force) {
		bad:
                	errno = mkdir_errno;
                        return -1;
                }
                if (!unlink(path, &st) && !mkdir(path, 0777))
                	return 0;
                /* we might have failed to unlink an existing symlink
		 * which happens to point at an existing directory; that
                 * directory is not what we want here.
                 */
		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
                	return 0;
		goto bad;
        }

and have current callers of mkdir() use it, regardless of the
platform.  It may not worth saving mkdir_errno, though.

Then everybody but entry.c one would say force=0, and entry.c
one passes force appropriately using the condition it uses in
its current if() statement.

Re: [PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM

[Alex Riesen]

Junio C Hamano, Mon, Jan 30, 2006 21:33:51 +0100:
> and have current callers of mkdir() use it, regardless of the
> platform.  It may not worth saving mkdir_errno, though.

errno may be worth saving. I find the process of finding
"why-the-f$%^-did-the-windows-broke-again" really tedious:
1. find the application which failed (grep for the die message)
2. put "%s", strerror(errno) in the die
3. retest
4. find out errno is 0 (success)
5. remove close(fd), munmap, whatever before the die
6. retest
7. repeat

> Then everybody but entry.c one would say force=0, and entry.c
> one passes force appropriately using the condition it uses in
> its current if() statement.

I like it :)