Re: [PATCH] mingw_rmdir: do not prompt for retry when non-empty

> Hi kusma,
>
> On Wed, 5 Dec 2012, Erik Faye-Lund wrote:
>
>> Sorry for a late reply.
>
> Yeah, sorry, my replies tend to be delayed a lot. For the record: your
> reply was not at all late.
>
>> On Tue, Dec 4, 2012 at 5:35 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> >
>> > On Tue, 4 Dec 2012, Erik Faye-Lund wrote:
>> >
>> >> in ab1a11be ("mingw_rmdir: set errno=ENOTEMPTY when appropriate"), a
>> >> check was added to prevent us from retrying to delete a directory
>> >> that is both in use and non-empty.
>> >>
>> >> However, this logic was slightly flawed; since we didn't return
>> >> immediately, we end up falling out of the retry-loop, but right into
>> >> the prompting loop.
>> >>
>> >> Fix this by simply returning from the function instead of breaking
>> >> the loop.
>> >>
>> >> While we're at it, change the second break to a return as well; we
>> >> already know that we won't enter the prompting-loop, beacuse
>> >> is_file_in_use_error(GetLastError()) already evaluated to false.
>> >
>> > I usually prefer to break from the loop, to be able to add whatever
>> > cleanup code we might need in the future after the loop.
>> >
>> > So does this fix the problem for you?
>> >
>> > -- snipsnap --
>> > diff --git a/compat/mingw.c b/compat/mingw.c
>> > index 04af3dc..504495a 100644
>> > --- a/compat/mingw.c
>> > +++ b/compat/mingw.c
>> > @@ -259,7 +259,8 @@ int mingw_rmdir(const char *pathname)
>> >                 return -1;
>> >
>> >         while ((ret = _wrmdir(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {
>> > -               if (!is_file_in_use_error(GetLastError()))
>> > +               errno = err_win_to_posix(GetLastError());
>> > +               if (errno != EACCESS)
>> >                         break;
>> >                 if (!is_dir_empty(wpathname)) {
>> >                         errno = ENOTEMPTY;
>> > @@ -275,7 +276,7 @@ int mingw_rmdir(const char *pathname)
>> >                 Sleep(delay[tries]);
>> >                 tries++;
>> >         }
>> > -       while (ret == -1 && is_file_in_use_error(GetLastError()) &&
>> > +       while (ret == -1 && errno == EACCESS &&
>> >                ask_yes_no_if_possible("Deletion of directory '%s' failed. "
>> >                         "Should I try again?", pathname))
>> >                ret = _wrmdir(wpathname);
>>
>> Yes, as long as you do s/EACCESS/EACCES/ first. I don't mind such a
>> version instead.
>
> As you probably suspected, I did not have a way to test-compile it before
> sending.
>
> The reason I was suggesting my version of the patch was to unify the error
> handling: rather than relying on both errno and GetLastError() (but for
> different error conditions), I would like to rely on only one: errno. That
> way, they cannot contradict each other (as they did in your case).
>