{"thread":{"id":"32267","subject":"[PATCH] mingw_rmdir: do not prompt for retry when non-empty","startedAt":"2012-12-04T10:41:53Z","lastAt":"2012-12-05T17:00:38Z","messageCount":6,"participants":["Erik Faye-Lund","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"204486","messageId":"1354617713-7436-1-git-send-email-kusmabite@gmail.com","threadId":"32267","inReplyTo":null,"subject":"[PATCH] mingw_rmdir: do not prompt for retry when non-empty","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-12-04T10:41:53Z","receivedAt":"2012-12-04T10:41:53Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"in ab1a11be (\"mingw_rmdir: set errno=ENOTEMPTY when appropriate\"),\na check was added to prevent us from retrying to delete a directory\nthat is both in use and non-empty.\n\nHowever, this logic was slightly flawed; since we didn't return\nimmediately, we end up falling out of the retry-loop, but right into\nthe prompting loop.\n\nFix this by simply returning from the function instead of breaking\nthe loop.\n\nWhile we're at it, change the second break to a return as well; we\nalready know that we won't enter the prompting-loop, beacuse\nis_file_in_use_error(GetLastError()) already evaluated to false.\n\nSigned-off-by: Erik Faye-Lund \n---\n\nHere's a quick patch for a small issue I recently encountered; when\ndeleting a file from inside a directory, we currently end up\nprompting the user if (s)he want us to retry deleting the directory\nthey are in.\n\n compat/mingw.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 1eb974f..2c29667 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -260,10 +260,10 @@ int mingw_rmdir(const char *pathname)\n \n \twhile ((ret = _wrmdir(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n \t\tif (!is_file_in_use_error(GetLastError()))\n-\t\t\tbreak;\n+\t\t\treturn ret;\n \t\tif (!is_dir_empty(wpathname)) {\n \t\t\terrno = ENOTEMPTY;\n-\t\t\tbreak;\n+\t\t\treturn ret;\n \t\t}\n \t\t/*\n \t\t * We assume that some other process had the source or\n-- \n1.8.0.msysgit.0.3.g0262b9f.dirty\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"204493","messageId":"alpine.DEB.1.00.1212041728210.31987@s15462909.onlinehome-server.info","threadId":"32267","inReplyTo":"1354617713-7436-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH] mingw_rmdir: do not prompt for retry when non-empty","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2012-12-04T16:35:05Z","receivedAt":"2012-12-04T16:35:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi kusma,\n\nOn Tue, 4 Dec 2012, Erik Faye-Lund wrote:\n\n> in ab1a11be (\"mingw_rmdir: set errno=ENOTEMPTY when appropriate\"),\n> a check was added to prevent us from retrying to delete a directory\n> that is both in use and non-empty.\n> \n> However, this logic was slightly flawed; since we didn't return\n> immediately, we end up falling out of the retry-loop, but right into\n> the prompting loop.\n> \n> Fix this by simply returning from the function instead of breaking\n> the loop.\n> \n> While we're at it, change the second break to a return as well; we\n> already know that we won't enter the prompting-loop, beacuse\n> is_file_in_use_error(GetLastError()) already evaluated to false.\n\nI usually prefer to break from the loop, to be able to add whatever\ncleanup code we might need in the future after the loop.\n\nSo does this fix the problem for you?\n\n-- snipsnap --\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 04af3dc..504495a 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -259,7 +259,8 @@ int mingw_rmdir(const char *pathname)\n \t\treturn -1;\n \n \twhile ((ret = _wrmdir(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n-\t\tif (!is_file_in_use_error(GetLastError()))\n+\t\terrno = err_win_to_posix(GetLastError());\n+\t\tif (errno != EACCESS)\n \t\t\tbreak;\n \t\tif (!is_dir_empty(wpathname)) {\n \t\t\terrno = ENOTEMPTY;\n@@ -275,7 +276,7 @@ int mingw_rmdir(const char *pathname)\n \t\tSleep(delay[tries]);\n \t\ttries++;\n \t}\n-\twhile (ret == -1 && is_file_in_use_error(GetLastError()) &&\n+\twhile (ret == -1 && errno == EACCESS &&\n \t ask_yes_no_if_possible(\"Deletion of directory '%s' failed. \"\n \t\t\t\"Should I try again?\", pathname))\n \t ret = _wrmdir(wpathname);\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"204525","messageId":"CABPQNSbcSEKApDBWWt7z67DvV6=JwTGebdk6hjgR1OppPyOQwg@mail.gmail.com","threadId":"32267","inReplyTo":"alpine.DEB.1.00.1212041728210.31987@s15462909.onlinehome-server.info","subject":"Re: [PATCH] mingw_rmdir: do not prompt for retry when non-empty","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-12-05T15:18:07Z","receivedAt":"2012-12-05T15:18:07Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Sorry for a late reply.\n\nOn Tue, Dec 4, 2012 at 5:35 PM, Johannes Schindelin\n wrote:\n> Hi kusma,\n>\n> On Tue, 4 Dec 2012, Erik Faye-Lund wrote:\n>\n>> in ab1a11be (\"mingw_rmdir: set errno=ENOTEMPTY when appropriate\"),\n>> a check was added to prevent us from retrying to delete a directory\n>> that is both in use and non-empty.\n>>\n>> However, this logic was slightly flawed; since we didn't return\n>> immediately, we end up falling out of the retry-loop, but right into\n>> the prompting loop.\n>>\n>> Fix this by simply returning from the function instead of breaking\n>> the loop.\n>>\n>> While we're at it, change the second break to a return as well; we\n>> already know that we won't enter the prompting-loop, beacuse\n>> is_file_in_use_error(GetLastError()) already evaluated to false.\n>\n> I usually prefer to break from the loop, to be able to add whatever\n> cleanup code we might need in the future after the loop.\n>\n> So does this fix the problem for you?\n>\n> -- snipsnap --\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 04af3dc..504495a 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -259,7 +259,8 @@ int mingw_rmdir(const char *pathname)\n> return -1;\n>\n> while ((ret = _wrmdir(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n> - if (!is_file_in_use_error(GetLastError()))\n> + errno = err_win_to_posix(GetLastError());\n> + if (errno != EACCESS)\n> break;\n> if (!is_dir_empty(wpathname)) {\n> errno = ENOTEMPTY;\n> @@ -275,7 +276,7 @@ int mingw_rmdir(const char *pathname)\n> Sleep(delay[tries]);\n> tries++;\n> }\n> - while (ret == -1 && is_file_in_use_error(GetLastError()) &&\n> + while (ret == -1 && errno == EACCESS &&\n> ask_yes_no_if_possible(\"Deletion of directory '%s' failed. \"\n> \"Should I try again?\", pathname))\n> ret = _wrmdir(wpathname);\n\nYes, as long as you do s/EACCESS/EACCES/ first. I don't mind such a\nversion instead.\n"},{"id":"204527","messageId":"alpine.DEB.1.00.1212051657131.31987@s15462909.onlinehome-server.info","threadId":"32267","inReplyTo":"CABPQNSbcSEKApDBWWt7z67DvV6=JwTGebdk6hjgR1OppPyOQwg@mail.gmail.com","subject":"Re: [PATCH] mingw_rmdir: do not prompt for retry when non-empty","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2012-12-05T16:02:10Z","receivedAt":"2012-12-05T16:02:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi kusma,\n\nOn Wed, 5 Dec 2012, Erik Faye-Lund wrote:\n\n> Sorry for a late reply.\n\nYeah, sorry, my replies tend to be delayed a lot. For the record: your\nreply was not at all late.\n\n> On Tue, Dec 4, 2012 at 5:35 PM, Johannes Schindelin\n> wrote:\n> >\n> > On Tue, 4 Dec 2012, Erik Faye-Lund wrote:\n> >\n> >> in ab1a11be (\"mingw_rmdir: set errno=ENOTEMPTY when appropriate\"), a\n> >> check was added to prevent us from retrying to delete a directory\n> >> that is both in use and non-empty.\n> >>\n> >> However, this logic was slightly flawed; since we didn't return\n> >> immediately, we end up falling out of the retry-loop, but right into\n> >> the prompting loop.\n> >>\n> >> Fix this by simply returning from the function instead of breaking\n> >> the loop.\n> >>\n> >> While we're at it, change the second break to a return as well; we\n> >> already know that we won't enter the prompting-loop, beacuse\n> >> is_file_in_use_error(GetLastError()) already evaluated to false.\n> >\n> > I usually prefer to break from the loop, to be able to add whatever\n> > cleanup code we might need in the future after the loop.\n> >\n> > So does this fix the problem for you?\n> >\n> > -- snipsnap --\n> > diff --git a/compat/mingw.c b/compat/mingw.c\n> > index 04af3dc..504495a 100644\n> > --- a/compat/mingw.c\n> > +++ b/compat/mingw.c\n> > @@ -259,7 +259,8 @@ int mingw_rmdir(const char *pathname)\n> > return -1;\n> >\n> > while ((ret = _wrmdir(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n> > - if (!is_file_in_use_error(GetLastError()))\n> > + errno = err_win_to_posix(GetLastError());\n> > + if (errno != EACCESS)\n> > break;\n> > if (!is_dir_empty(wpathname)) {\n> > errno = ENOTEMPTY;\n> > @@ -275,7 +276,7 @@ int mingw_rmdir(const char *pathname)\n> > Sleep(delay[tries]);\n> > tries++;\n> > }\n> > - while (ret == -1 && is_file_in_use_error(GetLastError()) &&\n> > + while (ret == -1 && errno == EACCESS &&\n> > ask_yes_no_if_possible(\"Deletion of directory '%s' failed. \"\n> > \"Should I try again?\", pathname))\n> > ret = _wrmdir(wpathname);\n> \n> Yes, as long as you do s/EACCESS/EACCES/ first. I don't mind such a\n> version instead.\n\nAs you probably suspected, I did not have a way to test-compile it before\nsending.\n\nThe reason I was suggesting my version of the patch was to unify the error\nhandling: rather than relying on both errno and GetLastError() (but for\ndifferent error conditions), I would like to rely on only one: errno. That\nway, they cannot contradict each other (as they did in your case).\n\nHowever, I have no strong opinion on this, so please apply the version you\nlike better.\n\nCiao,\nDscho\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"204529","messageId":"CABPQNSbsp_V7o8jQrwjSE_x3z3cAf7T3evK+Xq9kVZF0HrGwpg@mail.gmail.com","threadId":"32267","inReplyTo":"alpine.DEB.1.00.1212051657131.31987@s15462909.onlinehome-server.info","subject":"Re: [PATCH] mingw_rmdir: do not prompt for retry when non-empty","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-12-05T16:23:57Z","receivedAt":"2012-12-05T16:23:57Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Dec 5, 2012 at 5:02 PM, Johannes Schindelin\n wrote:\n> Hi kusma,\n>\n> On Wed, 5 Dec 2012, Erik Faye-Lund wrote:\n>\n>> Sorry for a late reply.\n>\n> Yeah, sorry, my replies tend to be delayed a lot. For the record: your\n> reply was not at all late.\n>\n>> On Tue, Dec 4, 2012 at 5:35 PM, Johannes Schindelin\n>> wrote:\n>> >\n>> > On Tue, 4 Dec 2012, Erik Faye-Lund wrote:\n>> >\n>> >> in ab1a11be (\"mingw_rmdir: set errno=ENOTEMPTY when appropriate\"), a\n>> >> check was added to prevent us from retrying to delete a directory\n>> >> that is both in use and non-empty.\n>> >>\n>> >> However, this logic was slightly flawed; since we didn't return\n>> >> immediately, we end up falling out of the retry-loop, but right into\n>> >> the prompting loop.\n>> >>\n>> >> Fix this by simply returning from the function instead of breaking\n>> >> the loop.\n>> >>\n>> >> While we're at it, change the second break to a return as well; we\n>> >> already know that we won't enter the prompting-loop, beacuse\n>> >> is_file_in_use_error(GetLastError()) already evaluated to false.\n>> >\n>> > I usually prefer to break from the loop, to be able to add whatever\n>> > cleanup code we might need in the future after the loop.\n>> >\n>> > So does this fix the problem for you?\n>> >\n>> > -- snipsnap --\n>> > diff --git a/compat/mingw.c b/compat/mingw.c\n>> > index 04af3dc..504495a 100644\n>> > --- a/compat/mingw.c\n>> > +++ b/compat/mingw.c\n>> > @@ -259,7 +259,8 @@ int mingw_rmdir(const char *pathname)\n>> > return -1;\n>> >\n>> > while ((ret = _wrmdir(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n>> > - if (!is_file_in_use_error(GetLastError()))\n>> > + errno = err_win_to_posix(GetLastError());\n>> > + if (errno != EACCESS)\n>> > break;\n>> > if (!is_dir_empty(wpathname)) {\n>> > errno = ENOTEMPTY;\n>> > @@ -275,7 +276,7 @@ int mingw_rmdir(const char *pathname)\n>> > Sleep(delay[tries]);\n>> > tries++;\n>> > }\n>> > - while (ret == -1 && is_file_in_use_error(GetLastError()) &&\n>> > + while (ret == -1 && errno == EACCESS &&\n>> > ask_yes_no_if_possible(\"Deletion of directory '%s' failed. \"\n>> > \"Should I try again?\", pathname))\n>> > ret = _wrmdir(wpathname);\n>>\n>> Yes, as long as you do s/EACCESS/EACCES/ first. I don't mind such a\n>> version instead.\n>\n> As you probably suspected, I did not have a way to test-compile it before\n> sending.\n>\n> The reason I was suggesting my version of the patch was to unify the error\n> handling: rather than relying on both errno and GetLastError() (but for\n> different error conditions), I would like to rely on only one: errno. That\n> way, they cannot contradict each other (as they did in your case).\n>\n\nSince we're justifying the approaches, I'd like to explain why I\npreferred the return approach: it performs less tests. While this\nmight sound like premature optimizations, performance is not why I\nthink it's a good idea. It makes the fix easier to verify; you don't\nneed to validate that the conditions of the second loop won't happen,\nbecause the code exits quickly.\n\nIf we added something that required cleanup, we could change the\nreturn to a goto with a cleanup-label, and it would still be\nrelatively easy to see what's going on.\n\n> However, I have no strong opinion on this, so please apply the version you\n> like better.\n\nSince the issue is present in mainline Git as well, I'd prefer if\nJunio merged whatever he prefers. I can produce a proper patch out of\nyour suggesting, if needed.\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"204532","messageId":"7v624gl9hl.fsf@alter.siamese.dyndns.org","threadId":"32267","inReplyTo":"CABPQNSbsp_V7o8jQrwjSE_x3z3cAf7T3evK+Xq9kVZF0HrGwpg@mail.gmail.com","subject":"Re: [PATCH] mingw_rmdir: do not prompt for retry when non-empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-05T17:00:38Z","receivedAt":"2012-12-05T17:00:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> On Wed, Dec 5, 2012 at 5:02 PM, Johannes Schindelin\n> wrote:\n> ...\n> Since we're justifying the approaches, I'd like to explain why I\n> preferred the return approach: it performs less tests. While this\n> might sound like premature optimizations, performance is not why I\n> think it's a good idea. It makes the fix easier to verify; you don't\n> need to validate that the conditions of the second loop won't happen,\n> because the code exits quickly.\n>\n> If we added something that required cleanup, we could change the\n> return to a goto with a cleanup-label, and it would still be\n> relatively easy to see what's going on.\n>\n>> However, I have no strong opinion on this, so please apply the version you\n>> like better.\n>\n> Since the issue is present in mainline Git as well, I'd prefer if\n> Junio merged whatever he prefers. I can produce a proper patch out of\n> your suggesting, if needed.\n\nThanks; what you and Dscho agreed in this discussion sounds good to\nme, too.\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"}]}