{"thread":{"id":"7732","subject":"[PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory","startedAt":"2007-04-18T22:33:27Z","lastAt":"2007-04-19T18:31:06Z","messageCount":6,"participants":["Alex Riesen","Linus Torvalds","Sam Ravnborg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"39838","messageId":"20070418223327.GC2477@steel.home","threadId":"7732","inReplyTo":null,"subject":"[PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-18T22:33:27Z","receivedAt":"2007-04-18T22:33:27Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Looks like this time it is not cygwin, the you-know-what actually does\nreturn a permission error.\n\nSigned-off-by: Alex Riesen \n---\n\nI am very tempted to conditionally #define gitunlink\nto say something rude about win32 and delete c:\\boot.ini\ninstead. It will even work, in most setups.\n\n merge-recursive.c | 9 ++++++---\n 1 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 595b022..ae4032b 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -610,16 +610,19 @@ static void update_file_flags(const unsigned char *sha,\n \t\t\t\tdie(msg, path, \"\");\n \t\t\t}\n \t\t\tif (unlink(path)) {\n-\t\t\t\tif (errno == EISDIR) {\n+\t\t\t\tstruct stat st;\n+\t\t\t\tint err = errno;\n+\t\t\t\tif (err == EISDIR ||\n+\t\t\t\t (err == EPERM && !lstat(path, &st) && S_ISDIR(st.st_mode))) {\n \t\t\t\t\t/* something else exists */\n \t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n \t\t\t\t\tupdate_wd = 0;\n \t\t\t\t\tgoto update_index;\n \t\t\t\t}\n-\t\t\t\tif (errno != ENOENT)\n+\t\t\t\tif (err != ENOENT)\n \t\t\t\t\tdie(\"failed to unlink %s \"\n \t\t\t\t\t \"in preparation to update: %s\",\n-\t\t\t\t\t path, strerror(errno));\n+\t\t\t\t\t path, strerror(err));\n \t\t\t}\n \t\t\tif (mode & 0100)\n \t\t\t\tmode = 0777;\n-- \n1.5.1.1.876.ge36f76\n"},{"id":"39840","messageId":"alpine.LFD.0.98.0704181537590.9964@woody.linux-foundation.org","threadId":"7732","inReplyTo":"20070418223327.GC2477@steel.home","subject":"Re: [PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-18T23:04:06Z","receivedAt":"2007-04-18T23:04:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 19 Apr 2007, Alex Riesen wrote:\n>\n> +\t\t\t\tstruct stat st;\n> +\t\t\t\tint err = errno;\n> +\t\t\t\tif (err == EISDIR ||\n> +\t\t\t\t (err == EPERM && !lstat(path, &st) && S_ISDIR(st.st_mode))) {\n\nCan I ask people to please *not* write things like this?\n\nHere's an important rule from Linus:\n\n\tRule#1 when fixing bugs: there's a reason for the bug. And \n\t_usually_ the reason was that the source code was hard to think \n\tabout or read.\n\n\tSo you should always make the source code more readable when you \n\tfix a bug. If you don't, the bug will just reappear or is just \n\tmore subtly hidden! A bugfix that just makes the same code even\n\t*harder* to read is not a fix at all!\n\n(Side note: EPERM is actually apparently the POSIXLY correct error!)\n\nComlex conditionals are really just asking for bugs - either because they \nare buggy themselves, or because people don't understand what they really \ntest for, and introduce bugs later.\n\nThat whole sequence should probably be a function of its own. But I'd also \nlike to note that for some strange and inexplicable reason, the regular \nfile handling and the symlink handling uses totally different setup, and \nthe symlink handling does *not* do any of the error checks that the file \ncase does.\n\nSo that function should be *common* to the two cases, and do the mkdir_p \n_and_ the unlink. As it is, I suspect we have some test-case for regular \nfiles that caused us to be careful with them, but we lack a test-case for \nsymlinks, so we never bothered to make the code work either!\n\nSo here's a suggested and totally untested patch. It makes the code more \nreadable, and probably fixes *two* bugs in the process. It also simply \ndoesn't really even care what the error actually was - the important part \nwas not that it was a directory, but that the unlink didn't succeed!\n\nBut even if somebody wants to re-introduce more errno value testing, I \nthink you should apply this patch *first*, because source code readability \nreally is very important!\n\nFunctions should be small and not have indentation of more than two \nlevels.\n\n[ And yes, I realize I don't always follow my own rules, but I try. I \n often don't write a hell of a lot of comments when I write the first \n version of the code - I tend to add them later when some piece of code \n has been shown to need them - but I try very hard to make functions \n small and simple and do *one* thing, and not have lots of levels of \n indentation. And if you see me write bad code, please flame me too! ]\n\n\t\tLinus\n\n---\n merge-recursive.c | 54 ++++++++++++++++++++++++++++------------------------\n 1 files changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 595b022..cea6c87 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -574,6 +574,31 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)\n \t}\n }\n \n+static int make_room_for_path(const char *path)\n+{\n+\tint status;\n+\tconst char *msg = \"failed to create path '%s'%s\";\n+\n+\tstatus = mkdir_p(path, 0777);\n+\tif (status) {\n+\t\tif (status == -3) {\n+\t\t\t/* something else exists */\n+\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n+\t\t\treturn -1;\n+\t\t}\n+\t\tdie(msg, path, \"\");\n+\t}\n+\n+\t/* Successful unlink is good.. */\n+\tif (!unlink(path))\n+\t\treturn 0;\n+\t/* .. and so is no existing file */\n+\tif (errno == ENOENT)\n+\t\treturn 0;\n+\t/* .. but not some other error (who really cares what?) */\n+\treturn error(msg, path, \": perhaps a D/F conflict?\");\n+}\n+\n static void update_file_flags(const unsigned char *sha,\n \t\t\t unsigned mode,\n \t\t\t const char *path,\n@@ -594,33 +619,12 @@ static void update_file_flags(const unsigned char *sha,\n \t\tif (type != OBJ_BLOB)\n \t\t\tdie(\"blob expected for %s '%s'\", sha1_to_hex(sha), path);\n \n+\t\tif (make_room_for_path(path) < 0) {\n+\t\t\tupdate_wd = 0;\n+\t\t\tgoto update_index;\n+\t\t}\n \t\tif (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {\n \t\t\tint fd;\n-\t\t\tint status;\n-\t\t\tconst char *msg = \"failed to create path '%s'%s\";\n-\n-\t\t\tstatus = mkdir_p(path, 0777);\n-\t\t\tif (status) {\n-\t\t\t\tif (status == -3) {\n-\t\t\t\t\t/* something else exists */\n-\t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n-\t\t\t\t\tupdate_wd = 0;\n-\t\t\t\t\tgoto update_index;\n-\t\t\t\t}\n-\t\t\t\tdie(msg, path, \"\");\n-\t\t\t}\n-\t\t\tif (unlink(path)) {\n-\t\t\t\tif (errno == EISDIR) {\n-\t\t\t\t\t/* something else exists */\n-\t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n-\t\t\t\t\tupdate_wd = 0;\n-\t\t\t\t\tgoto update_index;\n-\t\t\t\t}\n-\t\t\t\tif (errno != ENOENT)\n-\t\t\t\t\tdie(\"failed to unlink %s \"\n-\t\t\t\t\t \"in preparation to update: %s\",\n-\t\t\t\t\t path, strerror(errno));\n-\t\t\t}\n \t\t\tif (mode & 0100)\n \t\t\t\tmode = 0777;\n \t\t\telse\n"},{"id":"39841","messageId":"20070418234034.GE2477@steel.home","threadId":"7732","inReplyTo":"alpine.LFD.0.98.0704181537590.9964@woody.linux-foundation.org","subject":"Re: [PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-18T23:40:34Z","receivedAt":"2007-04-18T23:40:34Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Linus Torvalds, Thu, Apr 19, 2007 01:04:06 +0200:\n> >\n> > +\t\t\t\tstruct stat st;\n> > +\t\t\t\tint err = errno;\n> > +\t\t\t\tif (err == EISDIR ||\n> > +\t\t\t\t (err == EPERM && !lstat(path, &st) && S_ISDIR(st.st_mode))) {\n> \n> Can I ask people to please *not* write things like this?\n> \n\nErr... ok.\n\n> \n> (Side note: EPERM is actually apparently the POSIXLY correct error!)\n> \n\nIndeed it is 8-[]\n\n> \n> So here's a suggested and totally untested patch. It makes the code more \n> readable, and probably fixes *two* bugs in the process. It also simply \n> doesn't really even care what the error actually was - the important part \n> was not that it was a directory, but that the unlink didn't succeed!\n>\n\nWell, it is a bit tested now. I'll repeat the testing tomorrow on that\nwindows box.\n\n> +\t/* .. but not some other error (who really cares what?) */\n> +\treturn error(msg, path, \": perhaps a D/F conflict?\");\n\nI have to care sometimes when cygwin breaks where you never expect it\nto. These annoying strerror(errno)'s a very helpful. IOW, how can\nthe user respond to the message which just tells \"maybe it is\nexpected and you can fix it. Perhaps\"? What do I do here next?\n(well, I know what to do, but someone wont).\n\nAn lstat + S_ISDIR would at least make it plain \"D/F conflict\".\n"},{"id":"39871","messageId":"81b0412b0704190128o7fccbe77h8df3114328d6a0da@mail.gmail.com","threadId":"7732","inReplyTo":"20070418234034.GE2477@steel.home","subject":"Re: [PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-19T08:28:44Z","receivedAt":"2007-04-19T08:28:44Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/19/07, Alex Riesen wrote:\n> >\n> > So here's a suggested and totally untested patch. It makes the code more\n> > readable, and probably fixes *two* bugs in the process. It also simply\n> > doesn't really even care what the error actually was - the important part\n> > was not that it was a directory, but that the unlink didn't succeed!\n> >\n>\n> Well, it is a bit tested now. I'll repeat the testing tomorrow on that\n> windows box.\n>\n\nTested on windows too. Works.\n"},{"id":"39905","messageId":"alpine.LFD.0.98.0704190932450.9964@woody.linux-foundation.org","threadId":"7732","inReplyTo":"81b0412b0704190128o7fccbe77h8df3114328d6a0da@mail.gmail.com","subject":"Re: [PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-19T16:39:24Z","receivedAt":"2007-04-19T16:39:24Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 19 Apr 2007, Alex Riesen wrote:\n\n> On 4/19/07, Alex Riesen wrote:\n> > >\n> > > So here's a suggested and totally untested patch. It makes the code more\n> > > readable, and probably fixes *two* bugs in the process. It also simply\n> > > doesn't really even care what the error actually was - the important part\n> > > was not that it was a directory, but that the unlink didn't succeed!\n> > >\n> > \n> > Well, it is a bit tested now. I'll repeat the testing tomorrow on that\n> > windows box.\n> \n> Tested on windows too. Works.\n\nGood. Junio, I'd really suggest applying it. The old code was literally \nwrong, and depended on an error return that seems to be Linux-specific. It \nwas also pretty ugly.\n\nHere's the patch again with proper sign-off and a commentary..\n\nAs mentioned, maybe this wants expanding in the future, but regardless, \nthe patch not only fixes a git problem on windows (and quite possibly \nother unixes too), any extensions will be much easier on top of this.\n\n\t\tLinus\n\n---\nFrom: Linus Torvalds \nSubject: Fix working directory errno handling when unlinking a directory\n\nAlex Riesen noticed that the case where a file replaced a directory entry \nin the working tree was broken on cygwin. It turns out that the code made \nsome Linux-specific assumptions, and also ignored errors entirely for the \ncase where the entry was a symlink rather than a file.\n\nThis cleans it up by separating out the common case into a function of its \nown, so that both regular files and symlinks can share it, and by making \nthe error handling more obvious (and not depend on any Linux-specific \nbehaviour).\n\nAcked-by: Alex Riesen \nSigned-off-by: Linus Torvalds \n\n---\n merge-recursive.c | 54 ++++++++++++++++++++++++++++------------------------\n 1 files changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 595b022..cea6c87 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -574,6 +574,31 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)\n \t}\n }\n \n+static int make_room_for_path(const char *path)\n+{\n+\tint status;\n+\tconst char *msg = \"failed to create path '%s'%s\";\n+\n+\tstatus = mkdir_p(path, 0777);\n+\tif (status) {\n+\t\tif (status == -3) {\n+\t\t\t/* something else exists */\n+\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n+\t\t\treturn -1;\n+\t\t}\n+\t\tdie(msg, path, \"\");\n+\t}\n+\n+\t/* Successful unlink is good.. */\n+\tif (!unlink(path))\n+\t\treturn 0;\n+\t/* .. and so is no existing file */\n+\tif (errno == ENOENT)\n+\t\treturn 0;\n+\t/* .. but not some other error (who really cares what?) */\n+\treturn error(msg, path, \": perhaps a D/F conflict?\");\n+}\n+\n static void update_file_flags(const unsigned char *sha,\n \t\t\t unsigned mode,\n \t\t\t const char *path,\n@@ -594,33 +619,12 @@ static void update_file_flags(const unsigned char *sha,\n \t\tif (type != OBJ_BLOB)\n \t\t\tdie(\"blob expected for %s '%s'\", sha1_to_hex(sha), path);\n \n+\t\tif (make_room_for_path(path) < 0) {\n+\t\t\tupdate_wd = 0;\n+\t\t\tgoto update_index;\n+\t\t}\n \t\tif (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {\n \t\t\tint fd;\n-\t\t\tint status;\n-\t\t\tconst char *msg = \"failed to create path '%s'%s\";\n-\n-\t\t\tstatus = mkdir_p(path, 0777);\n-\t\t\tif (status) {\n-\t\t\t\tif (status == -3) {\n-\t\t\t\t\t/* something else exists */\n-\t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n-\t\t\t\t\tupdate_wd = 0;\n-\t\t\t\t\tgoto update_index;\n-\t\t\t\t}\n-\t\t\t\tdie(msg, path, \"\");\n-\t\t\t}\n-\t\t\tif (unlink(path)) {\n-\t\t\t\tif (errno == EISDIR) {\n-\t\t\t\t\t/* something else exists */\n-\t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n-\t\t\t\t\tupdate_wd = 0;\n-\t\t\t\t\tgoto update_index;\n-\t\t\t\t}\n-\t\t\t\tif (errno != ENOENT)\n-\t\t\t\t\tdie(\"failed to unlink %s \"\n-\t\t\t\t\t \"in preparation to update: %s\",\n-\t\t\t\t\t path, strerror(errno));\n-\t\t\t}\n \t\t\tif (mode & 0100)\n \t\t\t\tmode = 0777;\n \t\t\telse\n"},{"id":"39911","messageId":"20070419183106.GB3396@uranus.ravnborg.org","threadId":"7732","inReplyTo":"alpine.LFD.0.98.0704190932450.9964@woody.linux-foundation.org","subject":"Re: [PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory","fromName":"Sam Ravnborg","fromEmail":"sam@ravnborg.org","sentAt":"2007-04-19T18:31:06Z","receivedAt":"2007-04-19T18:31:06Z","isPatch":true,"sender":{"key":"sam@ravnborg.org","avatar":"https://gravatar.com/avatar/168a912606ed0742d840bb365e3cc21db390c36531a58341dc7a069cc1f15f62?d=mp&s=160"},"body":"> +\t\tif (status == -3) {\n> +\t\t\t/* something else exists */\n> +\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n\nIt would be more helpful it you spelled it out like:\nDirectory/File conflict?\n\n\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tdie(msg, path, \"\");\n> +\t}\n> +\n> +\t/* Successful unlink is good.. */\n> +\tif (!unlink(path))\n> +\t\treturn 0;\n> +\t/* .. and so is no existing file */\n> +\tif (errno == ENOENT)\n> +\t\treturn 0;\n> +\t/* .. but not some other error (who really cares what?) */\n> +\treturn error(msg, path, \": perhaps a D/F conflict?\");\nditto.\n\nI know this was how it looked previously too but thats just a lame excuse.\n\n\tSam\n"}]}