make open/unlink failures user friendly on windows using retry/abort

PATCH Superseded Heiko Voigt 4 participants 34 messages Jun 15, 2016

lore

Share & tools

lore

Heiko Voigt

author

2016-06-15 22:50 UTC #0 lore raw

v3 cover make open/unlink failures user friendly on windows using retry/abort

Hi,

Quoted 8 lines · Johannes Sixt

Quoted 5 lines · Heiko Voigt
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(-)

Signature · 1 line

1.7.3.3.566.gf422f

Heiko Voigt

author

2016-06-15 22:50 UTC #1 lore raw

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

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

Signed-off-by Heiko Voigt <[email protected]>

---
 compat/mingw.c |    8 ++++++++
 compat/mingw.h |   11 +++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

compat/mingw.c +8 -0

index bee6054..a7e1c6b 100644
@@ -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, ...)
 {

compat/mingw.h +3 -8

index 2283071..8316938 100644
@@ -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

Signature · 1 line

1.7.3.3.566.gf422f

Heiko Voigt

author

2016-06-15 22:50 UTC #2 lore raw

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

In reply to 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 <[email protected]>

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

compat/mingw.c +29 -2

index a7e1c6b..52183a7 100644
@@ -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.

Signature · 1 line

1.7.3.3.566.gf422f

Erik Faye-Lund

2016-06-15 22:50 UTC #3 lore raw

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

Quoted 23 lines · 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 <[email protected]>
---
compat/mingw.c | 31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)
compat/mingw.c +2 -0
index a7e1c6b..52183a7 100644
@@ -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;

compat/mingw.c int err_win_to_posix(DWORD winerr) · line 123

Quoted 6 lines

compat/mingw.c +3 -0

@@ -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?

Heiko Voigt

author

2016-06-15 22:50 UTC #4 lore raw

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

Quoted 7 lines · Erik Faye-Lund

compat/mingw.c int err_win_to_posix(DWORD winerr) · line 123
Quoted 3 lines · Heiko Voigt
compat/mingw.c +3 -0
+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

Heiko Voigt

author

2016-06-15 22:50 UTC #5 lore raw

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

Signed-off-by Heiko Voigt <[email protected]>

---
Here is a replacement for the previous patch.

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

compat/mingw.c +29 -2

index a7e1c6b..4a1c218 100644
@@ -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.

Signature · 1 line

1.7.3.3.566.g98577

Heiko Voigt

author

2016-06-15 22:50 UTC #6 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Albert Dvornik <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

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

compat/mingw.c +82 -0

index 52183a7..ac9fb4a 100644
@@ -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;
 }

Signature · 1 line

1.7.3.3.566.gf422f

Erik Faye-Lund

2016-06-15 22:50 UTC #7 lore raw

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

Quoted 51 lines · 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 <[email protected]>
Signed-off-by Albert Dvornik <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>
---
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(-)
compat/mingw.c +1 -0
index 52183a7..ac9fb4a 100644
@@ -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 };
 

compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 133

Quoted 4 lines

compat/mingw.c +1 -0

@@ -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?

Quoted 22 lines

compat/mingw.c +22 -0

+{
+       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 */

compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 165

Quoted 10 lines

compat/mingw.c +10 -0

+               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?

Quoted 8 lines

compat/mingw.c +8 -0

+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"))) {
+

compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 188

Quoted 10 lines

compat/mingw.c +10 -0

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

Junio C Hamano

2016-06-15 22:50 UTC #8 lore raw

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

Quoted 7 lines · Erik Faye-Lund

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:
(quoted diff) +1 -1
-       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 ;-)

Quoted 6 lines

compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 188
Quoted 2 lines
compat/mingw.c +2 -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.

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?

Heiko Voigt

author

2016-06-15 22:50 UTC #9 lore raw

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

Hi,

Quoted 8 lines · Junio C Hamano

Quoted 6 lines · Erik Faye-Lund
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:
(quoted diff) +1 -1
-       if (answer[0] == 'n' && strlen(answer) == 1)
+       if (tolower(answer[0]) == 'n' && strlen(answer) == 1)

Quoted 7 lines

Quoted 1 line
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?

Quoted 13 lines

Quoted 6 lines
compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 188
Quoted 2 lines
compat/mingw.c +2 -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.
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".

Quoted 5 lines

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.

Quoted 3 lines

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

Johannes Schindelin

2016-06-15 22:50 UTC #10 lore raw

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

In reply to Erik Faye-Lund

Hi,

Quoted 29 lines · Erik Faye-Lund

Quoted 28 lines · 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.

Quoted 6 lines

Quoted 6 lines
Signed-off-by Heiko Voigt <[email protected]>
Signed-off-by Albert Dvornik <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>
---
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.

Quoted 15 lines

compat/mingw.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++

 1 files changed, 82 insertions(+), 0 deletions(-)

compat/mingw.c +1 -0

index 52183a7..ac9fb4a 100644
@@ -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 };

Quoted 7 lines

compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 133
Quoted 5 lines
compat/mingw.c +1 -0
@@ -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.

Quoted 36 lines

Quoted 22 lines

compat/mingw.c +22 -0

+{
+       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 */

compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 165

Quoted 10 lines

compat/mingw.c +10 -0

+               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:

Quoted 4 lines

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

Makes sense to me.

Quoted 17 lines

compat/mingw.c +17 -0

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

Quoted 8 lines

compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 183
Quoted 1 line
compat/mingw.c +1 -0
+               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

Heiko Voigt

author

2016-06-15 22:50 UTC #11 lore raw

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

Hi,

Quoted 3 lines · Johannes Schindelin

Quoted 1 line · Erik Faye-Lund
On Tue, Dec 14, 2010 at 11:21 PM, Heiko Voigt <[email protected]> wrote:

[...]

Quoted 12 lines

Quoted 12 lines
Quoted 8 lines
+ 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:

Quoted 6 lines

Quoted 4 lines
- 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.

Quoted 1 line

Quoted 1 line
Quoted 1 line
+static int ask_user_yes_no(const char *format, ...)

[...]

Quoted 9 lines

Quoted 7 lines
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

Junio C Hamano

2016-06-15 22:50 UTC #12 lore raw

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

In reply to Johannes Schindelin

Quoted 9 lines · Johannes Schindelin

Quoted 7 lines
compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 133
Quoted 5 lines
compat/mingw.c +1 -0
@@ -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 ;-)

Quoted 2 lines

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.

Heiko Voigt

author

2016-06-15 22:50 UTC #13 lore raw

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

Quoted 15 lines · Junio C Hamano

Quoted 9 lines · Johannes Schindelin
Quoted 7 lines
compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 133
Quoted 5 lines
compat/mingw.c +1 -0
@@ -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 ;-)

Quoted 8 lines

Quoted 2 lines
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

Erik Faye-Lund

2016-06-15 22:50 UTC #14 lore raw

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

In reply to Junio C Hamano

Quoted 6 lines · Junio C Hamano

Quoted 4 lines · Johannes Schindelin
Quoted 4 lines
Quoted 4 lines
compat/mingw.c +0 -0
@@ -129,6 +130,78 @@ static inline int is_file_in_use_error(DWORD errcode)
        return 0;
  }
 

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
compat/mingw.c static inline int is_file_in_use_error(DWORD errcode) · line 133
Quoted 1 line
compat/mingw.c +1 -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?

Heiko Voigt

author

2016-06-15 22:50 UTC #15 lore raw

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

In reply to 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 <[email protected]>

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

compat/mingw.c +25 -0

index ac9fb4a..b920644 100644
@@ -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, ...)
 {

compat/mingw.h +3 -0

index 8316938..8b159c4 100644
@@ -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

Signature · 1 line

1.7.3.3.566.gf422f

Heiko Voigt

author

2016-06-15 22:50 UTC #16 lore raw

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

In reply to Heiko Voigt

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

Signed-off-by Heiko Voigt <[email protected]>

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

compat/mingw.c +28 -0

index b920644..3e013c6 100644
@@ -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.

Signature · 1 line

1.7.3.3.566.gf422f

Erik Faye-Lund

2016-06-15 22:50 UTC #17 lore raw

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

Quoted 7 lines · Heiko Voigt

From: Johannes Schindelin <[email protected]>
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

Johannes Schindelin

2016-06-15 22:50 UTC #18 lore raw

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

Hi,

Quoted 9 lines · Erik Faye-Lund

Quoted 7 lines · Heiko Voigt
From: Johannes Schindelin <[email protected]>
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.

Quoted 4 lines

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

Erik Faye-Lund

2016-06-15 22:50 UTC #19 lore raw

In reply to Heiko Voigt

Quoted 29 lines · Heiko Voigt

Hi,
Quoted 8 lines · Johannes Sixt
Quoted 5 lines · Heiko Voigt
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

Quoted 7 lines

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

Junio C Hamano

2016-06-15 22:50 UTC #20 lore raw

Quoted 3 lines · Erik Faye-Lund

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.

Heiko Voigt

author

2016-06-15 22:50 UTC #21 lore raw

v4 cover make open/unlink failures user friendly on windows using retry/abort

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

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · Erik Faye-Lund
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)?

Quoted 7 lines

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

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

Signature · 1 line

1.7.4.34.gd2cb1

Heiko Voigt

author

2016-06-15 22:50 UTC #22 lore raw

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

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

Signed-off-by Heiko Voigt <[email protected]>

---
 compat/mingw.c |    8 ++++++++
 compat/mingw.h |   11 +++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

compat/mingw.c +8 -0

index bee6054..a7e1c6b 100644
@@ -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, ...)
 {

compat/mingw.h +3 -8

index cafc1eb..970d743 100644
@@ -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

Signature · 1 line

1.7.4.34.gd2cb1

Heiko Voigt

author

2016-06-15 22:50 UTC #24 lore raw

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

In reply to Heiko Voigt

Signed-off-by Heiko Voigt <[email protected]>

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

compat/mingw.c +29 -2

index a7e1c6b..4a1c218 100644
@@ -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.

Signature · 1 line

1.7.4.34.gd2cb1

Heiko Voigt

author

2016-06-15 22:50 UTC #25 lore raw

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

In reply to 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:

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.

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 <[email protected]>

Signed-off-by Albert Dvornik <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

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

compat/mingw.c +78 -0

index 4a1c218..1354b8e 100644
@@ -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;
 }

Signature · 1 line

1.7.4.34.gd2cb1

Heiko Voigt

author

2016-06-15 22:50 UTC #26 lore raw

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

In reply to 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 <[email protected]>

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

compat/mingw.c +25 -0

index 1354b8e..68cc79f 100644
@@ -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, ...)
 {

compat/mingw.h +3 -0

index 970d743..fe6ba34 100644
@@ -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

Signature · 1 line

1.7.4.34.gd2cb1

Heiko Voigt

author

2016-06-15 22:50 UTC #27 lore raw

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

In reply to Heiko Voigt

From: Johannes Schindelin <[email protected]>

Noticed by Greg Hazel.

Signed-off-by Johannes Schindelin <[email protected]>

Signed-off-by Heiko Voigt <[email protected]>

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

compat/mingw.c +28 -0

index 68cc79f..368edf2 100644
@@ -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.

Signature · 1 line

1.7.4.34.gd2cb1

Erik Faye-Lund

2016-06-15 22:50 UTC #28 lore raw

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

Quoted 7 lines · Heiko Voigt

From: Johannes Schindelin <[email protected]>
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)

Heiko Voigt

author

2016-06-15 22:50 UTC #29 lore raw

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

Quoted 11 lines · Erik Faye-Lund

Quoted 7 lines · Heiko Voigt
From: Johannes Schindelin <[email protected]>
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

Erik Faye-Lund

2016-06-15 22:50 UTC #30 lore raw

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

Quoted 10 lines · Heiko Voigt

Quoted 9 lines · Erik Faye-Lund
Quoted 7 lines · Heiko Voigt
From: Johannes Schindelin <[email protected]>
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.

Quoted 8 lines

Quoted 2 lines
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)

Junio C Hamano

2016-06-15 22:50 UTC #31 lore raw

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

Quoted 8 lines · Erik Faye-Lund

On Mon, Feb 7, 2011 at 10:18 PM, Heiko Voigt <[email protected]> wrote:
...
Quoted 4 lines
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:

Quoted 8 lines

$ 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

Erik Faye-Lund

2016-06-15 22:50 UTC #32 lore raw

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

Quoted 19 lines · Junio C Hamano

Quoted 8 lines · Erik Faye-Lund
On Mon, Feb 7, 2011 at 10:18 PM, Heiko Voigt <[email protected]> wrote:
...
Quoted 4 lines
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:
Quoted 8 lines
$ 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)

Quoted 7 lines

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

Junio C Hamano

2016-06-15 22:50 UTC #33 lore raw

In reply to Heiko Voigt

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

Johannes Schindelin

2016-06-15 22:50 UTC #23 lore raw

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

In reply to Heiko Voigt

Hi,

Quoted 3 lines · Heiko Voigt

The next patch implements a workaround in case unlink fails on Windows.
Signed-off-by Heiko Voigt <[email protected]>

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

Thanks!
Dscho