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

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

[Erik Faye-Lund]

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.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---

Here's a quick patch for a small issue I recently encountered; when
deleting a file from inside a directory, we currently end up
prompting the user if (s)he want us to retry deleting the directory
they are in.

 compat/mingw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 1eb974f..2c29667 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -260,10 +260,10 @@ int mingw_rmdir(const char *pathname)
 
 	while ((ret = _wrmdir(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {
 		if (!is_file_in_use_error(GetLastError()))
-			break;
+			return ret;
 		if (!is_dir_empty(wpathname)) {
 			errno = ENOTEMPTY;
-			break;
+			return ret;
 		}
 		/*
 		 * We assume that some other process had the source or
-- 
1.8.0.msysgit.0.3.g0262b9f.dirty

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

[Johannes Schindelin]

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

[Erik Faye-Lund]

Sorry for a late reply.

On Tue, Dec 4, 2012 at 5:35 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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);

Yes, as long as you do s/EACCESS/EACCES/ first. I don't mind such a
version instead.

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

[Johannes Schindelin]

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

However, I have no strong opinion on this, so please apply the version you
like better.

Ciao,
Dscho

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

[Erik Faye-Lund]

On Wed, Dec 5, 2012 at 5:02 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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).
>

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.

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

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

Thanks; what you and Dscho agreed in this discussion sounds good to
me, too.

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en