patchmingw_rmdir: do not prompt for retry when non-empty

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

> Hi kusma,
>
> 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);

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

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

> On Wed, Dec 5, 2012 at 5:02 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> ...
> Since we're justifying the approaches, I'd like to explain why I
> preferred the return approach: it performs less tests. While this
> might sound like premature optimizations, performance is not why I
> think it's a good idea. It makes the fix easier to verify; you don't
> need to validate that the conditions of the second loop won't happen,
> because the code exits quickly.
>
> If we added something that required cleanup, we could change the
> return to a goto with a cleanup-label, and it would still be
> relatively easy to see what's going on.
>
>> However, I have no strong opinion on this, so please apply the version you
>> like better.
>
> Since the issue is present in mainline Git as well, I'd prefer if
> Junio merged whatever he prefers. I can produce a proper patch out of
> your suggesting, if needed.