On Wed, Dec 5, 2012 at 5:02 PM, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> 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@xxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html