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

> 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.