[PATCH v3 0/5] make open/unlink failures user friendly on windows using retry/abort

[PATCH v3 0/5] make open/unlink failures user friendly on windows using retry/abort

[Heiko Voigt]

Hi,

On Wed, Nov 10, 2010 at 09:32:00PM +0100, Johannes Sixt wrote:
> On Dienstag, 9. November 2010, Heiko Voigt wrote:
> > So it seems that unlink also has the problem of getting an
> > ERROR_ACCESS_DENIED (Code 5) sometimes. Johannes would you agree that
> > including this code into the is_file_in_use_error() function and thus
> > having the potential risk of a 71ms delay for real access denials for
> > these calls makes sense?
> 
> Of course, it matches my own observations.

Sorry for the delay. Here is finally a new iteration of the patch
series. I hope I have addressed all raised issues. An extra pair of eyes
is appreciated.

This series has been ported to Junios tree.

I also added one patch from Johannes which I think should be part of
this series.

Cheers Heiko

Heiko Voigt (4):
  mingw: move unlink wrapper to mingw.c
  mingw: work around irregular failures of unlink on windows
  mingw: make failures to unlink or move raise a question
  mingw: add fallback for rmdir in case directory is in use

Johannes Schindelin (1):
  mingw_rmdir: set errno=ENOTEMPTY when appropriate

 compat/mingw.c |  172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 compat/mingw.h |   14 ++---
 2 files changed, 177 insertions(+), 9 deletions(-)

-- 
1.7.3.3.566.gf422f

[PATCH v3 1/8] mingw: move unlink wrapper to mingw.c

[Heiko Voigt]

The next patch implements a workaround in case unlink fails on Windows.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |    8 ++++++++
 compat/mingw.h |   11 +++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index bee6054..a7e1c6b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -116,6 +116,14 @@ int err_win_to_posix(DWORD winerr)
 	return error;
 }
 
+#undef unlink
+int mingw_unlink(const char *pathname)
+{
+	/* read-only files cannot be removed */
+	chmod(pathname, 0666);
+	return unlink(pathname);
+}
+
 #undef open
 int mingw_open (const char *filename, int oflags, ...)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index 2283071..8316938 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -125,14 +125,6 @@ static inline int mingw_mkdir(const char *path, int mode)
 }
 #define mkdir mingw_mkdir
 
-static inline int mingw_unlink(const char *pathname)
-{
-	/* read-only files cannot be removed */
-	chmod(pathname, 0666);
-	return unlink(pathname);
-}
-#define unlink mingw_unlink
-
 #define WNOHANG 1
 pid_t waitpid(pid_t pid, int *status, unsigned options);
 
@@ -180,6 +172,9 @@ int link(const char *oldpath, const char *newpath);
  * replacements of existing functions
  */
 
+int mingw_unlink(const char *pathname);
+#define unlink mingw_unlink
+
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
-- 
1.7.3.3.566.gf422f

[PATCH v3 2/8] mingw: work around irregular failures of unlink on windows

[Heiko Voigt]

If a file is opened by another process (e.g. indexing of an IDE) for
reading it is not allowed to be deleted. So in case unlink fails retry
after waiting for some time. This extends the workaround from 6ac6f878.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index a7e1c6b..52183a7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3,6 +3,8 @@
 #include <conio.h>
 #include "../strbuf.h"
 
+static const int delay[] = { 0, 1, 10, 20, 40 };
+
 int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
@@ -116,12 +118,38 @@ int err_win_to_posix(DWORD winerr)
 	return error;
 }
 
+static inline int is_file_in_use_error(DWORD errcode)
+{
+	switch(GetLastError()) {
+	case ERROR_SHARING_VIOLATION:
+	case ERROR_ACCESS_DENIED:
+		return 1;
+	}
+
+	return 0;
+}
+
 #undef unlink
 int mingw_unlink(const char *pathname)
 {
+	int ret, tries = 0;
+
 	/* read-only files cannot be removed */
 	chmod(pathname, 0666);
-	return unlink(pathname);
+	while ((ret = unlink(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
+		if (!is_file_in_use_error(GetLastError()))
+			break;
+		/*
+		 * We assume that some other process had the source or
+		 * destination file open at the wrong moment and retry.
+		 * In order to give the other process a higher chance to
+		 * complete its operation, we give up our time slice now.
+		 * If we have to retry again, we do sleep a bit.
+		 */
+		Sleep(delay[tries]);
+		tries++;
+	}
+	return ret;
 }
 
 #undef open
@@ -1257,7 +1285,6 @@ int mingw_rename(const char *pold, const char *pnew)
 {
 	DWORD attrs, gle;
 	int tries = 0;
-	static const int delay[] = { 0, 1, 10, 20, 40 };
 
 	/*
 	 * Try native rename() first to get errno right.
-- 
1.7.3.3.566.gf422f

Re: [PATCH v3 2/8] mingw: work around irregular failures of unlink on windows

[Erik Faye-Lund]

On Tue, Dec 14, 2010 at 11:11 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> If a file is opened by another process (e.g. indexing of an IDE) for
> reading it is not allowed to be deleted. So in case unlink fails retry
> after waiting for some time. This extends the workaround from 6ac6f878.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  compat/mingw.c |   31 +++++++++++++++++++++++++++++--
>  1 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index a7e1c6b..52183a7 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -3,6 +3,8 @@
>  #include <conio.h>
>  #include "../strbuf.h"
>
> +static const int delay[] = { 0, 1, 10, 20, 40 };
> +
>  int err_win_to_posix(DWORD winerr)
>  {
>        int error = ENOSYS;
> @@ -116,12 +118,38 @@ int err_win_to_posix(DWORD winerr)
>        return error;
>  }
>
> +static inline int is_file_in_use_error(DWORD errcode)
> +{
> +       switch(GetLastError()) {

Why pass the error code in, just to ignore it? Shouldn't this switch
on "errcode" instead?

Re: Re: [PATCH v3 2/8] mingw: work around irregular failures of unlink on windows

[Heiko Voigt]

On Tue, Dec 14, 2010 at 11:14:13PM +0100, Erik Faye-Lund wrote:
> On Tue, Dec 14, 2010 at 11:11 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > +static inline int is_file_in_use_error(DWORD errcode)
> > +{
> > +       switch(GetLastError()) {
> 
> Why pass the error code in, just to ignore it? Shouldn't this switch
> on "errcode" instead?

Definitely. Thanks for spotting that.

Cheers Heiko

[PATCH v4 2/5] mingw: work around irregular failures of unlink on windows

[Heiko Voigt]

If a file is opened by another process (e.g. indexing of an IDE) for
reading it is not allowed to be deleted. So in case unlink fails retry
after waiting for some time. This extends the workaround from 6ac6f878.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
Here is a replacement for the previous patch.

 compat/mingw.c |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index a7e1c6b..4a1c218 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3,6 +3,8 @@
 #include <conio.h>
 #include "../strbuf.h"
 
+static const int delay[] = { 0, 1, 10, 20, 40 };
+
 int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
@@ -116,12 +118,38 @@ int err_win_to_posix(DWORD winerr)
 	return error;
 }
 
+static inline int is_file_in_use_error(DWORD errcode)
+{
+	switch(errcode) {
+	case ERROR_SHARING_VIOLATION:
+	case ERROR_ACCESS_DENIED:
+		return 1;
+	}
+
+	return 0;
+}
+
 #undef unlink
 int mingw_unlink(const char *pathname)
 {
+	int ret, tries = 0;
+
 	/* read-only files cannot be removed */
 	chmod(pathname, 0666);
-	return unlink(pathname);
+	while ((ret = unlink(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
+		if (!is_file_in_use_error(GetLastError()))
+			break;
+		/*
+		 * We assume that some other process had the source or
+		 * destination file open at the wrong moment and retry.
+		 * In order to give the other process a higher chance to
+		 * complete its operation, we give up our time slice now.
+		 * If we have to retry again, we do sleep a bit.
+		 */
+		Sleep(delay[tries]);
+		tries++;
+	}
+	return ret;
 }
 
 #undef open
@@ -1257,7 +1285,6 @@ int mingw_rename(const char *pold, const char *pnew)
 {
 	DWORD attrs, gle;
 	int tries = 0;
-	static const int delay[] = { 0, 1, 10, 20, 40 };
 
 	/*
 	 * Try native rename() first to get errno right.
-- 
1.7.3.3.566.g98577

[PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Heiko Voigt]

On Windows in case a program is accessing a file unlink or
move operations may fail. To give the user a chance to correct
this we simply wait until the user asks us to retry or fail.

This is useful because of the following use case which seem
to happen rarely but when it does it is a mess:

After making some changes the user realizes that he was on the
incorrect branch. When trying to change the branch some file
is still in use by some other process and git stops in the
middle of changing branches. Now the user has lots of files
with changes mixed with his own. This is especially confusing
on repositories that contain lots of files.

Although the recent implementation of automatic retry makes
this scenario much more unlikely lets provide a fallback as
a last resort.

Thanks to Albert Dvornik for disabling the question if users can't see it.

If the stdout of the command is connected to a terminal but the stderr
has been redirected, the odds are good that the user can't see any
question we print out to stderr.  This will result in a "mysterious
hang" while the app is waiting for user input.

It seems better to be conservative, and avoid asking for input
whenever the stderr is not a terminal, just like we do for stdin.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Albert Dvornik <dvornik+git@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
I have added the sign-off from the squashed commit of Albert and
Johannes. I hope its ok this way.

 compat/mingw.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 52183a7..ac9fb4a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2,6 +2,7 @@
 #include "win32.h"
 #include <conio.h>
 #include "../strbuf.h"
+#include "../run-command.h"
 
 static const int delay[] = { 0, 1, 10, 20, 40 };
 
@@ -129,6 +130,78 @@ static inline int is_file_in_use_error(DWORD errcode)
 	return 0;
 }
 
+static int read_yes_no_answer()
+{
+	char answer[1024];
+
+	if (fgets(answer, sizeof(answer), stdin)) {
+		size_t answer_len = strlen(answer);
+		int got_full_line = 0, c;
+
+		/* remove the newline */
+		if (answer_len >= 2 && answer[answer_len-2] == '\r') {
+			answer[answer_len-2] = '\0';
+			got_full_line = 1;
+		}
+		else if (answer_len >= 1 && answer[answer_len-1] == '\n') {
+			answer[answer_len-1] = '\0';
+			got_full_line = 1;
+		}
+		/* flush the buffer in case we did not get the full line */
+		if (!got_full_line)
+			while((c = getchar()) != EOF && c != '\n');
+	} else
+		/* we could not read, return the
+		 * default answer which is no */
+		return 0;
+
+	if (answer[0] == 'y' && strlen(answer) == 1)
+		return 1;
+	if (!strncasecmp(answer, "yes", sizeof(answer)))
+		return 1;
+	if (answer[0] == 'n' && strlen(answer) == 1)
+		return 0;
+	if (!strncasecmp(answer, "no", sizeof(answer)))
+		return 0;
+
+	/* did not find an answer we understand */
+	return -1;
+}
+
+static int ask_user_yes_no(const char *format, ...)
+{
+	char question[4096];
+	const char *retry_hook[] = { NULL, NULL, NULL };
+	va_list args;
+
+	if ((retry_hook[0] = getenv("GIT_ASK_YESNO"))) {
+
+		va_start(args, format);
+		vsnprintf(question, sizeof(question), format, args);
+		va_end(args);
+
+		retry_hook[1] = question;
+		return !run_command_v_opt(retry_hook, 0);
+	}
+
+	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
+		return 0;
+
+	while (1) {
+		int answer;
+		va_start(args, format);
+		vfprintf(stderr, format, args);
+		va_end(args);
+		fprintf(stderr, " (y/n)? ");
+
+		if ((answer = read_yes_no_answer()) >= 0)
+			return answer;
+
+		fprintf(stderr, "Sorry, I did not understand your answer. "
+				"Please type 'y' or 'n'\n");
+	}
+}
+
 #undef unlink
 int mingw_unlink(const char *pathname)
 {
@@ -149,6 +222,10 @@ int mingw_unlink(const char *pathname)
 		Sleep(delay[tries]);
 		tries++;
 	}
+	while (ret == -1 && is_file_in_use_error(GetLastError()) &&
+	       ask_user_yes_no("Unlink of file '%s' failed. "
+			"Should I try again?", pathname))
+	       ret = unlink(pathname);
 	return ret;
 }
 
@@ -1326,6 +1403,11 @@ repeat:
 		tries++;
 		goto repeat;
 	}
+	if (gle == ERROR_ACCESS_DENIED &&
+	       ask_user_yes_no("Rename from '%s' to '%s' failed. "
+		       "Should I try again?", pold, pnew))
+		goto repeat;
+
 	errno = EACCES;
 	return -1;
 }
-- 
1.7.3.3.566.gf422f

Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Erik Faye-Lund]

On Tue, Dec 14, 2010 at 11:21 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Windows in case a program is accessing a file unlink or
> move operations may fail. To give the user a chance to correct
> this we simply wait until the user asks us to retry or fail.
>
> This is useful because of the following use case which seem
> to happen rarely but when it does it is a mess:
>
> After making some changes the user realizes that he was on the
> incorrect branch. When trying to change the branch some file
> is still in use by some other process and git stops in the
> middle of changing branches. Now the user has lots of files
> with changes mixed with his own. This is especially confusing
> on repositories that contain lots of files.
>
> Although the recent implementation of automatic retry makes
> this scenario much more unlikely lets provide a fallback as
> a last resort.
>
> Thanks to Albert Dvornik for disabling the question if users can't see it.
>
> If the stdout of the command is connected to a terminal but the stderr
> has been redirected, the odds are good that the user can't see any
> question we print out to stderr.  This will result in a "mysterious
> hang" while the app is waiting for user input.
>
> It seems better to be conservative, and avoid asking for input
> whenever the stderr is not a terminal, just like we do for stdin.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> Signed-off-by: Albert Dvornik <dvornik+git@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> I have added the sign-off from the squashed commit of Albert and
> Johannes. I hope its ok this way.
>
>  compat/mingw.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 82 insertions(+), 0 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 52183a7..ac9fb4a 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2,6 +2,7 @@
>  #include "win32.h"
>  #include <conio.h>
>  #include "../strbuf.h"
> +#include "../run-command.h"
>
>  static const int delay[] = { 0, 1, 10, 20, 40 };
>
> @@ -129,6 +130,78 @@ static inline int is_file_in_use_error(DWORD errcode)
>        return 0;
>  }
>
> +static int read_yes_no_answer()

Perhaps "static int read_yes_no_answer(void)" for portability?

> +{
> +       char answer[1024];
> +
> +       if (fgets(answer, sizeof(answer), stdin)) {
> +               size_t answer_len = strlen(answer);
> +               int got_full_line = 0, c;
> +
> +               /* remove the newline */
> +               if (answer_len >= 2 && answer[answer_len-2] == '\r') {
> +                       answer[answer_len-2] = '\0';
> +                       got_full_line = 1;
> +               }
> +               else if (answer_len >= 1 && answer[answer_len-1] == '\n') {
> +                       answer[answer_len-1] = '\0';
> +                       got_full_line = 1;
> +               }
> +               /* flush the buffer in case we did not get the full line */
> +               if (!got_full_line)
> +                       while((c = getchar()) != EOF && c != '\n');
> +       } else
> +               /* we could not read, return the
> +                * default answer which is no */
> +               return 0;
> +
> +       if (answer[0] == 'y' && strlen(answer) == 1)
> +               return 1;
> +       if (!strncasecmp(answer, "yes", sizeof(answer)))
> +               return 1;
> +       if (answer[0] == 'n' && strlen(answer) == 1)
> +               return 0;
> +       if (!strncasecmp(answer, "no", sizeof(answer)))
> +               return 0;

Since you're doing case insensitive checks for "yes" and "no", perhaps
it'd make sense to allow upper case 'Y' and 'N' also? Something like:

-       if (answer[0] == 'n' && strlen(answer) == 1)
+       if (tolower(answer[0]) == 'n' && strlen(answer) == 1)

hm?


> +static int ask_user_yes_no(const char *format, ...)
> +{
> +       char question[4096];
> +       const char *retry_hook[] = { NULL, NULL, NULL };
> +       va_list args;
> +
> +       if ((retry_hook[0] = getenv("GIT_ASK_YESNO"))) {
> +
> +               va_start(args, format);
> +               vsnprintf(question, sizeof(question), format, args);
> +               va_end(args);
> +
> +               retry_hook[1] = question;
> +               return !run_command_v_opt(retry_hook, 0);
> +       }
> +
> +       if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
> +               return 0;

I'm wondering, doesn't this make the semantics a bit wrong? The
function is called "ask_user_yes_no", but it might end up not asking
after all. Perhaps it should be called something that reflects this?
"maybe_ask_yes_no", "ask_yes_no_if_tty", "should_retry"? I don't have
a non-ugly suggestion, but I suspect something like that might leave
other people less puzzled when reading the code.

Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Junio C Hamano]

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

> Since you're doing case insensitive checks for "yes" and "no", perhaps
> it'd make sense to allow upper case 'Y' and 'N' also? Something like:
>
> -       if (answer[0] == 'n' && strlen(answer) == 1)
> +       if (tolower(answer[0]) == 'n' && strlen(answer) == 1)
>
> hm?

Why not

	if (tolower(answer[0]) == 'n' && !answer[1])

think of the case answer[] is very long ;-)

>> +       if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
>> +               return 0;
>
> I'm wondering, doesn't this make the semantics a bit wrong? The
> function is called "ask_user_yes_no", but it might end up not asking
> after all.

I don't think that is such a big issue.

Imagine you had only getenv("GIT_ASK_YESNO") codepath, and no fallback
"tty" codepath.  And you ship with a separate program as a default
"asker".

The implementation of that asker happens to read yes/no from the tty, but
it defauts to "no" if there is no tty interaction available.

If you view it that way, the code we see above is just an optimization to
avoid spawning that default "asker" as a separate process.

I was more puzzled by the code to formulate question[]; why doesn't it
build the same question for both codepaths and spit that out to stderr
with fputs() in the fallvack asker?

Re: Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Heiko Voigt]

Hi,

On Tue, Dec 14, 2010 at 03:52:52PM -0800, Junio C Hamano wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
> 
> > Since you're doing case insensitive checks for "yes" and "no", perhaps
> > it'd make sense to allow upper case 'Y' and 'N' also? Something like:
> >
> > -       if (answer[0] == 'n' && strlen(answer) == 1)
> > +       if (tolower(answer[0]) == 'n' && strlen(answer) == 1)
> >
> > hm?
> 
> Why not
> 
> 	if (tolower(answer[0]) == 'n' && !answer[1])
> 
> think of the case answer[] is very long ;-)

Will change as stated in the previous email but of course using this
code for efficiency ;) Can anybody estimate how fast the user would need
to type to actually make this noticeable and how much heat that would
produce on the keyboard?

> >> +       if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
> >> +               return 0;
> >
> > I'm wondering, doesn't this make the semantics a bit wrong? The
> > function is called "ask_user_yes_no", but it might end up not asking
> > after all.
> 
> I don't think that is such a big issue.
> 
> Imagine you had only getenv("GIT_ASK_YESNO") codepath, and no fallback
> "tty" codepath.  And you ship with a separate program as a default
> "asker".
> 
> The implementation of that asker happens to read yes/no from the tty, but
> it defauts to "no" if there is no tty interaction available.
> 
> If you view it that way, the code we see above is just an optimization to
> avoid spawning that default "asker" as a separate process.

I do not mind to change this function name to make it more match what
its doing. Since code is read way more often than written I think this
makes sense. See my other email about the suggestion.

> I was more puzzled by the code to formulate question[]; why doesn't it
> build the same question for both codepaths and spit that out to stderr
> with fputs() in the fallvack asker?

Do you mean that I append " (y/n)? " ? For the (y/n) you can think of it
as the tui implementation of the yes/no button. I can see that the ?
might need to go into the question string itself. Is it that what you
meant?

Cheers Heiko

Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5394 bytes --]

Hi,

On Tue, 14 Dec 2010, Erik Faye-Lund wrote:

> On Tue, Dec 14, 2010 at 11:21 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > On Windows in case a program is accessing a file unlink or
> > move operations may fail. To give the user a chance to correct
> > this we simply wait until the user asks us to retry or fail.
> >
> > This is useful because of the following use case which seem
> > to happen rarely but when it does it is a mess:
> >
> > After making some changes the user realizes that he was on the
> > incorrect branch. When trying to change the branch some file
> > is still in use by some other process and git stops in the
> > middle of changing branches. Now the user has lots of files
> > with changes mixed with his own. This is especially confusing
> > on repositories that contain lots of files.
> >
> > Although the recent implementation of automatic retry makes
> > this scenario much more unlikely lets provide a fallback as
> > a last resort.
> >
> > Thanks to Albert Dvornik for disabling the question if users can't see it.
> >
> > If the stdout of the command is connected to a terminal but the stderr
> > has been redirected, the odds are good that the user can't see any
> > question we print out to stderr.  This will result in a "mysterious
> > hang" while the app is waiting for user input.
> >
> > It seems better to be conservative, and avoid asking for input
> > whenever the stderr is not a terminal, just like we do for stdin.
> >
> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> > Signed-off-by: Albert Dvornik <dvornik+git@gmail.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > I have added the sign-off from the squashed commit of Albert and
> > Johannes. I hope its ok this way.

I'm fine with it.

> >  compat/mingw.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 82 insertions(+), 0 deletions(-)
> >
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index 52183a7..ac9fb4a 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -2,6 +2,7 @@
> >  #include "win32.h"
> >  #include <conio.h>
> >  #include "../strbuf.h"
> > +#include "../run-command.h"
> >
> >  static const int delay[] = { 0, 1, 10, 20, 40 };
> >
> > @@ -129,6 +130,78 @@ static inline int is_file_in_use_error(DWORD errcode)
> >        return 0;
> >  }
> >
> > +static int read_yes_no_answer()
> 
> Perhaps "static int read_yes_no_answer(void)" for portability?

LOL. This file is called compat/mingw.c... :-)

But I have no objection to stay with the convention of the rest of Git. 
Nobody needs to convince me that consistency is good.

> > +{
> > +       char answer[1024];
> > +
> > +       if (fgets(answer, sizeof(answer), stdin)) {
> > +               size_t answer_len = strlen(answer);
> > +               int got_full_line = 0, c;
> > +
> > +               /* remove the newline */
> > +               if (answer_len >= 2 && answer[answer_len-2] == '\r') {
> > +                       answer[answer_len-2] = '\0';
> > +                       got_full_line = 1;
> > +               }
> > +               else if (answer_len >= 1 && answer[answer_len-1] == '\n') {
> > +                       answer[answer_len-1] = '\0';
> > +                       got_full_line = 1;
> > +               }
> > +               /* flush the buffer in case we did not get the full line */
> > +               if (!got_full_line)
> > +                       while((c = getchar()) != EOF && c != '\n');
> > +       } else
> > +               /* we could not read, return the
> > +                * default answer which is no */
> > +               return 0;
> > +
> > +       if (answer[0] == 'y' && strlen(answer) == 1)
> > +               return 1;
> > +       if (!strncasecmp(answer, "yes", sizeof(answer)))
> > +               return 1;
> > +       if (answer[0] == 'n' && strlen(answer) == 1)
> > +               return 0;
> > +       if (!strncasecmp(answer, "no", sizeof(answer)))
> > +               return 0;
> 
> Since you're doing case insensitive checks for "yes" and "no", perhaps
> it'd make sense to allow upper case 'Y' and 'N' also? Something like:
> 
> -       if (answer[0] == 'n' && strlen(answer) == 1)
> +       if (tolower(answer[0]) == 'n' && strlen(answer) == 1)
> 
> hm?

Makes sense to me.

> > +static int ask_user_yes_no(const char *format, ...)
> > +{
> > +       char question[4096];
> > +       const char *retry_hook[] = { NULL, NULL, NULL };
> > +       va_list args;
> > +
> > +       if ((retry_hook[0] = getenv("GIT_ASK_YESNO"))) {
> > +
> > +               va_start(args, format);
> > +               vsnprintf(question, sizeof(question), format, args);
> > +               va_end(args);
> > +
> > +               retry_hook[1] = question;
> > +               return !run_command_v_opt(retry_hook, 0);
> > +       }
> > +
> > +       if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
> > +               return 0;
> 
> I'm wondering, doesn't this make the semantics a bit wrong? The
> function is called "ask_user_yes_no", but it might end up not asking
> after all. Perhaps it should be called something that reflects this?
> "maybe_ask_yes_no", "ask_yes_no_if_tty", "should_retry"? I don't have
> a non-ugly suggestion, but I suspect something like that might leave
> other people less puzzled when reading the code.

I like ask_yes_no_if_tty.

Ciao,
Dscho

Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > @@ -129,6 +130,78 @@ static inline int is_file_in_use_error(DWORD errcode)
>> >        return 0;
>> >  }
>> >
>> > +static int read_yes_no_answer()
>> 
>> Perhaps "static int read_yes_no_answer(void)" for portability?
>
> LOL. This file is called compat/mingw.c... :-)

I had the same reaction.  Maybe MinGW will get a different compiler
someday ;-)

> But I have no objection to stay with the convention of the rest of Git. 
> Nobody needs to convince me that consistency is good.

I recall there are a few old-style declaration in compat/ directory,
especially in borrowed code like nedmalloc/ and possibly regex/, and
I am not so sure if we want to touch them.

I'll leave this up to msysgit folks.

Thanks.

Re: Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Heiko Voigt]

On Tue, Dec 14, 2010 at 07:05:00PM -0800, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > @@ -129,6 +130,78 @@ static inline int is_file_in_use_error(DWORD errcode)
> >> >        return 0;
> >> >  }
> >> >
> >> > +static int read_yes_no_answer()
> >> 
> >> Perhaps "static int read_yes_no_answer(void)" for portability?
> >
> > LOL. This file is called compat/mingw.c... :-)
> 
> I had the same reaction.  Maybe MinGW will get a different compiler
> someday ;-)
> 
> > But I have no objection to stay with the convention of the rest of Git. 
> > Nobody needs to convince me that consistency is good.
> 
> I recall there are a few old-style declaration in compat/ directory,
> especially in borrowed code like nedmalloc/ and possibly regex/, and
> I am not so sure if we want to touch them.
> 
> I'll leave this up to msysgit folks.

I do not mind changing this sincce I need to change this patch anyway.
Will do in the next iteration.

Cheers Heiko

Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Erik Faye-Lund]

On Wed, Dec 15, 2010 at 4:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > @@ -129,6 +130,78 @@ static inline int is_file_in_use_error(DWORD errcode)
>>> >        return 0;
>>> >  }
>>> >
>>> > +static int read_yes_no_answer()
>>>
>>> Perhaps "static int read_yes_no_answer(void)" for portability?
>>
>> LOL. This file is called compat/mingw.c... :-)
>
> I had the same reaction.  Maybe MinGW will get a different compiler
> someday ;-)
>

We already have; compat/msvc.c includes compat/mingw.c. mingw.c is
called mingw.c because it was the first native windows port, not
because it will always be compiled with MinGW. So this file is REALLY
more about the OS than the compiler.

I don't think MSVC has a problem with this declaration either, but
wouldn't it be nicer if we had Windows-code that was as portable as
possible across compilers? I've also been playing around with the idea
of using LLVM's clang for Git on Windows, because it's support for
cross compiling between 32bit and 64bit is a bit less nasty than
MinGW's. This might never come happen, and I don't know if clang
supports this or not. And then there's Intel's ICC that some times
outperforms GCC. I don't think it would hurt fixing it in case people
will port - one less trip-wire in the code.

But I of course only suggest this because this is new code. It's easy
to change it to be slightly more portable (and more consistent with
the rest of the code base), so why not?

Re: Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Heiko Voigt]

Hi,

On Wed, Dec 15, 2010 at 01:11:03AM +0100, Johannes Schindelin wrote:
> On Tue, 14 Dec 2010, Erik Faye-Lund wrote:
> 
> > On Tue, Dec 14, 2010 at 11:21 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
[...]
> > > +       if (answer[0] == 'y' && strlen(answer) == 1)
> > > +               return 1;
> > > +       if (!strncasecmp(answer, "yes", sizeof(answer)))
> > > +               return 1;
> > > +       if (answer[0] == 'n' && strlen(answer) == 1)
> > > +               return 0;
> > > +       if (!strncasecmp(answer, "no", sizeof(answer)))
> > > +               return 0;
> > 
> > Since you're doing case insensitive checks for "yes" and "no", perhaps
> > it'd make sense to allow upper case 'Y' and 'N' also? Something like:
> > 
> > -       if (answer[0] == 'n' && strlen(answer) == 1)
> > +       if (tolower(answer[0]) == 'n' && strlen(answer) == 1)
> > 
> > hm?
> 
> Makes sense to me.

Will do in the next iteration.

> > > +static int ask_user_yes_no(const char *format, ...)
[...]
> > 
> > I'm wondering, doesn't this make the semantics a bit wrong? The
> > function is called "ask_user_yes_no", but it might end up not asking
> > after all. Perhaps it should be called something that reflects this?
> > "maybe_ask_yes_no", "ask_yes_no_if_tty", "should_retry"? I don't have
> > a non-ugly suggestion, but I suspect something like that might leave
> > other people less puzzled when reading the code.
> 
> I like ask_yes_no_if_tty.

How about ask_yes_no_if_possible() ? Since we do not just rely on tty
but still have GIT_ASK_YESNO to get to the user I think this would be
a closer description of what happens.

Cheers Heiko

[PATCH v3 4/5] mingw: add fallback for rmdir in case directory is in use

[Heiko Voigt]

The same logic as for unlink and rename also applies to rmdir. For
example in case you have a shell open in a git controlled folder. This
will easily fail. So lets be nice for such cases as well.

Signed-off-by: Heiko Voigt <heiko.voigt@mahr.de>
---
I just realized that there was a wrong patch count in the subject. It
should have been 5 instead of 8 all the time.

 compat/mingw.c |   25 +++++++++++++++++++++++++
 compat/mingw.h |    3 +++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index ac9fb4a..b920644 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -229,6 +229,31 @@ int mingw_unlink(const char *pathname)
 	return ret;
 }
 
+#undef rmdir
+int mingw_rmdir(const char *pathname)
+{
+	int ret, tries = 0;
+
+	while ((ret = rmdir(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
+		if (!is_file_in_use_error(GetLastError()))
+			break;
+		/*
+		 * We assume that some other process had the source or
+		 * destination file open at the wrong moment and retry.
+		 * In order to give the other process a higher chance to
+		 * complete its operation, we give up our time slice now.
+		 * If we have to retry again, we do sleep a bit.
+		 */
+		Sleep(delay[tries]);
+		tries++;
+	}
+	while (ret == -1 && is_file_in_use_error(GetLastError()) &&
+	       ask_user_yes_no("Deletion of directory '%s' failed. "
+			"Should I try again?", pathname))
+	       ret = rmdir(pathname);
+	return ret;
+}
+
 #undef open
 int mingw_open (const char *filename, int oflags, ...)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index 8316938..8b159c4 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -175,6 +175,9 @@ int link(const char *oldpath, const char *newpath);
 int mingw_unlink(const char *pathname);
 #define unlink mingw_unlink
 
+int mingw_rmdir(const char *path);
+#define rmdir mingw_rmdir
+
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
-- 
1.7.3.3.566.gf422f

[PATCH v3 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Heiko Voigt]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, EACCES overrules ENOTEMPTY when calling rmdir(). But if the
directory is busy, we only want to retry deleting the directory if it
is empty, so test specifically for that case and set ENOTEMPTY rather
than EACCES.

Noticed by Greg Hazel.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index b920644..3e013c6 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -229,6 +229,30 @@ int mingw_unlink(const char *pathname)
 	return ret;
 }
 
+static int is_dir_empty(const char *path)
+{
+	struct strbuf buf = STRBUF_INIT;
+	WIN32_FIND_DATAA findbuf;
+	HANDLE handle;
+
+	strbuf_addf(&buf, "%s\\*", path);
+	handle = FindFirstFileA(buf.buf, &findbuf);
+	if (handle == INVALID_HANDLE_VALUE) {
+		strbuf_release(&buf);
+		return GetLastError() == ERROR_NO_MORE_FILES;
+	}
+
+	while (!strcmp(findbuf.cFileName, ".") ||
+			!strcmp(findbuf.cFileName, ".."))
+		if (!FindNextFile(handle, &findbuf)) {
+			strbuf_release(&buf);
+			return GetLastError() == ERROR_NO_MORE_FILES;
+		}
+	FindClose(handle);
+	strbuf_release(&buf);
+	return 0;
+}
+
 #undef rmdir
 int mingw_rmdir(const char *pathname)
 {
@@ -237,6 +261,10 @@ int mingw_rmdir(const char *pathname)
 	while ((ret = rmdir(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
 		if (!is_file_in_use_error(GetLastError()))
 			break;
+		if (!is_dir_empty(pathname)) {
+			errno = ENOTEMPTY;
+			break;
+		}
 		/*
 		 * We assume that some other process had the source or
 		 * destination file open at the wrong moment and retry.
-- 
1.7.3.3.566.gf422f

Re: [PATCH v3 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Erik Faye-Lund]

On Tue, Dec 14, 2010 at 11:28 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> On Windows, EACCES overrules ENOTEMPTY when calling rmdir(). But if the
> directory is busy, we only want to retry deleting the directory if it
> is empty, so test specifically for that case and set ENOTEMPTY rather
> than EACCES.
>

Hmm... According to MSDN, rmdir(*) should already handle ENOTEMPTY.
Isn't the problem rather the structure of that loop? Shouldn't it be
sufficient to do something like this (note: untested, but the concept
should work, no)?

---8<---
#undef rmdir
int mingw_rmdir(const char *pathname)
{
       int ret, tries;

       for (tries = 0; tries < ARRAY_SIZE(delay); ++tries) {
               if (!(ret = rmdir(pathname)) || errno == ENOTEMPTY ||
                   !is_file_in_use_error(GetLastError()))
                      return ret;

               /*
                * We assume that some other process had the source or
                * destination file open at the wrong moment and retry.
                * In order to give the other process a higher chance to
                * complete its operation, we give up our time slice now.
                * If we have to retry again, we do sleep a bit.
                */
               Sleep(delay[tries]);
       }
       while (is_file_in_use_error(GetLastError()) &&
              ask_user_yes_no("Deletion of directory '%s' failed. "
                       "Should I try again?", pathname))
              ret = rmdir(pathname);
       return ret;
}
---8<---

(*) http://msdn.microsoft.com/en-us/library/wt8es881(v=VS.80).aspx

Re: [PATCH v3 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Johannes Schindelin]

Hi,

On Tue, 14 Dec 2010, Erik Faye-Lund wrote:

> On Tue, Dec 14, 2010 at 11:28 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > On Windows, EACCES overrules ENOTEMPTY when calling rmdir(). But if the
> > directory is busy, we only want to retry deleting the directory if it
> > is empty, so test specifically for that case and set ENOTEMPTY rather
> > than EACCES.
> >
> 
> Hmm... According to MSDN, rmdir(*) should already handle ENOTEMPTY. 
> Isn't the problem rather the structure of that loop? Shouldn't it be 
> sufficient to do something like this (note: untested, but the concept 
> should work, no)?

This is how the patch looks in current 4msysgit.git's devel branch:

-- snip --
 #undef rmdir
 int mingw_rmdir(const char *pathname)
 {
-    int ret, tries = 0;
+       int ret, tries = 0;
 
        while ((ret = rmdir(pathname)) == -1 && tries < ARRAY_SIZE(delay)) 
{
                if (errno != EACCES)
                        break;
+               if (!is_dir_empty(pathname)) {
+                       errno = ENOTEMPTY;
+                       break;
+               }
                /*
                 * We assume that some other process had the source or
                 * destination file open at the wrong moment and retry.
-- snap --

Of course, with so much water running down the Elbe between me writing 
that patch and me answering you, I cannot really say whether rmdir() 
set errno to ENOTEMPTY.

But as the patch looked the same when I wrote it originally (you can see 
it in the history, since I introduced rebasing merges prior to making this 
patch) I would assume that in my tests, errno was set to EACCESS rather 
than ENOTEMPTY.

However, since I am distrusted on the Git mailing list it would be good if 
you tried your version and verified whether what I say is true.

Thanks,
Dscho

Re: [PATCH v3 0/5] make open/unlink failures user friendly on windows using retry/abort

[Erik Faye-Lund]

On Tue, Dec 14, 2010 at 11:06 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> Hi,
>
> On Wed, Nov 10, 2010 at 09:32:00PM +0100, Johannes Sixt wrote:
>> On Dienstag, 9. November 2010, Heiko Voigt wrote:
>> > So it seems that unlink also has the problem of getting an
>> > ERROR_ACCESS_DENIED (Code 5) sometimes. Johannes would you agree that
>> > including this code into the is_file_in_use_error() function and thus
>> > having the potential risk of a 71ms delay for real access denials for
>> > these calls makes sense?
>>
>> Of course, it matches my own observations.
>
> Sorry for the delay. Here is finally a new iteration of the patch
> series. I hope I have addressed all raised issues. An extra pair of eyes
> is appreciated.
>
> This series has been ported to Junios tree.
>
> I also added one patch from Johannes which I think should be part of
> this series.
>
> Cheers Heiko
>
> Heiko Voigt (4):
>  mingw: move unlink wrapper to mingw.c
>  mingw: work around irregular failures of unlink on windows
>  mingw: make failures to unlink or move raise a question
>  mingw: add fallback for rmdir in case directory is in use
>
> Johannes Schindelin (1):
>  mingw_rmdir: set errno=ENOTEMPTY when appropriate
>
>  compat/mingw.c |  172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  compat/mingw.h |   14 ++---
>  2 files changed, 177 insertions(+), 9 deletions(-)
>

I like the goal of the series (and enjoy the end-result on mingw), but
the more I think of this the more I wonder if this is solving the
problem on the wrong abstraction level.

POSIX says that unlink et al can set errno to EBUSY if a file is in
use and the implementation considers that an error. I suspect they had
a reason for adding that, and I doubt that reason was Windows. POSIX
is already pretty incompatible with Windows. Some Googling have
suggested (but I haven't found definitive proof) that some flavors of
QNX (depending on the file system used, it seems) and Solaris might be
affected. Even Linux seems to be affected if the NTFS 3G driver is
used. Perhaps we've only seen this on Windows so far because
anti-virus is uncommon on these other systems? Perhaps something like
Dropbox-syncing on a mounted NTFS drive under Linux can cause this in
real life as well?

Perhaps instead we would be better of with something like an xunlink
(etc) in wrapper.c that deals with this on all platforms (and make
sure that unlink sets errno to EBUSY correctly if it doesn't already)?

Re: [PATCH v3 0/5] make open/unlink failures user friendly on windows using retry/abort

[Junio C Hamano]

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

> Perhaps instead we would be better of with something like an xunlink
> (etc) in wrapper.c that deals with this on all platforms (and make
> sure that unlink sets errno to EBUSY correctly if it doesn't already)?

That is superficially similar to the way we let xread() silently handle
short read to give us an easier to use API.  Especially, the part to
silently retry for a few times is similar to xread() recovering by
repeating short reads.

I do not think "ask the user one last time" part belongs to such a
wrapper, though.

[PATCH v4 0/5] make open/unlink failures user friendly on windows using retry/abort

[Heiko Voigt]

This is the fourth iteration of my patch series with hopefully all the
mentioned issues addressed.

On Wed, Dec 15, 2010 at 12:45:09PM -0800, Junio C Hamano wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
> 
> > Perhaps instead we would be better of with something like an xunlink
> > (etc) in wrapper.c that deals with this on all platforms (and make
> > sure that unlink sets errno to EBUSY correctly if it doesn't already)?
> 
> That is superficially similar to the way we let xread() silently handle
> short read to give us an easier to use API.  Especially, the part to
> silently retry for a few times is similar to xread() recovering by
> repeating short reads.
> 
> I do not think "ask the user one last time" part belongs to such a
> wrapper, though.

I think we should currently keep this Windows specific because this is the
only system where such problems occur in practise. If we find later on
that other systems are affected with the same problem we can still move
the functions to another more public place. But without proper testing
whether it helps on other setups as well putting this into a more
prominent place does not make sense, IMO.

Cheers Heiko

Heiko Voigt (4):
  mingw: move unlink wrapper to mingw.c
  mingw: work around irregular failures of unlink on windows
  mingw: make failures to unlink or move raise a question
  mingw: add fallback for rmdir in case directory is in use

Johannes Schindelin (1):
  mingw_rmdir: set errno=ENOTEMPTY when appropriate

 compat/mingw.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 compat/mingw.h |   14 ++---
 2 files changed, 173 insertions(+), 9 deletions(-)

-- 
1.7.4.34.gd2cb1

[PATCH v4 1/5] mingw: move unlink wrapper to mingw.c

[Heiko Voigt]

The next patch implements a workaround in case unlink fails on Windows.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |    8 ++++++++
 compat/mingw.h |   11 +++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index bee6054..a7e1c6b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -116,6 +116,14 @@ int err_win_to_posix(DWORD winerr)
 	return error;
 }
 
+#undef unlink
+int mingw_unlink(const char *pathname)
+{
+	/* read-only files cannot be removed */
+	chmod(pathname, 0666);
+	return unlink(pathname);
+}
+
 #undef open
 int mingw_open (const char *filename, int oflags, ...)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index cafc1eb..970d743 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -119,14 +119,6 @@ static inline int mingw_mkdir(const char *path, int mode)
 }
 #define mkdir mingw_mkdir
 
-static inline int mingw_unlink(const char *pathname)
-{
-	/* read-only files cannot be removed */
-	chmod(pathname, 0666);
-	return unlink(pathname);
-}
-#define unlink mingw_unlink
-
 #define WNOHANG 1
 pid_t waitpid(pid_t pid, int *status, unsigned options);
 
@@ -174,6 +166,9 @@ int link(const char *oldpath, const char *newpath);
  * replacements of existing functions
  */
 
+int mingw_unlink(const char *pathname);
+#define unlink mingw_unlink
+
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
-- 
1.7.4.34.gd2cb1

Re: [PATCH v4 1/5] mingw: move unlink wrapper to mingw.c

[Johannes Schindelin]

Hi,

On Mon, 7 Feb 2011, Heiko Voigt wrote:

> The next patch implements a workaround in case unlink fails on Windows.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

I merged the current 'next', which has them all.

Thanks!
Dscho

[PATCH v4 2/5] mingw: work around irregular failures of unlink on windows

[Heiko Voigt]

If a file is opened by another process (e.g. indexing of an IDE) for
reading it is not allowed to be deleted. So in case unlink fails retry
after waiting for some time. This extends the workaround from 6ac6f878.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index a7e1c6b..4a1c218 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3,6 +3,8 @@
 #include <conio.h>
 #include "../strbuf.h"
 
+static const int delay[] = { 0, 1, 10, 20, 40 };
+
 int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
@@ -116,12 +118,38 @@ int err_win_to_posix(DWORD winerr)
 	return error;
 }
 
+static inline int is_file_in_use_error(DWORD errcode)
+{
+	switch(errcode) {
+	case ERROR_SHARING_VIOLATION:
+	case ERROR_ACCESS_DENIED:
+		return 1;
+	}
+
+	return 0;
+}
+
 #undef unlink
 int mingw_unlink(const char *pathname)
 {
+	int ret, tries = 0;
+
 	/* read-only files cannot be removed */
 	chmod(pathname, 0666);
-	return unlink(pathname);
+	while ((ret = unlink(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
+		if (!is_file_in_use_error(GetLastError()))
+			break;
+		/*
+		 * We assume that some other process had the source or
+		 * destination file open at the wrong moment and retry.
+		 * In order to give the other process a higher chance to
+		 * complete its operation, we give up our time slice now.
+		 * If we have to retry again, we do sleep a bit.
+		 */
+		Sleep(delay[tries]);
+		tries++;
+	}
+	return ret;
 }
 
 #undef open
@@ -1257,7 +1285,6 @@ int mingw_rename(const char *pold, const char *pnew)
 {
 	DWORD attrs, gle;
 	int tries = 0;
-	static const int delay[] = { 0, 1, 10, 20, 40 };
 
 	/*
 	 * Try native rename() first to get errno right.
-- 
1.7.4.34.gd2cb1

[PATCH v4 3/5] mingw: make failures to unlink or move raise a question

[Heiko Voigt]

On Windows in case a program is accessing a file unlink or
move operations may fail. To give the user a chance to correct
this we simply wait until the user asks us to retry or fail.

This is useful because of the following use case which seem
to happen rarely but when it does it is a mess:

After making some changes the user realizes that he was on the
incorrect branch. When trying to change the branch some file
is still in use by some other process and git stops in the
middle of changing branches. Now the user has lots of files
with changes mixed with his own. This is especially confusing
on repositories that contain lots of files.

Although the recent implementation of automatic retry makes
this scenario much more unlikely lets provide a fallback as
a last resort.

Thanks to Albert Dvornik for disabling the question if users can't see it.

If the stdout of the command is connected to a terminal but the stderr
has been redirected, the odds are good that the user can't see any
question we print out to stderr.  This will result in a "mysterious
hang" while the app is waiting for user input.

It seems better to be conservative, and avoid asking for input
whenever the stderr is not a terminal, just like we do for stdin.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Albert Dvornik <dvornik+git@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 4a1c218..1354b8e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2,6 +2,7 @@
 #include "win32.h"
 #include <conio.h>
 #include "../strbuf.h"
+#include "../run-command.h"
 
 static const int delay[] = { 0, 1, 10, 20, 40 };
 
@@ -129,6 +130,74 @@ static inline int is_file_in_use_error(DWORD errcode)
 	return 0;
 }
 
+static int read_yes_no_answer(void)
+{
+	char answer[1024];
+
+	if (fgets(answer, sizeof(answer), stdin)) {
+		size_t answer_len = strlen(answer);
+		int got_full_line = 0, c;
+
+		/* remove the newline */
+		if (answer_len >= 2 && answer[answer_len-2] == '\r') {
+			answer[answer_len-2] = '\0';
+			got_full_line = 1;
+		}
+		else if (answer_len >= 1 && answer[answer_len-1] == '\n') {
+			answer[answer_len-1] = '\0';
+			got_full_line = 1;
+		}
+		/* flush the buffer in case we did not get the full line */
+		if (!got_full_line)
+			while((c = getchar()) != EOF && c != '\n');
+	} else
+		/* we could not read, return the
+		 * default answer which is no */
+		return 0;
+
+	if (tolower(answer[0]) == 'y' && !answer[1])
+		return 1;
+	if (!strncasecmp(answer, "yes", sizeof(answer)))
+		return 1;
+	if (tolower(answer[0]) == 'n' && !answer[1])
+		return 0;
+	if (!strncasecmp(answer, "no", sizeof(answer)))
+		return 0;
+
+	/* did not find an answer we understand */
+	return -1;
+}
+
+static int ask_yes_no_if_possible(const char *format, ...)
+{
+	char question[4096];
+	const char *retry_hook[] = { NULL, NULL, NULL };
+	va_list args;
+
+	va_start(args, format);
+	vsnprintf(question, sizeof(question), format, args);
+	va_end(args);
+
+	if ((retry_hook[0] = getenv("GIT_ASK_YESNO"))) {
+		retry_hook[1] = question;
+		return !run_command_v_opt(retry_hook, 0);
+	}
+
+	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
+		return 0;
+
+	while (1) {
+		int answer;
+		fprintf(stderr, "%s (y/n) ", question);
+
+		if ((answer = read_yes_no_answer()) >= 0)
+			return answer;
+
+		fprintf(stderr, "Sorry, I did not understand your answer. "
+				"Please type 'y' or 'n'\n");
+	}
+}
+
 #undef unlink
 int mingw_unlink(const char *pathname)
 {
@@ -149,6 +218,10 @@ int mingw_unlink(const char *pathname)
 		Sleep(delay[tries]);
 		tries++;
 	}
+	while (ret == -1 && is_file_in_use_error(GetLastError()) &&
+	       ask_yes_no_if_possible("Unlink of file '%s' failed. "
+			"Should I try again?", pathname))
+	       ret = unlink(pathname);
 	return ret;
 }
 
@@ -1326,6 +1399,11 @@ repeat:
 		tries++;
 		goto repeat;
 	}
+	if (gle == ERROR_ACCESS_DENIED &&
+	       ask_yes_no_if_possible("Rename from '%s' to '%s' failed. "
+		       "Should I try again?", pold, pnew))
+		goto repeat;
+
 	errno = EACCES;
 	return -1;
 }
-- 
1.7.4.34.gd2cb1

[PATCH v4 4/5] mingw: add fallback for rmdir in case directory is in use

[Heiko Voigt]

The same logic as for unlink and rename also applies to rmdir. For
example in case you have a shell open in a git controlled folder. This
will easily fail. So lets be nice for such cases as well.

Signed-off-by: Heiko Voigt <heiko.voigt@mahr.de>
---
 compat/mingw.c |   25 +++++++++++++++++++++++++
 compat/mingw.h |    3 +++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 1354b8e..68cc79f 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -225,6 +225,31 @@ int mingw_unlink(const char *pathname)
 	return ret;
 }
 
+#undef rmdir
+int mingw_rmdir(const char *pathname)
+{
+	int ret, tries = 0;
+
+	while ((ret = rmdir(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
+		if (!is_file_in_use_error(GetLastError()))
+			break;
+		/*
+		 * We assume that some other process had the source or
+		 * destination file open at the wrong moment and retry.
+		 * In order to give the other process a higher chance to
+		 * complete its operation, we give up our time slice now.
+		 * If we have to retry again, we do sleep a bit.
+		 */
+		Sleep(delay[tries]);
+		tries++;
+	}
+	while (ret == -1 && is_file_in_use_error(GetLastError()) &&
+	       ask_yes_no_if_possible("Deletion of directory '%s' failed. "
+			"Should I try again?", pathname))
+	       ret = rmdir(pathname);
+	return ret;
+}
+
 #undef open
 int mingw_open (const char *filename, int oflags, ...)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index 970d743..fe6ba34 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -169,6 +169,9 @@ int link(const char *oldpath, const char *newpath);
 int mingw_unlink(const char *pathname);
 #define unlink mingw_unlink
 
+int mingw_rmdir(const char *path);
+#define rmdir mingw_rmdir
+
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
-- 
1.7.4.34.gd2cb1

[PATCH v4 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Heiko Voigt]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, EACCES overrules ENOTEMPTY when calling rmdir(). But if the
directory is busy, we only want to retry deleting the directory if it
is empty, so test specifically for that case and set ENOTEMPTY rather
than EACCES.

Noticed by Greg Hazel.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 68cc79f..368edf2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -225,6 +225,30 @@ int mingw_unlink(const char *pathname)
 	return ret;
 }
 
+static int is_dir_empty(const char *path)
+{
+	struct strbuf buf = STRBUF_INIT;
+	WIN32_FIND_DATAA findbuf;
+	HANDLE handle;
+
+	strbuf_addf(&buf, "%s\\*", path);
+	handle = FindFirstFileA(buf.buf, &findbuf);
+	if (handle == INVALID_HANDLE_VALUE) {
+		strbuf_release(&buf);
+		return GetLastError() == ERROR_NO_MORE_FILES;
+	}
+
+	while (!strcmp(findbuf.cFileName, ".") ||
+			!strcmp(findbuf.cFileName, ".."))
+		if (!FindNextFile(handle, &findbuf)) {
+			strbuf_release(&buf);
+			return GetLastError() == ERROR_NO_MORE_FILES;
+		}
+	FindClose(handle);
+	strbuf_release(&buf);
+	return 0;
+}
+
 #undef rmdir
 int mingw_rmdir(const char *pathname)
 {
@@ -233,6 +257,10 @@ int mingw_rmdir(const char *pathname)
 	while ((ret = rmdir(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
 		if (!is_file_in_use_error(GetLastError()))
 			break;
+		if (!is_dir_empty(pathname)) {
+			errno = ENOTEMPTY;
+			break;
+		}
 		/*
 		 * We assume that some other process had the source or
 		 * destination file open at the wrong moment and retry.
-- 
1.7.4.34.gd2cb1

Re: [PATCH v4 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Erik Faye-Lund]

On Mon, Feb 7, 2011 at 9:54 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> On Windows, EACCES overrules ENOTEMPTY when calling rmdir(). But if the
> directory is busy, we only want to retry deleting the directory if it
> is empty, so test specifically for that case and set ENOTEMPTY rather
> than EACCES.
>

I'm sorry, I don't quite understand. rmdir on Windows/MinGW fails with
errno=ENOTEMPTY if a directory isn't empty:
---8<---
#include <errno.h>
#include <stdlib.h>
#include <stdio.h>

int main()
{
	if (mkdir("foo") < 0 && errno != EEXIST) {
		perror("mkdir");
		exit(-1);
	}

	if (!fopen("foo/bar", "w")) {
		perror("fopen");
		exit(-1);
	}

	if (rmdir("foo") < 0)
		perror("rmdir");
	printf("errno: %d (expected %d)\n", errno, ENOTEMPTY);
}
---8<---
$ ./a.exe
rmdir: Directory not empty
errno: 41 (expected 41)

Re: [msysGit] Re: [PATCH v4 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Heiko Voigt]

On Mon, Feb 07, 2011 at 10:07:10PM +0100, Erik Faye-Lund wrote:
> On Mon, Feb 7, 2011 at 9:54 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > On Windows, EACCES overrules ENOTEMPTY when calling rmdir(). But if the
> > directory is busy, we only want to retry deleting the directory if it
> > is empty, so test specifically for that case and set ENOTEMPTY rather
> > than EACCES.
> >
> 
> I'm sorry, I don't quite understand. rmdir on Windows/MinGW fails with
> errno=ENOTEMPTY if a directory isn't empty:

I think Johannes was referring to the case when a directory is busy.
E.g. a process is running that has its working directory inside that
directory. In that case ENOTEMPTY was not returned even though the
directory is not empty. Thats what I read from the patch.

Cheers Heiko

Re: [msysGit] Re: [PATCH v4 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Erik Faye-Lund]

On Mon, Feb 7, 2011 at 10:18 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Mon, Feb 07, 2011 at 10:07:10PM +0100, Erik Faye-Lund wrote:
>> On Mon, Feb 7, 2011 at 9:54 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> >
>> > On Windows, EACCES overrules ENOTEMPTY when calling rmdir(). But if the
>> > directory is busy, we only want to retry deleting the directory if it
>> > is empty, so test specifically for that case and set ENOTEMPTY rather
>> > than EACCES.
>> >
>>
>> I'm sorry, I don't quite understand. rmdir on Windows/MinGW fails with
>> errno=ENOTEMPTY if a directory isn't empty:
>
> I think Johannes was referring to the case when a directory is busy.
> E.g. a process is running that has its working directory inside that
> directory. In that case ENOTEMPTY was not returned even though the
> directory is not empty. Thats what I read from the patch.
>

I don't think that's the case either:
$ echo "int main() { while (1); }" | gcc -x c - -o foo/bin.exe
[kusma@HUE-PC:/git@work/xgetenv]
$ foo/bin.exe &
[2] 3188
[kusma@HUE-PC:/git@work/xgetenv]
$ ./a.exe
rmdir: Directory not empty
errno: 41 (expected 41)

Re: [msysGit] Re: [PATCH v4 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Junio C Hamano]

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

> On Mon, Feb 7, 2011 at 10:18 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> ...
>> I think Johannes was referring to the case when a directory is busy.
>> E.g. a process is running that has its working directory inside that
>> directory. In that case ENOTEMPTY was not returned even though the
>> directory is not empty. Thats what I read from the patch.
>
> I don't think that's the case either:
> $ echo "int main() { while (1); }" | gcc -x c - -o foo/bin.exe
> [kusma@HUE-PC:/git@work/xgetenv]
> $ foo/bin.exe &
> [2] 3188
> [kusma@HUE-PC:/git@work/xgetenv]
> $ ./a.exe
> rmdir: Directory not empty
> errno: 41 (expected 41)

I don't do windows, but I think Heiko is talking about running some
process inside the directory that is getting removed.  Assuming that your
a.exe is to remove the 'foo' directory, you would need to run ./bin.exe
after going into 'foo' directory, perhaps like this:

    $ (cd foo && ./bin.exe) &
    $ ./a.exe

Re: [msysGit] Re: [PATCH v4 5/5] mingw_rmdir: set errno=ENOTEMPTY when appropriate

[Erik Faye-Lund]

On Mon, Feb 7, 2011 at 10:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> On Mon, Feb 7, 2011 at 10:18 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>> ...
>>> I think Johannes was referring to the case when a directory is busy.
>>> E.g. a process is running that has its working directory inside that
>>> directory. In that case ENOTEMPTY was not returned even though the
>>> directory is not empty. Thats what I read from the patch.
>>
>> I don't think that's the case either:
>> $ echo "int main() { while (1); }" | gcc -x c - -o foo/bin.exe
>> [kusma@HUE-PC:/git@work/xgetenv]
>> $ foo/bin.exe &
>> [2] 3188
>> [kusma@HUE-PC:/git@work/xgetenv]
>> $ ./a.exe
>> rmdir: Directory not empty
>> errno: 41 (expected 41)
>
> I don't do windows, but I think Heiko is talking about running some
> process inside the directory that is getting removed.  Assuming that your
> a.exe is to remove the 'foo' directory, you would need to run ./bin.exe
> after going into 'foo' directory, perhaps like this:
>
>    $ (cd foo && ./bin.exe) &
>    $ ./a.exe

Of course, silly me:

$ ./a.exe
rmdir: Permission denied
errno: 13 (expected 41)

Thanks for clearing that up, now I'm on board :)

Re: [PATCH v4 0/5] make open/unlink failures user friendly on windows using retry/abort

[Junio C Hamano]

Thanks, will queue (with a handful of minor style fixes).