{"thread":{"id":"19749","subject":"[PATCH] Expand the rename(2) workaround in mingw to cover case change in filename","startedAt":"2009-06-08T20:32:48Z","lastAt":"2009-06-09T07:27:34Z","messageCount":7,"participants":["Alex Riesen","Johannes Schindelin","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"115832","messageId":"20090608203248.GA22972@blimp.localdomain","threadId":"19749","inReplyTo":null,"subject":"[PATCH] Expand the rename(2) workaround in mingw to cover case change in filename","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-06-08T20:32:48Z","receivedAt":"2009-06-08T20:32:48Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Windows, being a case-confused operating system, sometimes has\nproblems renaming directory entries with only change in the case\nof their characters. Try to work the problem around by using an\nintermediate file.\n\n---\n\nNOT TESTED. Can't. My Windows broke again. Not even compile-tested.\n\nAnd sorry for somewhat extended Cc:. Thought the problem need a little\nmore experienced attention (then again, maybe it doesn't).\n\nThat's a response to Bug 228 (which is properly \"WONTFIX\",\nif you ask my opinion):\n\n    http://code.google.com/p/msysgit/issues/detail?id=228\n\nI don't like it, but it is the same what people usually forced to do\nanyway, so here it is.\n\nIt has a racing problem which can cause the original file be renamed\ninto a temporary name (as returned by mkstemp):\n\n    - the rename of the source name to temp name succeeds\n    - the rename of temp name to destination name fails (i.e. it\n      exists and is open)\n\nIt is not fixable (because the atomicity of rename(2) is broken by the\nintermediate name and two separate calls to rename involved).\nI could have tried to rename back to source, but got fed up with\nanother Windows stupidity and gave up.\n\n compat/mingw.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++--------\n 1 files changed, 55 insertions(+), 10 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex e190fdd..ad60dff 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -931,21 +931,12 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)\n \treturn connect(s, sa, sz);\n }\n \n-#undef rename\n-int mingw_rename(const char *pold, const char *pnew)\n+static int move_file_replace(const char *pold, const char *pnew)\n {\n \tDWORD attrs, gle;\n \tint tries = 0;\n \tstatic const int delay[] = { 0, 1, 10, 20, 40 };\n \n-\t/*\n-\t * Try native rename() first to get errno right.\n-\t * It is based on MoveFile(), which cannot overwrite existing files.\n-\t */\n-\tif (!rename(pold, pnew))\n-\t\treturn 0;\n-\tif (errno != EEXIST)\n-\t\treturn -1;\n repeat:\n \tif (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))\n \t\treturn 0;\n@@ -982,6 +973,60 @@ repeat:\n \treturn -1;\n }\n \n+#undef rename\n+int mingw_rename(const char *pold, const char *pnew)\n+{\n+\tint fd, rc = -1;\n+\tchar *p, *tmp = NULL;\n+\n+\t/*\n+\t * Try native rename() first to get errno right.\n+\t * It is based on MoveFile(), which cannot overwrite existing files.\n+\t */\n+\tif (!rename(pold, pnew))\n+\t\treturn 0;\n+\tif (errno != EEXIST)\n+\t\treturn -1;\n+\t/*\n+\t * Windows' case-insensitivity does not allow it to directly\n+\t * do a rename where the only change in the file name is\n+\t * the change of a letter case. Work this around with a\n+\t * temporary file.\n+\t */\n+\tif (!dst) {\n+\t\terrno = EINVAL;\n+\t\tgoto fail;\n+\t}\n+\ttmp = malloc(strlen(dst) + 7 /* reserved for template */);\n+\tif (!tmp) {\n+\t\terrno = ENOMEM;\n+\t\tgoto fail;\n+\t}\n+\tstrcpy(tmp, dst);\n+\tp = tmp + strlen(dst);\n+\tfor (p = tmp + strlen(dst); p > tmp; --p)\n+\t\tif ('\\\\' == *p || '/' == *p) {\n+\t\t\t++p;\n+\t\t\tbreak;\n+\t\t}\n+\tstrcpy(p, \"XXXXXX\")\n+\tfd = mkstemp(tmp);\n+\tif (fd < 0)\n+\t\tgoto fail;\n+\tclose(fd);\n+\tif (move_file_replace(src, tmp)) {\n+\t\trc = errno;\n+\t\tunlink(tmp);\n+\t\terrno = rc;\n+\t\trc = -1;\n+\t\tgoto fail;\n+\t}\n+\trc = move_file_replace(tmp, dst);\n+fail:\n+\tfree(tmp);\n+\treturn rc;\n+}\n+\n struct passwd *getpwuid(int uid)\n {\n \tstatic char user_name[100];\n-- \n1.6.3.2.214.g82a9d\n"},{"id":"115850","messageId":"alpine.DEB.1.00.0906082355350.26154@pacific.mpi-cbg.de","threadId":"19749","inReplyTo":"20090608203248.GA22972@blimp.localdomain","subject":"Re: [PATCH] Expand the rename(2) workaround in mingw to cover case change in filename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-06-08T21:59:18Z","receivedAt":"2009-06-08T21:59:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 8 Jun 2009, Alex Riesen wrote:\n\n> Windows, being a case-confused operating system, sometimes has\n> problems renaming directory entries with only change in the case\n> of their characters. Try to work the problem around by using an\n> intermediate file.\n> \n> ---\n> \n> NOT TESTED. Can't. My Windows broke again. Not even compile-tested.\n\nYes, that is pretty easy to see as you first used pold/pnew and then \nsrc/dst.\n\nI minimally fixed up your patch (it now uses strbuf), and added a \ntest-case.\n\nThis test-case is actually crucial: it shows that your patch is not \nenough: the culprit is this code in builtin-mv.c:167--168:\n\n                else if (lstat(dst, &st) == 0) {\n                        bad = \"destination exists\";\n\nCiao,\nDscho\n"},{"id":"115851","messageId":"alpine.DEB.1.00.0906082359320.26154@pacific.mpi-cbg.de","threadId":"19749","inReplyTo":"alpine.DEB.1.00.0906082355350.26154@pacific.mpi-cbg.de","subject":"Re: [PATCH] Expand the rename(2) workaround in mingw to cover case change in filename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-06-08T22:00:51Z","receivedAt":"2009-06-08T22:00:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 8 Jun 2009, Johannes Schindelin wrote:\n\n> On Mon, 8 Jun 2009, Alex Riesen wrote:\n> \n> > Windows, being a case-confused operating system, sometimes has\n> > problems renaming directory entries with only change in the case\n> > of their characters. Try to work the problem around by using an\n> > intermediate file.\n> > \n> > ---\n> > \n> > NOT TESTED. Can't. My Windows broke again. Not even compile-tested.\n> \n> Yes, that is pretty easy to see as you first used pold/pnew and then \n> src/dst.\n> \n> I minimally fixed up your patch (it now uses strbuf), and added a \n> test-case.\n\nI probably should have mentioned that you can see it here:\n\nhttp://repo.or.cz/w/git/mingw/4msysgit.git?a=shortlog;h=refs/heads/work/rename\n\nCiao,\nDscho\n"},{"id":"115854","messageId":"81b0412b0906081508v5435c59cm3faf3ac92a56578c@mail.gmail.com","threadId":"19749","inReplyTo":"alpine.DEB.1.00.0906082355350.26154@pacific.mpi-cbg.de","subject":"Re: [PATCH] Expand the rename(2) workaround in mingw to cover case change in filename","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-06-08T22:08:09Z","receivedAt":"2009-06-08T22:08:09Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/6/8 Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n> On Mon, 8 Jun 2009, Alex Riesen wrote:\n>> NOT TESTED. Can't. My Windows broke again. Not even compile-tested.\n>\n> Yes, that is pretty easy to see as you first used pold/pnew and then\n> src/dst.\n\nYep.\n\n> I minimally fixed up your patch (it now uses strbuf), and added a\n> test-case.\n>\n> This test-case is actually crucial: it shows that your patch is not\n> enough: the culprit is this code in builtin-mv.c:167--168:\n>\n>                else if (lstat(dst, &st) == 0) {\n>                        bad = \"destination exists\";\n\nAh, thanks. Missed that completely.\n"},{"id":"115871","messageId":"4A2DF92D.1080605@viscovery.net","threadId":"19749","inReplyTo":"81b0412b0906081508v5435c59cm3faf3ac92a56578c@mail.gmail.com","subject":"Re: [PATCH] Expand the rename(2) workaround in mingw to cover case change in filename","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-06-09T05:54:53Z","receivedAt":"2009-06-09T05:54:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> 2009/6/8 Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n>> On Mon, 8 Jun 2009, Alex Riesen wrote:\n>>> NOT TESTED. Can't. My Windows broke again. Not even compile-tested.\n>> Yes, that is pretty easy to see as you first used pold/pnew and then\n>> src/dst.\n> \n> Yep.\n> \n>> I minimally fixed up your patch (it now uses strbuf), and added a\n>> test-case.\n>>\n>> This test-case is actually crucial: it shows that your patch is not\n>> enough: the culprit is this code in builtin-mv.c:167--168:\n>>\n>>                else if (lstat(dst, &st) == 0) {\n>>                        bad = \"destination exists\";\n> \n> Ah, thanks. Missed that completely.\n\nThat's the reason why I think any work-around for this problem is not\nworth it.\n\nIf you want to be cross-platfrom, make up your mind about file names in\nadvance.\n\n-- Hannes\n"},{"id":"115872","messageId":"81b0412b0906082304x750eb8cif3356458829de3bc@mail.gmail.com","threadId":"19749","inReplyTo":"4A2DF92D.1080605@viscovery.net","subject":"Re: [PATCH] Expand the rename(2) workaround in mingw to cover case change in filename","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-06-09T06:04:25Z","receivedAt":"2009-06-09T06:04:25Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/6/9 Johannes Sixt <j.sixt@viscovery.net>:\n> Alex Riesen schrieb:\n>> 2009/6/8 Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n>>> On Mon, 8 Jun 2009, Alex Riesen wrote:\n>>>> NOT TESTED. Can't. My Windows broke again. Not even compile-tested.\n>>> Yes, that is pretty easy to see as you first used pold/pnew and then\n>>> src/dst.\n>>\n>> Yep.\n>>\n>>> I minimally fixed up your patch (it now uses strbuf), and added a\n>>> test-case.\n>>>\n>>> This test-case is actually crucial: it shows that your patch is not\n>>> enough: the culprit is this code in builtin-mv.c:167--168:\n>>>\n>>>                else if (lstat(dst, &st) == 0) {\n>>>                        bad = \"destination exists\";\n>>\n>> Ah, thanks. Missed that completely.\n>\n> That's the reason why I think any work-around for this problem is not\n> worth it.\n\nWhat is the lstat is for, anyway?\n\n> If you want to be cross-platfrom, make up your mind about file names in\n> advance.\n\nI suspect it is the other way around. Some platforms just hate the idea\nof being ported to. Or its designers have poor imagination and never\nthough about the fact that other platforms exist.\n"},{"id":"115880","messageId":"alpine.DEB.1.00.0906090924170.26154@pacific.mpi-cbg.de","threadId":"19749","inReplyTo":"81b0412b0906082304x750eb8cif3356458829de3bc@mail.gmail.com","subject":"Re: [PATCH] Expand the rename(2) workaround in mingw to cover case change in filename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-06-09T07:27:34Z","receivedAt":"2009-06-09T07:27:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 9 Jun 2009, Alex Riesen wrote:\n\n> 2009/6/9 Johannes Sixt <j.sixt@viscovery.net>:\n> > Alex Riesen schrieb:\n> >> 2009/6/8 Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n> >>> On Mon, 8 Jun 2009, Alex Riesen wrote:\n> >>>> NOT TESTED. Can't. My Windows broke again. Not even compile-tested.\n> >>> Yes, that is pretty easy to see as you first used pold/pnew and then\n> >>> src/dst.\n> >>\n> >> Yep.\n> >>\n> >>> I minimally fixed up your patch (it now uses strbuf), and added a\n> >>> test-case.\n> >>>\n> >>> This test-case is actually crucial: it shows that your patch is not\n> >>> enough: the culprit is this code in builtin-mv.c:167--168:\n> >>>\n> >>>                else if (lstat(dst, &st) == 0) {\n> >>>                        bad = \"destination exists\";\n> >>\n> >> Ah, thanks. Missed that completely.\n> >\n> > That's the reason why I think any work-around for this problem is not\n> > worth it.\n> \n> What is the lstat is for, anyway?\n\nIt is to avoid overwriting existing files.\n\n> > If you want to be cross-platfrom, make up your mind about file names \n> > in advance.\n> \n> I suspect it is the other way around. Some platforms just hate the idea \n> of being ported to. Or its designers have poor imagination and never \n> though about the fact that other platforms exist.\n\nActually, think about it as a brilliant business plan: make developers be \nstuck with developing for your platform, then they lack the time to take \ncare of other platforms.\n\nThe bigger question with the patch is if the lstat() call can tell us \nif this is the same file (really the same, not just a hardlink (= same \ncontents)).\n\nCiao,\nDscho\n"}]}