Pull request for msysGit patches

Quoted 6 lines · Johannes Sixt

Quoted 1 line
mingw: do not crash on open(NULL, ...)
This one is bogus, and as it stands, it must have my Ack removed. :) Needs the same fix in mingw_fopen as mingw_freopen. (There remains an unprotected dereference of filename.)

I believe the version in for-junio already has this fix squashed in. The following hunk, taken from
http://repo.or.cz/w/git/mingw/4msysgit.git/blobdiff/4e93566b07dcf47ecb6484d225418c04c1eedee6..b18500977d88b13803ecc60cf383538139ec09d8:/compat/mingw.c
shows that it is... Or are you thinking of something else?

@@ -346,7 +346,7 @@ FILE *mingw_fopen (const char *filename, const char *otype)
if (hide_dotfiles == HIDE_DOTFILES_TRUE && basename((char*)filename)[0] == '.')
hide = access(filename, F_OK);
- if (!strcmp(filename, "/dev/null"))
+ if (filename && !strcmp(filename, "/dev/null"))
filename = "nul";
file = fopen(filename, otype);
if (file && hide && make_hidden(filename))

Johannes Sixt

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

Quoted 14 lines · Erik Faye-Lund

Quoted 5 lines · Johannes Sixt
Quoted 1 line
mingw: do not crash on open(NULL, ...)
This one is bogus, and as it stands, it must have my Ack removed. :) Needs the same fix in mingw_fopen as mingw_freopen. (There remains an unprotected dereference of filename.)
I believe the version in for-junio already has this fix squashed in. The following hunk, taken from
http://repo.or.cz/w/git/mingw/4msysgit.git/blobdiff/4e93566b07dcf47ecb6484d
225418c04c1eedee6..b18500977d88b13803ecc60cf383538139ec09d8:/compat/mingw.c shows that it is... Or are you thinking of something else?

Review FILE *mingw_fopen (const char *filename, const char · line 347

Quoted 2 lines

(quoted diff) +0 -0

@@ -346,7 +346,7 @@ FILE *mingw_fopen (const char *filename, const char
 	if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
 	basename((char*)filename)[0] == '.')

                        ^^^^^^^^

This can crash, too.

Review FILE *mingw_fopen (const char *filename, const char · line 352

Quoted 6 lines

(quoted diff) +1 -1

 		hide = access(filename, F_OK);
-	if (!strcmp(filename, "/dev/null"))
+	if (filename && !strcmp(filename, "/dev/null"))
 		filename = "nul";
 	file = fopen(filename, otype);
 	if (file && hide && make_hidden(filename))

-- Hannes

Erik Faye-Lund

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

Quoted 14 lines · Johannes Sixt

Quoted 13 lines · Erik Faye-Lund
Quoted 5 lines · Johannes Sixt
Quoted 1 line
mingw: do not crash on open(NULL, ...)
This one is bogus, and as it stands, it must have my Ack removed. :) Needs the same fix in mingw_fopen as mingw_freopen. (There remains an unprotected dereference of filename.)
I believe the version in for-junio already has this fix squashed in. The following hunk, taken from
http://repo.or.cz/w/git/mingw/4msysgit.git/blobdiff/4e93566b07dcf47ecb6484d
225418c04c1eedee6..b18500977d88b13803ecc60cf383538139ec09d8:/compat/mingw.c shows that it is... Or are you thinking of something else?

Quoted 6 lines

Quoted 3 lines
(quoted diff) +0 -0
@@ -346,7 +346,7 @@ FILE *mingw_fopen (const char *filename, const char
if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
basename((char*)filename)[0] == '.')
^^^^^^^^
This can crash, too.

Ah, indeed. Thanks for pointing that out! I'll send out a new version, with this squashed on top:

compat/mingw.c +0 -0

index 2fbe381..2584e9f 100644
@@ -343,11 +343,13 @@ FILE *mingw_fopen (const char *filename, const

char *otype)
{
int hide = 0;
FILE *file;
- if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
- basename((char*)filename)[0] == '.')
- hide = access(filename, F_OK);
- if (filename && !strcmp(filename, "/dev/null"))
- filename = "nul";
+ if (filename) {
+ if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
+ basename((char*)filename)[0] == '.')
+ hide = access(filename, F_OK);
+ if (!strcmp(filename, "/dev/null"))
+ filename = "nul";
+ }
file = fopen(filename, otype);
if (file && hide && make_hidden(filename))
warning("Could not mark '%s' as hidden.", filename);
@@ -359,11 +361,13 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream)
{
int hide = 0;
FILE *file;
- if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
- basename((char*)filename)[0] == '.')
- hide = access(filename, F_OK);
- if (filename && !strcmp(filename, "/dev/null"))
- filename = "nul";
+ if (filename) {
+ if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
+ basename((char*)filename)[0] == '.')
+ hide = access(filename, F_OK);
+ if (!strcmp(filename, "/dev/null"))
+ filename = "nul";
+ }
file = freopen(filename, otype, stream);
if (file && hide && make_hidden(filename))
warning("Could not mark '%s' as hidden.", filename);

Erik Faye-Lund

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

Quoted 22 lines · Erik Faye-Lund

Quoted 14 lines · Johannes Sixt
Quoted 13 lines · Erik Faye-Lund
Quoted 5 lines · Johannes Sixt
Quoted 1 line
mingw: do not crash on open(NULL, ...)
This one is bogus, and as it stands, it must have my Ack removed. :) Needs the same fix in mingw_fopen as mingw_freopen. (There remains an unprotected dereference of filename.)
I believe the version in for-junio already has this fix squashed in. The following hunk, taken from
http://repo.or.cz/w/git/mingw/4msysgit.git/blobdiff/4e93566b07dcf47ecb6484d
225418c04c1eedee6..b18500977d88b13803ecc60cf383538139ec09d8:/compat/mingw.c shows that it is... Or are you thinking of something else?
Quoted 6 lines
Quoted 3 lines
(quoted diff) +0 -0
@@ -346,7 +346,7 @@ FILE *mingw_fopen (const char *filename, const char
if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
basename((char*)filename)[0] == '.')
^^^^^^^^
This can crash, too.

Quoted 3 lines

Ah, indeed. Thanks for pointing that out! I'll send out a new version, with this squashed on top:

And now that I assemble it, I all of a sudden understand where your complaint came from. The issue wasn't in the original patch, it was introduced when the patch was applied.

Basically, I introduced a similar crash-bug to the one this patch fixes in the original hide-dotfiles patch, and my this patch was written against junio's master, which doesn't contain that newer bug.

I also seem to remember there being some controversy over the hide-dotfiles series, whether or not it was in git's scope to allow hiding non-dotgit files at all etc. So, perhaps what makes the most sense would be to eject the hide-dotfiles stuff out of for-junio, and rework it on top, going through the main git mailing list? That series did get quite a bit messy as patches were applied a little too quickly IMO.

Eric Sunshine

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

Hi Hannes,

Quoted 10 lines · Johannes Sixt

Quoted 4 lines · Pat Thoyts
Junio,
The msysGit tree currently tracks some 50+ patches on top of 'next'. I have gathered 42 of these that look good to move upstream.
I've browsed through the list, and I'll annotate my opinion below. When I say 'OK', then this means that the patch looks good, but it doesn't imply that I have tested it.

Quoted 5 lines

Quoted 1 line
Side-step line-ending corruption leading to t3032 failures.
This one has non-portable 'export foo=bar', but it is in a MinGW specific path, so it should be fine. But why do we need to export GREP_OPTIONS, but not SED_OPTIONS?

I also normally avoid unportable 'export foo=bar'. In the particular case of GREP_OPTIONS, when commenting on my original patch submission, Dscho suggested 'test_have_prereq MINGW && export GREP_OPTIONS=foo' so that is the form which made it into the final patch. While considering whether this unportable usage was worthwhile, I took into consideration the fact that a much more senior project member preferred it, that the MinGW-specific build environment is Bash-based, and that a couple other test scripts (t1509, t5560) also employ this form. If preferred, the patches can be re-submitted to avoid the unportable usage.

Regarding exporting GREP_OPTIONS but not SED_OPTIONS: grep explicitly recognizes the GREP_OPTIONS environment variable. My original patch did not export GREP_OPTIONS, but instead referenced it as $GREP_OPTIONS in the grep invocation. Upon review, Dscho preferred the variable to be exported rather than referenced manually as $GREP_OPTIONS. sed, on the other hand, does not recognize any 'options' environment variable. It is necessary, therefore, to interpolate $SED_OPTIONS directly during the sed invocation. This variable is therefore local to the script and need not be exported.

-- ES

Junio C Hamano

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

Quoted 4 lines · Eric Sunshine

I also normally avoid unportable 'export foo=bar'. In the particular case of GREP_OPTIONS, when commenting on my original patch submission, Dscho suggested 'test_have_prereq MINGW && export GREP_OPTIONS=foo' so that is the form which made it into the final patch.

Well, since bbc09c2 (grep: rip out support for external grep, 2010-01-12) we do not run external grep at all, so GREP_OPTIONS is irrelevant.

Unless you are planning to run tests on installed version of git older than v1.7.0, that is ;-).

Eric Sunshine

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

Quoted 11 lines · Junio C Hamano

Quoted 4 lines · Eric Sunshine
I also normally avoid unportable 'export foo=bar'. In the particular case of GREP_OPTIONS, when commenting on my original patch submission, Dscho suggested 'test_have_prereq MINGW&& export GREP_OPTIONS=foo' so that is the form which made it into the final patch.
Well, since bbc09c2 (grep: rip out support for external grep, 2010-01-12) we do not run external grep at all, so GREP_OPTIONS is irrelevant.
Unless you are planning to run tests on installed version of git older than v1.7.0, that is ;-).

The patches in question involve test scripts which themselves invoke external grep for various reasons. An example is t5560 where external grep is invoked to extract the Status: header from an HTTP response. The patches fix instances where grep invocations give incorrect results on Windows due to grep undesirably swallowing CR from CRLF line-terminators in the few tests where those terminators are actually significant (such as t5560). In the context of these test scripts, external grep is still employed, so GREP_OPTIONS may still be relevant.

-- ES

Pat Thoyts

author

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

PATCH 1/1 git-am: fix detection of absolute paths for windows

Add an is_absolute_path function to abstract out platform differences in checking for an absolute or relative path.
Specifically fixes t4150-am on Windows.

Signed-off-by Pat Thoyts <[email protected]>

---

In response to the following comment...

Quoted 6 lines

Quoted 5 lines · Johannes Sixt
Quoted 1 line
git-am: fix absolute path logic on Windows
This mistakes a file that has a colon in the second position as absolute, even on non-Windows. IIUC, this patch is not intended for upstream git.

This patch is an alternative solution that only tries the Windows paths on msysGit. This is_absolute_path might find use elsewhere in the future so I elected to place it in the sh-setup script. Also added support for identifying a UNC path as absolute.

 git-am.sh       |   12 ++++++------
 git-sh-setup.sh |   15 +++++++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

git-am.sh +6 -6

index e7f008c..9317b38 100755
@@ -444,12 +444,12 @@ else
 				set x
 				first=
 			}
-			case "$arg" in
-			/*)
-				set "$@" "$arg" ;;
-			*)
-				set "$@" "$prefix$arg" ;;
-			esac
+			if is_absolute_path "$arg"
+			then
+				set "$@" "$arg"
+			else
+				set "$@" "$prefix$arg"
+			fi
 		done
 		shift
 	fi

git-sh-setup.sh +15 -0

index 6131670..b9eb0bf 100644
@@ -209,5 +209,20 @@ case $(uname -s) in
 	find () {
 		/usr/bin/find "$@"
 	}
+	is_absolute_path () {
+		case "$1" in
+		/* | ?:* | \\\\*)
+			return 0 ;;
+		esac
+		return 1
+	}
 	;;
+*)
+	is_absolute_path () {
+		case "$1" in
+		/*)
+			return 0 ;;
+		esac
+		return 1
+	}
 esac

Signature · 1 line

1.7.3

Johannes Sixt

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

Re: [PATCH] git-am: fix detection of absolute paths for windows

Quoted 3 lines · Pat Thoyts

Add an is_absolute_path function to abstract out platform differences in checking for an absolute or relative path.
Specifically fixes t4150-am on Windows.

Thanks for tackling this!

git-sh-setup.sh case $(uname -s) in · line 214

Quoted 7 lines

git-sh-setup.sh +3 -0

@@ -209,5 +209,20 @@ case $(uname -s) in
 	find () {
 		/usr/bin/find "$@"
 	}
+	is_absolute_path () {
+		case "$1" in
+		/* | ?:* | \\\\*)

Absolute paths can also start with a backslash, and UNC paths can start with double-slash. Therefore, this should be:

		[/\\]* | [A-Za-z]:*)

git-sh-setup.sh case $(uname -s) in · line 218

Quoted 4 lines

git-sh-setup.sh +4 -0

+			return 0 ;;
+		esac
+		return 1
+	}

-- Hannes

Pat Thoyts

author

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

PATCH 1/1 git-am: fix detection of absolute paths for windows

Add an is_absolute_path function to abstract out platform differences in checking for an absolute or relative path.
Specifically fixes t4150-am on Windows.

[PT: updated following suggestion from j6t to support \* and //*]

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

Signed-off-by Pat Thoyts <[email protected]>

---

Quoted 18 lines · Johannes Sixt

Quoted 3 lines · Pat Thoyts
Add an is_absolute_path function to abstract out platform differences in checking for an absolute or relative path.
Specifically fixes t4150-am on Windows.
Thanks for tackling this!
Review case $(uname -s) in · line 214
Quoted 7 lines
(quoted diff) +3 -0
@@ -209,5 +209,20 @@ case $(uname -s) in
 	find () {
 		/usr/bin/find "$@"
 	}
+	is_absolute_path () {
+		case "$1" in
+		/* | ?:* | \\\\*)
Absolute paths can also start with a backslash, and UNC paths can start with double-slash. Therefore, this should be:

Quoted 8 lines

		[/\\]* | [A-Za-z]:*)
Review case $(uname -s) in · line 218
Quoted 4 lines
(quoted diff) +4 -0
+			return 0 ;;
+		esac
+		return 1
+	}
-- Hannes

I've modified the patch and added your signoff - hopefully that is ok?

 git-am.sh       |   12 ++++++------
 git-sh-setup.sh |   15 +++++++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

git-am.sh +6 -6

index e7f008c..9317b38 100755
@@ -444,12 +444,12 @@ else
 				set x
 				first=
 			}
-			case "$arg" in
-			/*)
-				set "$@" "$arg" ;;
-			*)
-				set "$@" "$prefix$arg" ;;
-			esac
+			if is_absolute_path "$arg"
+			then
+				set "$@" "$arg"
+			else
+				set "$@" "$prefix$arg"
+			fi
 		done
 		shift
 	fi

git-sh-setup.sh +15 -0

index 6131670..58d30c9 100644
@@ -209,5 +209,20 @@ case $(uname -s) in
 	find () {
 		/usr/bin/find "$@"
 	}
+	is_absolute_path () {
+		case "$1" in
+		[/\\]* | [A-Za-z]:*)
+			return 0 ;;
+		esac
+		return 1
+	}
 	;;
+*)
+	is_absolute_path () {
+		case "$1" in
+		/*)
+			return 0 ;;
+		esac
+		return 1
+	}
 esac

Signature · 1 line

1.7.3

Heiko Voigt

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

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

http://msdn.microsoft.com/en-us/library/ms681382%28v=VS.85%29.aspx

Hi,

here is a new iteration of my original patch series. This series replaces the newest patches currently present in 4msysgit.git's master and does not cleanly apply to git.git's master. Once everybody is happy with the outcome I will port it to git.git so msysgit can fetch it from upstream.

Quoted 7 lines · Johannes Sixt

Quoted 2 lines
Heiko Voigt (4):
mingw: work around irregular failures of unlink on windows
The workaround is to retry the unlink() after a delay when it failed with EACCES. What happens if the EACCES is for a good reason? Doesn't this delay the process by 71ms per unlink() invocation? Can't this become a problem if many unlink()s are tried by git code?

I have changed the triggering error code to be ERROR_SHARING_VIOLATION which seems to be the appropriate code for files that are in use by another process.

Thus we do not need to worry about whether we are retrying on a valid access error. I have tested this on my windows box and it works here.

j6t: I have not changed your error code in mingw_rename since you explicitely compare with the windows error code ERROR_ACCESS_DENIED and do not use the err_win_to_posix() function. Did you do this on purpose or should I also refer to ERROR_SHARING_VIOLATION ?

Quoted 8 lines

Quoted 1 line
mingw: make failures to unlink or move raise a question
Gaah! But people seem to like it. Since the question is only triggered after all retries fail, I can live with this.
But isn't the implementation a bit sloppy? Can strlen(answer)-2 be negative? What happens if the user typed more than 4 characters? Wouldn't it leave data in the buffer for the next question?

I have extracted reading of the answer into its own function and made the reading more robust which should now take care of the above issues.

Quoted 3 lines

Quoted 1 line
mingw: add fallback for rmdir in case directory is in use
Depends on the previous patch. OK.

No changes.

During pick up of the series I had to gather my testing script which can only be used manually for testing. Is there any place in git.git where we can store such "manual testing tools" ?

Cheers Heiko

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

Signature · 1 line

1.7.2.2.177.geec0d

Heiko Voigt

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

PATCH v2 1/4 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 98163da..49c594f 100644
@@ -155,6 +155,14 @@ int mingw_mkdir(const char *path, int mode)
 	return ret;
 }
 
+#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 56e58ba..220ae90 100644
@@ -119,14 +119,6 @@ static inline int fcntl(int fd, int cmd, ...)
 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
-
 static inline pid_t waitpid(pid_t pid, int *status, unsigned options)
 {
 	if (options == 0)

@@ -177,6 +169,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.2.2.177.geec0d

Johannes Sixt

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

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

[1] http://groups.google.com/group/msysgit/msg/fcad5f1455b263ca

Quoted 4 lines · Heiko Voigt

j6t: I have not changed your error code in mingw_rename since you explicitely compare with the windows error code ERROR_ACCESS_DENIED and do not use the err_win_to_posix() function. Did you do this on purpose or should I also refer to ERROR_SHARING_VIOLATION ?

IIRC, I dumped the error code before I implemented the retry logic, and the error was ERROR_ACCESS_DENIED. Did you verify that the occasions where you want to retry the operations indeed fail due to ERROR_SHARING_VIOLATION, or do you trust the documentation?

In any case, it is better to check for the Windows error code (which ever you finally choose) rather than errno, IMO.

-- Hannes

Heiko Voigt

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

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

Hi,

Quoted 10 lines · Johannes Sixt

Quoted 4 lines · Heiko Voigt
j6t: I have not changed your error code in mingw_rename since you explicitely compare with the windows error code ERROR_ACCESS_DENIED and do not use the err_win_to_posix() function. Did you do this on purpose or should I also refer to ERROR_SHARING_VIOLATION ?
IIRC, I dumped the error code before I implemented the retry logic, and the error was ERROR_ACCESS_DENIED. Did you verify that the occasions where you want to retry the operations indeed fail due to ERROR_SHARING_VIOLATION, or do you trust the documentation?

I tested this for my use case which is described by the manual testing script[1]. I ran it once to create the test structure and then from inside the test repository

	./../block_read 5000 file

and manually checked out the other branch in parallel. In this situation git will stop with the retry question. But I am not sure which use case you where addressing. Do you have a script or some information to reproduce?

Quoted 2 lines

In any case, it is better to check for the Windows error code (which ever you finally choose) rather than errno, IMO.

I agree because that should give us more fine grained control and since in err_win_to_posix() many codes are mapped to EACCES.

Cheers Heiko

Johannes Sixt

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

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

Quoted 9 lines · Heiko Voigt

I tested this for my use case which is described by the manual testing script[1]. I ran it once to create the test structure and then from inside the test repository
	./../block_read 5000 file
and manually checked out the other branch in parallel. In this situation git will stop with the retry question. But I am not sure which use case you where addressing. Do you have a script or some information to reproduce?

Sorry, I don't have a reliable test case. I usually encountered this problem in the test suite at random places when it was run with 'make -j2' and in a production repository sometimes during rebase --interactive.

-- Hannes

Heiko Voigt

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

PATCH v2 2/4 mingw: work around irregular failures of unlink on windows

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 |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

compat/mingw.c +24 -2

index 49c594f..bf9ffb8 100644
@@ -5,6 +5,7 @@
 #include "../cache.h"
 
 unsigned int _CRT_fmode = _O_BINARY;
+static const int delay[] = { 0, 1, 10, 20, 40 };
 
 int err_win_to_posix(DWORD winerr)
 {

@@ -119,6 +120,13 @@ int err_win_to_posix(DWORD winerr)
 	return error;
 }
 
+static inline int is_file_in_use_error()
+{
+	if (GetLastError() == ERROR_SHARING_VIOLATION)
+		return 1;
+	return 0;
+}
+
 static int make_hidden(const char *path)
 {
 	DWORD attribs = GetFileAttributes(path);

@@ -158,9 +166,24 @@ int mingw_mkdir(const char *path, int mode)
 #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())
+			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

@@ -1279,7 +1302,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.2.2.177.geec0d

Heiko Voigt

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

PATCH v2 3/4 mingw: make failures to unlink or move raise a question

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.

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

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

compat/mingw.c +82 -0

index bf9ffb8..b66bf89 100644
@@ -3,6 +3,7 @@
 #include <conio.h>
 #include "../strbuf.h"
 #include "../cache.h"
+#include "../run-command.h"
 
 unsigned int _CRT_fmode = _O_BINARY;
 static const int delay[] = { 0, 1, 10, 20, 40 };

@@ -163,6 +164,78 @@ int mingw_mkdir(const char *path, int mode)
 	return ret;
 }
 
+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)))
+		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)
 {

@@ -183,6 +256,10 @@ int mingw_unlink(const char *pathname)
 		Sleep(delay[tries]);
 		tries++;
 	}
+	while (ret == -1 && is_file_in_use_error() &&
+	       ask_user_yes_no("Unlink of file '%s' failed. "
+			"Should I try again?", pathname))
+	       ret = unlink(pathname);
 	return ret;
 }

@@ -1343,6 +1420,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.2.2.177.geec0d

Heiko Voigt

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

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

From: Heiko Voigt <[email protected]>

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 b66bf89..8e4f1e2 100644
@@ -263,6 +263,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())
+			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() &&
+	       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 220ae90..cf69d39 100644
@@ -172,6 +172,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.2.2.177.geec0d

Junio C Hamano

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

Quoted 8 lines · Pat Thoyts

Junio,
The msysGit tree currently tracks some 50+ patches on top of 'next'. I have gathered 42 of these that look good to move upstream. Please pull from
git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
also visible for inspection at
http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-junio

Sorry, I cannot pull anything based directly on top of 'next'. However, except for the one that touch t/t3032-merge-recursive-options.sh at the tip, the series seem to apply cleanly to the tip of 'master'.

There seem to be many patches that touch outside compat/ area. Have they been reviewed and discussed here already?

A quick and superficial review follows.

----------------------------------------------------------------
abspath.c

@@ -108,10 +108,15 @@ const char *make_nonrelative_path(const char *path)
if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
die("Too long path: %.*s", 60, path);
} else {
+ size_t len;
+ const char *fmt;
const char *cwd = get_pwd_cwd();
if (!cwd)
die_errno("Cannot determine the current working directory");
- if (snprintf(buf, PATH_MAX, "%s/%s", cwd, path) >= PATH_MAX)
+ len = strlen(cwd);
+ /* For cwd c:/, return c:/foo rather than URL-like c://foo */

For the patch to be regression free, the logic described by this comment requires get_pwd_cmd() to return a string with trailing dir-sep only at slash. IOW, if you see any non-root path returned with a trailing dir-sep for whatever reason, you are changing the behaviour in that case as well, and that clearly is not "fix at the root level".

But if you label this as "avoid duplicated dir-sep", everything flows smoothly ;-).

+ fmt = len > 0 && is_dir_sep(cwd[len-1]) ? "%s%s" : "%s/%s";

Please have () around "len > 0 && is_dir_sep(cwd[len-1])" for readability.

+ if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
die("Too long path: %.*s", 60, path);

----------------------------------------------------------------
get_home_directory()

The patch looks fine, but the commit log message is way insufficient. Can you restate what problem it addresses, and why it is the best solution?

----------------------------------------------------------------
Hide dotfiles

It is somewhat unfortunate that this Windows-only hack needs to touch cache.h and environment.c.

hidedotfiles may currently be the only platform specific configuration variable, but we might want to futureproof by abstracting this out, perhaps by adding a call to platform_core_config() at the end of git_default_core_config(), provide a default implementation that is a no-op, and allow platforms to override it, or something like that?

----------------------------------------------------------------
pack-objects.c usage string
connect.c use of unchecked git_getpass()

Good eyes, but these should not be part of the series but applied to maint. If possible please send them separately.

----------------------------------------------------------------
compat/regex/regexec.c

Thanks for fixing up my mess ;-)

----------------------------------------------------------------
git-am.sh

This is questionable. Does this mean that on POSIX boxes I cannot have my patch mailbox named 0:pt.patch, 1:pt.patch, etc.?

Adding "is_absolute_path" helper that has a different implementation depending on the platform in git-sh-setup and using it here would limit the extent of damage?

----------------------------------------------------------------
git-merge-octopus.sh

Transliterating a-z to A-z may happen to work because A-z is A-Z with a lot of garbage concatenated at the end that won't be used, but it feels sloppy.

Why isn't upcasing necessary for all the other uses of environment variables? For example, we pass reflog action by exporting a variable, and we use GIT_AUTHOR_NAME and friends to override configuration variables. Do they get upcased?

What I am getting at is that this might be just fixing a symptom, and between applying similar band-aid to many other places and finding out why undesired upcaing happens and fixing that, I'd rather see the latter done.

----------------------------------------------------------------
git-send-email.perl

Similar comment as is_absolute_path(), although in Perl environment I suspect we can just use an existing package without adding our own.

----------------------------------------------------------------
t/test-lib.sh

Is "DIFF=${DIFF:-diff}" needed? How is the existing initialization (in 'maint') of GIT_TEST_CMP working?

----------------------------------------------------------------

I didn't look at tests very deeply. Other changes outside compat/ that I didn't mention above looked fine.

Thanks.

Johannes Sixt

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

Quoted 6 lines · Junio C Hamano

git-merge-octopus.sh
Why isn't upcasing necessary for all the other uses of environment variables? For example, we pass reflog action by exporting a variable, and we use GIT_AUTHOR_NAME and friends to override configuration variables. Do they get upcased?

It is the environment variable *names* that get upper-cased, not the values. The variable name that was set for use by git-merge-octopus is of the form GITHEAD_deadbeef, but inside git-merge-octopus, the name is GITHEAD_DEADBEEF. Only MSYS programs like bash, but not native Windows programs like git, suffer from this.

-- Hannes

Junio C Hamano

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

Quoted 9 lines · Johannes Sixt

Quoted 6 lines · Junio C Hamano
git-merge-octopus.sh
Why isn't upcasing necessary for all the other uses of environment variables? For example, we pass reflog action by exporting a variable, and we use GIT_AUTHOR_NAME and friends to override configuration variables. Do they get upcased?
It is the environment variable *names* that get upper-cased, not the values.

Ah, of course. I should have noticed it if I read the patch more carefully.

Thanks for an explanation.

Ævar Arnfjörð Bjarmason

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

Quoted 4 lines · Junio C Hamano

git-send-email.perl
Similar comment as is_absolute_path(), although in Perl environment I suspect we can just use an existing package without adding our own.

It seems to me that the code added by Erik Faye-Lund in 33b2e81f84875bf515b4c0de830eeddfd04227dc and this new code in "send-email: handle Windows paths for display just like we do for processing" by bert could both be replaced by:

    use File::Spec::Functions qw(file_name_is_absolute);
    file_name_is_absolute($some_path);

And by doing so we'd also be compatible with e.g. VMS. Can the people with the odd non-Unix systems maybe try this out and see if it works? :)

Pat Thoyts

author

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

Quoted 12 lines · Ævar Arnfjörð Bjarmason

Quoted 4 lines · Junio C Hamano
git-send-email.perl
Similar comment as is_absolute_path(), although in Perl environment I suspect we can just use an existing package without adding our own.
It seems to me that the code added by Erik Faye-Lund in 33b2e81f84875bf515b4c0de830eeddfd04227dc and this new code in "send-email: handle Windows paths for display just like we do for processing" by bert could both be replaced by:

Quoted 6 lines

   use File::Spec::Functions qw(file_name_is_absolute);
   file_name_is_absolute($some_path);
And by doing so we'd also be compatible with e.g. VMS. Can the people with the odd non-Unix systems maybe try this out and see if it works? :)

I just looked into using this function with msysGit. Unfortunately it doesn't work as our perl is msys-compiled and doesn't recognise c:\ as an absolute path. It's using the unix path functions. Possibly we could use a native perl if the git perl functions were making more use of these platform-abstracting functions. Ultimately this is the right way to go.

Erik Faye-Lund

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

Quoted 21 lines · Pat Thoyts

Quoted 12 lines · Ævar Arnfjörð Bjarmason
Quoted 4 lines · Junio C Hamano
git-send-email.perl
Similar comment as is_absolute_path(), although in Perl environment I suspect we can just use an existing package without adding our own.
It seems to me that the code added by Erik Faye-Lund in 33b2e81f84875bf515b4c0de830eeddfd04227dc and this new code in "send-email: handle Windows paths for display just like we do for processing" by bert could both be replaced by:
Quoted 6 lines
use File::Spec::Functions qw(file_name_is_absolute); file_name_is_absolute($some_path);
And by doing so we'd also be compatible with e.g. VMS. Can the people with the odd non-Unix systems maybe try this out and see if it works? :)

Quoted 7 lines

I just looked into using this function with msysGit. Unfortunately it doesn't work as our perl is msys-compiled and doesn't recognise c:\ as an absolute path. It's using the unix path functions. Possibly we could use a native perl if the git perl functions were making more use of these platform-abstracting functions. Ultimately this is the right way to go.

Unfortunately, changing our perl is easier said than done, see issue 218 in the msysGit issue tracker for some of the nasty details:
http://code.google.com/p/msysgit/issues/detail?id=218

Ævar Arnfjörð Bjarmason

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

Quoted 21 lines · Pat Thoyts

Quoted 12 lines · Ævar Arnfjörð Bjarmason
Quoted 4 lines · Junio C Hamano
git-send-email.perl
Similar comment as is_absolute_path(), although in Perl environment I suspect we can just use an existing package without adding our own.
It seems to me that the code added by Erik Faye-Lund in 33b2e81f84875bf515b4c0de830eeddfd04227dc and this new code in "send-email: handle Windows paths for display just like we do for processing" by bert could both be replaced by:
Quoted 6 lines
use File::Spec::Functions qw(file_name_is_absolute); file_name_is_absolute($some_path);
And by doing so we'd also be compatible with e.g. VMS. Can the people with the odd non-Unix systems maybe try this out and see if it works? :)

Quoted 6 lines

I just looked into using this function with msysGit. Unfortunately it doesn't work as our perl is msys-compiled and doesn't recognise c:\ as an absolute path. It's using the unix path functions. Possibly we could use a native perl if the git perl functions were making more use of these platform-abstracting functions. Ultimately this is the right way to go.

That sounds like something msysGit needs to patch in its Perl build. The path functions on Windows account for drive letters, but if they're just using the Unix versions on not-quite-Unix that might break.

Pat Thoyts

author

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

msysGit patches for upstream

Quoted 5 lines · Junio C Hamano

pack-objects.c usage string
connect.c use of unchecked git_getpass()
Good eyes, but these should not be part of the series but applied to maint. If possible please send them separately.

Following up on this comment here are the two patches from msysGit.

Pat.

Junio C Hamano

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

Re: msysGit patches for upstream

Quoted 9 lines · Pat Thoyts

Quoted 5 lines · Junio C Hamano
pack-objects.c usage string
connect.c use of unchecked git_getpass()
Good eyes, but these should not be part of the series but applied to maint. If possible please send them separately.
Following up on this comment here are the two patches from msysGit.

Thanks.

Since you are forwarding the patch to me, I'll add your Sign-off (which was in the original series anyway) before applying them.

Pat Thoyts

author

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

PATCH 1/2 Make sure that git_getpass() never returns NULL

From: Johannes Schindelin <[email protected]>

The result of git_getpass() is used without checking for NULL, so let's just die() instead of returning NULL.

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

---
 connect.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

connect.c +6 -2

index 3450cab..57dc20c 100644
@@ -631,8 +631,12 @@ char *git_getpass(const char *prompt)
 		askpass = askpass_program;
 	if (!askpass)
 		askpass = getenv("SSH_ASKPASS");
-	if (!askpass || !(*askpass))
-		return getpass(prompt);
+	if (!askpass || !(*askpass)) {
+		char *result = getpass(prompt);
+		if (!result)
+			die_errno("Could not read password");
+		return result;
+	}
 
 	args[0] = askpass;
 	args[1]	= prompt;

Signature · 1 line

1.7.3

Pat Thoyts

author

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

PATCH 2/2 Fix typo in pack-objects' usage

From: Johannes Schindelin <[email protected]>

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

---
 builtin/pack-objects.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

builtin/pack-objects.c +1 -1

index 0e81673..3756cf3 100644
@@ -30,7 +30,7 @@ static const char pack_usage[] =
   "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset]\n"
   "        [--threads=N] [--non-empty] [--revs [--unpacked | --all]*]\n"
   "        [--reflog] [--stdout | base-name] [--include-tag]\n"
-  "        [--keep-unreachable | --unpack-unreachable \n"
+  "        [--keep-unreachable | --unpack-unreachable]\n"
   "        [<ref-list | <object-list]";
 
 struct object_entry {

Signature · 1 line

1.7.3

Eric Sunshine

2016-06-15 22:49 UTC #34 lore raw

Hi Junio,

Quoted 22 lines · Junio C Hamano

Quoted 3 lines · Pat Thoyts
Junio,
The msysGit tree currently tracks some 50+ patches on top of 'next'. I have gathered 42 of these that look good to move upstream.
A quick and superficial review follows.
----------------------------------------------------------------
abspath.c
(quoted diff) +4 -1
@@ -108,10 +108,15 @@ const char *make_nonrelative_path(const char *path)
  		if (strlcpy(buf, path, PATH_MAX)>= PATH_MAX)
  			die("Too long path: %.*s", 60, path);
  	} else {
+		size_t len;
+		const char *fmt;
  		const char *cwd = get_pwd_cwd();
  		if (!cwd)
  			die_errno("Cannot determine the current working directory");
-		if (snprintf(buf, PATH_MAX, "%s/%s", cwd, path)>= PATH_MAX)
+		len = strlen(cwd);
+		/* For cwd c:/, return c:/foo rather than URL-like c://foo */
 

Quoted 7 lines

For the patch to be regression free, the logic described by this comment requires get_pwd_cmd() to return a string with trailing dir-sep only at slash. IOW, if you see any non-root path returned with a trailing dir-sep for whatever reason, you are changing the behaviour in that case as well, and that clearly is not "fix at the root level".
But if you label this as "avoid duplicated dir-sep", everything flows smoothly ;-).

I am the author of this patch. Do I understand correctly that your primary concern is that you find the comment misleading? I consider the code

  fmt = (len > 0 && is_dir_sep(cwd[len-1])) ? "%s%s" : "%s/%s";

sufficiently self-documenting that the comment is superfluous, and would be happy to remove the comment. Would you prefer the patch submitted with the comment removed?

-- ES

A message that is not in the archive (sent off-list or not yet received): [email protected]

Eric Sunshine

2016-06-15 22:49 UTC #36 lore raw

Quoted 6 lines · Junio C Hamano

Quoted 4 lines · Eric Sunshine
I am the author of this patch. Do I understand correctly that your primary concern is that you find the comment misleading? I consider the code

Quoted 7 lines

Quoted 5 lines
   fmt = (len>  0&&  is_dir_sep(cwd[len-1])) ? "%s%s" : "%s/%s";
sufficiently self-documenting that the comment is superfluous, and would be happy to remove the comment. Would you prefer the patch submitted with the comment removed?
Yeah, that sounds sensible. Thanks.

Thank you for the response. I will re-submit the patch with the noted changes: (1) parentheses surrounding conditional expression, (2) removal of superfluous comment.

-- ES

yj2133011

2016-06-15 22:49 UTC #37 lore raw

This struct for now is just a wrapper for the current pathspec form:
const char **. It is intended to be extended with more useful pathspec-related information over time.

The data structure for passing pathspec around remains const char **, struct pathspec will be initialized locally to be used and destroyed. Hopefully all pathspec related code will be gradually migrated to pass this struct instead.

-----
The voice input and output is very good in this
http://www.tomtop.com/black-ps3-wireless-bluetooth-headset-for-playstation-3.html?aid=z
Wireless PS3 Headset . It is compatible with all PS3 games.Buy from Reliable
http://www.tomtop.com/google-android-7-notebook-3g-tablet-pc-umpc-wifi-mid-pda.html?aid=z
Google Android PC apad Wholesalers.

Signature · 2 lines

View this message in context: http://git.661346.n2.nabble.com/Pull-request-for-msysGit-patches-tp5578682p5582620.html
Sent from the git mailing list archive at Nabble.com.

Ramsay Jones

2016-06-15 22:49 UTC #38 lore raw