{"thread":{"id":"3195","subject":"[PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM","startedAt":"2006-01-30T19:38:39Z","lastAt":"2006-01-30T23:16:22Z","messageCount":3,"participants":["Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"15275","messageId":"20060130193839.GA6575@steel.home","threadId":"3195","inReplyTo":null,"subject":"[PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2006-01-30T19:38:39Z","receivedAt":"2006-01-30T19:38:39Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"... and the directory already exists. I.E. Cygwin is such\na case: mkdir fail for mounts which reference directly\nto windows mounts (\"drives\").\n\n---\n\nThe discussion, which ended up with this patch can be read\nhere: http://www.cygwin.com/ml/cygwin/2006-01/msg01276.html\n\nBTW, there is this:\nhttp://www.cygwin.com/ml/cygwin/2006-01/msg01380.html\nSo this patch will probably be not needed soon.\n\nStill I post it just for reference, or in case someone\ncan't afford an upgrade.\n\n Makefile          |    8 ++++++++\n checkout-index.c  |    2 +-\n compat/mkdir.c    |   23 +++++++++++++++++++++++\n git-compat-util.h |    6 ++++++\n 4 files changed, 38 insertions(+), 1 deletions(-)\n create mode 100644 compat/mkdir.c\n\nc9f480f5d1e777cc0f4861e94c2449bd8c7cf73a\ndiff --git a/Makefile b/Makefile\nindex 2e95353..596c2d8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -66,6 +66,9 @@ all:\n # Define USE_STDEV below if you want git to care about the underlying device\n # change being considered an inode change from the update-cache perspective.\n \n+# Define FIX_MKDIR_ERRNO if your mkdir(2) does not always set errno to EEXIST\n+# if it failed because of existing directory (notably Cygwin).\n+\n GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE\n \t@$(SHELL) ./GIT-VERSION-GEN\n -include GIT-VERSION-FILE\n@@ -249,6 +252,7 @@ ifeq ($(uname_O),Cygwin)\n \t# Try uncommenting this if you see things break -- YMMV.\n \t# NO_MMAP = YesPlease\n \tNO_IPV6 = YesPlease\n+\tFIX_MKDIR_ERRNO = YesPlease\n \tX = .exe\n endif\n ifeq ($(uname_S),OpenBSD)\n@@ -392,6 +396,10 @@ else\n endif\n endif\n endif\n+ifdef FIX_MKDIR_ERRNO\n+\tCOMPAT_CFLAGS += -DFIX_MKDIR_ERRNO\n+\tCOMPAT_OBJS += compat/mkdir.o\n+endif\n \n ALL_CFLAGS += -DSHA1_HEADER=$(call shellquote,$(SHA1_HEADER)) $(COMPAT_CFLAGS)\n LIB_OBJS += $(COMPAT_OBJS)\ndiff --git a/checkout-index.c b/checkout-index.c\nindex 53dd8cb..c5a6d6e 100644\n--- a/checkout-index.c\n+++ b/checkout-index.c\n@@ -199,6 +199,6 @@ int main(int argc, char **argv)\n \tif (0 <= newfd &&\n \t    (write_cache(newfd, active_cache, active_nr) ||\n \t     commit_index_file(&cache_file)))\n-\t\tdie(\"Unable to write new cachefile\");\n+\t\tdie(\"Unable to write new cachefile: %s\", strerror(errno));\n \treturn 0;\n }\ndiff --git a/compat/mkdir.c b/compat/mkdir.c\nnew file mode 100644\nindex 0000000..1c2260c\n--- /dev/null\n+++ b/compat/mkdir.c\n@@ -0,0 +1,23 @@\n+#include <unistd.h>\n+#include <errno.h>\n+#include <sys/stat.h>\n+#include \"../git-compat-util.h\"\n+#undef mkdir\n+\n+int gitmkdir(const char *dir, mode_t mode)\n+{\n+\tint rc = mkdir(dir, mode);\n+\tif ( rc < 0 ) {\n+\t\tint e  = errno;\n+\t\tif ( EACCES == e || EPERM == e )\n+\t\t{\n+\t\t\tstruct stat st;\n+\t\t\tif ( stat(dir, &st) == 0 )\n+\t\t\t\terrno = EEXIST;\n+\t\t\telse\n+\t\t\t\terrno = e;\n+\t\t}\n+\t}\n+\treturn rc;\n+}\n+\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f982b8e..9cf3aab 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -154,4 +154,10 @@ static inline int sane_case(int x, int h\n #ifndef MAXPATHLEN\n #define MAXPATHLEN 256\n #endif\n+\n+#ifdef FIX_MKDIR_ERRNO\n+#define mkdir gitmkdir\n+int gitmkdir(const char *dir, mode_t mode);\n+#endif\n+\n #endif\n-- \n1.1.4.g7397-dirty\n"},{"id":"15278","messageId":"7vvew14frk.fsf@assigned-by-dhcp.cox.net","threadId":"3195","inReplyTo":"20060130193839.GA6575@steel.home","subject":"Re: [PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-01-30T20:33:51Z","receivedAt":"2006-01-30T20:33:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> ... and the directory already exists. I.E. Cygwin is such\n> a case: mkdir fail for mounts which reference directly\n> to windows mounts (\"drives\").\n>\n> ---\n>\n> The discussion, which ended up with this patch can be read\n> here: http://www.cygwin.com/ml/cygwin/2006-01/msg01276.html\n>\n> BTW, there is this:\n> http://www.cygwin.com/ml/cygwin/2006-01/msg01380.html\n> So this patch will probably be not needed soon.\n\nThanks for all the background information.  Although it is very\ntempting for me to adopt your patch as an easy way out, I would\nfeel *dirty* if I did so.\n\nEric Blake is right in his argument in that thread.  Our code\nshould not depend on the Linux EEXIST behaviour.  The reason\nCygwin folks want to be as close to Linux is to work around\napplication bugs like this.  Which is a valid concern for them,\nbut it does not mean the application has license to depend on\nLinux behaviour.\n\nWe, as an application, should take a different attitude -- we\nshould fix things to make things portable, not work things\naround, if both are equally easily doable.  I do not object to\nusing a function that has a semantics Linux mkdir() gives us,\nbut calling that mkdir() does not feel quite right.  \n\nAlso the wrapper implies that in our future use of mkdir() we\ncannot tell the difference between EEXIST and other errors if we\nlater wanted to.\n\nSo let's look at our existing uses first:\n\napply.c:1577:\t\tif (mkdir(buf, 0777) < 0) {\napply.c-1578-\t\t\tif (errno != EEXIST)\napply.c-1579-\t\t\t\tbreak;\napply.c-1580-\t\t}\napply.c-1581-\t}\n\nThis is \"we want to see buf directory exist and we would create\none if there isn't\".  Instead of checking errno, we could stat\nas your patch does.\n\nentry.c:15:\t\tif (mkdir(buf, 0777)) {\nentry.c-16-\t\t\tif (errno == EEXIST) {\nentry.c-17-\t\t\t\tstruct stat st;\nentry.c:18:\t\t\t\tif (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))\nentry.c-19-\t\t\t\t\tcontinue;\nentry.c-20-\t\t\t\tif (!stat(buf, &st) && S_ISDIR(st.st_mode))\nentry.c-21-\t\t\t\t\tcontinue; /* ok */\nentry.c-22-\t\t\t}\n\nThis is a bit more involved. \"we want to have buf directory and\nwe would create it, and if force is given to have something\nunder that directory, we would even unlink the nondirectory that\nsits there\".\n\nI asked \"git-grep\" where else we use mkdir(), and all other\nusers want the semantics of the apply.c use quoted above.\n\nSo I'd rather see us create a generic helper function like this:\n\n\tint make_directory(const char *path, int force)\n        {\n\t\tstruct stat st;\n\t\tint mkdir_errno;\n\t\tif (!mkdir(path, 0777))\n\t\t\treturn 0;\n                mkdir_errno = errno;\n\t\tif (!lstat(path, &st) && S_ISDIR(st.st_mode))\n                \treturn 0;\n                if (!force) {\n\t\tbad:\n                \terrno = mkdir_errno;\n                        return -1;\n                }\n                if (!unlink(path, &st) && !mkdir(path, 0777))\n                \treturn 0;\n                /* we might have failed to unlink an existing symlink\n\t\t * which happens to point at an existing directory; that\n                 * directory is not what we want here.\n                 */\n\t\tif (!lstat(path, &st) && S_ISDIR(st.st_mode))\n                \treturn 0;\n\t\tgoto bad;\n        }\n\nand have current callers of mkdir() use it, regardless of the\nplatform.  It may not worth saving mkdir_errno, though.\n\nThen everybody but entry.c one would say force=0, and entry.c\none passes force appropriately using the condition it uses in\nits current if() statement.\n"},{"id":"15326","messageId":"20060130231622.GB3857@limbo.home","threadId":"3195","inReplyTo":"7vvew14frk.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2006-01-30T23:16:22Z","receivedAt":"2006-01-30T23:16:22Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Mon, Jan 30, 2006 21:33:51 +0100:\n> and have current callers of mkdir() use it, regardless of the\n> platform.  It may not worth saving mkdir_errno, though.\n\nerrno may be worth saving. I find the process of finding\n\"why-the-f$%^-did-the-windows-broke-again\" really tedious:\n1. find the application which failed (grep for the die message)\n2. put \"%s\", strerror(errno) in the die\n3. retest\n4. find out errno is 0 (success)\n5. remove close(fd), munmap, whatever before the die\n6. retest\n7. repeat\n\n> Then everybody but entry.c one would say force=0, and entry.c\n> one passes force appropriately using the condition it uses in\n> its current if() statement.\n\nI like it :)\n"}]}