diff mbox series

[v3,1/1] git clone <url> C:\cygwin\home\USER\repo' is working (again)

Message ID 20181208151109.2097-1-tboegi@web.de (mailing list archive)
State New, archived
Headers show
Series [v3,1/1] git clone <url> C:\cygwin\home\USER\repo' is working (again) | expand

Commit Message

Torsten Bögershausen Dec. 8, 2018, 3:11 p.m. UTC
From: Torsten Bögershausen <tboegi@web.de>

A regression for cygwin users was introduced with commit 05b458c,
 "real_path: resolve symlinks by hand".

In the the commit message we read:
  The current implementation of real_path uses chdir() in order to resolve
    symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
      process as a whole...

The old (and non-thread-save) OS calls chdir()/pwd() had been
replaced by a string operation.
The cygwin layer "knows" that "C:\cygwin" is an absolute path,
but the new string operation does not.

"git clone <url> C:\cygwin\home\USER\repo" fails like this:
fatal: Invalid path '/home/USER/repo/C:\cygwin\home\USER\repo'

The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]

Extract the needed code into compat/win32/path-utils.[ch] and use it
for cygwin as well.

Reported-by: Steven Penny <svnpenn@gmail.com>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
Changes since V2:
- Settled on a better name:
  The common code is in compat/win32/path-utils.c/h
- Skip the 2 patches which "only" do a cleanup (for a moment)
  put those cleanups onto the "todo stack".
- The "DOS" moniker is still used for 2 reasons:
  Windows inherited the "drive letter" concept from DOS,
  and everybody (tm) familar with the code and the path handling
  in Git is used to that wording.
  Even if there was a better name, it needed to be addressed
  in a patch series different from this one.
  Here I want to fix a reported regression.
   
And, before any cleanup is done, I sould like to ask if anybody
can build the code with VS and confirm that it works, please ?

Thanks for the reviews, testing and comment.

compat/cygwin.c           | 19 -------------------
 compat/cygwin.h           |  2 --
 compat/mingw.c            | 29 +----------------------------
 compat/mingw.h            | 20 --------------------
 compat/win32/path-utils.c | 28 ++++++++++++++++++++++++++++
 compat/win32/path-utils.h | 20 ++++++++++++++++++++
 config.mak.uname          |  3 ++-
 git-compat-util.h         |  3 ++-
 8 files changed, 53 insertions(+), 71 deletions(-)
 delete mode 100644 compat/cygwin.c
 delete mode 100644 compat/cygwin.h
 create mode 100644 compat/win32/path-utils.c
 create mode 100644 compat/win32/path-utils.h

Comments

Steven Penny Dec. 8, 2018, 4:24 p.m. UTC | #1
On Sat, Dec 8, 2018 at 9:11 AM wrote:
> Changes since V2:

latest patch still fixes original issue - thanks

> - Settled on a better name:
>   The common code is in compat/win32/path-utils.c/h
>   [...]
> - The "DOS" moniker is still used for 2 reasons:
>   Windows inherited the "drive letter" concept from DOS,
>   and everybody (tm) familar with the code and the path handling
>   in Git is used to that wording.
>   Even if there was a better name, it needed to be addressed
>   in a patch series different from this one.
>   Here I want to fix a reported regression.

i still disagree with this - but i understand if naming argument is out of scope
for thread

> And, before any cleanup is done, I sould like to ask if anybody
> can build the code with VS and confirm that it works, please ?

sorry but i am decidedly *not* interested in doing this. i use cygwin
specifically so that i can avoid VS. hopefully someone else will be able to
test. cheers
Junio C Hamano Dec. 9, 2018, 1:39 a.m. UTC | #2
tboegi@web.de writes:

> - The "DOS" moniker is still used for 2 reasons:
>   Windows inherited the "drive letter" concept from DOS,
>   and everybody (tm) familar with the code and the path handling
>   in Git is used to that wording.

Yeah, for the same reason as win32 can refer to their API that is
used on platforms that are 64-bit, the fact that the "drive letter"
concept came from DOS is so widely ingrained, I do not think it is a
beter change to deviate from it.

> And, before any cleanup is done, I sould like to ask if anybody
> can build the code with VS and confirm that it works, please ?

Yup, that is much more important.

Thanks.
Johannes Schindelin Dec. 10, 2018, 8:32 a.m. UTC | #3
Hi Torsten,

On Sat, 8 Dec 2018, tboegi@web.de wrote:

> And, before any cleanup is done, I sould like to ask if anybody
> can build the code with VS and confirm that it works, please ?

Can you give me an easy-to-fetch branch?

Thanks,
Dscho
Torsten Bögershausen Dec. 11, 2018, 6:12 a.m. UTC | #4
On Mon, Dec 10, 2018 at 09:32:03AM +0100, Johannes Schindelin wrote:
> Hi Torsten,
> 
> On Sat, 8 Dec 2018, tboegi@web.de wrote:
> 
> > And, before any cleanup is done, I sould like to ask if anybody
> > can build the code with VS and confirm that it works, please ?
> 
> Can you give me an easy-to-fetch branch?
> 
> Thanks,
> Dscho

@Dscho: The branch should be here:
  https://github.com/tboegi/git/tree/tb.181208_0844_cygwin-dos-drive
  (or fetch it from Junio, please see below:)

@Junio:
  Please keep tb/use-common-win32-pathfuncs-on-cygwin
  in pu for a while. I need to send a V4 to fix t5601 for cygwin.
Johannes Schindelin Dec. 11, 2018, 1:28 p.m. UTC | #5
Hi Torsten,

On Tue, 11 Dec 2018, Torsten Bögershausen wrote:

> On Mon, Dec 10, 2018 at 09:32:03AM +0100, Johannes Schindelin wrote:
> > 
> > On Sat, 8 Dec 2018, tboegi@web.de wrote:
> > 
> > > And, before any cleanup is done, I sould like to ask if anybody
> > > can build the code with VS and confirm that it works, please ?
> > 
> > Can you give me an easy-to-fetch branch?
> > 
> > Thanks,
> > Dscho
> 
> @Dscho: The branch should be here:
>   https://github.com/tboegi/git/tree/tb.181208_0844_cygwin-dos-drive
>   (or fetch it from Junio, please see below:)

I fetched it from you, as Junio frequently applies patches anywhere except
where they were developed. I'd rather see what you see. For the record,
this is the commit I tested: cc1e08eeb83b.

It builds fine here, and some cursory tests reveal that it works as
advertised (I ran t0001, t0060 and t5580).

However.

Can you please replace the rather unnecessary, very, very long
`win_path_utils_` function name prefix by the much better prefix `win32_`,
to keep in line with the current, already existing, surrounding files'
convention? Thanks a bunch.

Ciao,
Dscho

> @Junio:
>   Please keep tb/use-common-win32-pathfuncs-on-cygwin
>   in pu for a while. I need to send a V4 to fix t5601 for cygwin.
>  
>
Torsten Bögershausen Dec. 11, 2018, 6:55 p.m. UTC | #6
> 
> Can you please replace the rather unnecessary, very, very long
> `win_path_utils_` function name prefix by the much better prefix `win32_`,
> to keep in line with the current, already existing, surrounding files'
> convention? Thanks a bunch.
> 

That makes sense - thanks for the suggestion & testing.
diff mbox series

Patch

diff --git a/compat/cygwin.c b/compat/cygwin.c
deleted file mode 100644
index b9862d606d..0000000000
--- a/compat/cygwin.c
+++ /dev/null
@@ -1,19 +0,0 @@ 
-#include "../git-compat-util.h"
-#include "../cache.h"
-
-int cygwin_offset_1st_component(const char *path)
-{
-	const char *pos = path;
-	/* unc paths */
-	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
-		/* skip server name */
-		pos = strchr(pos + 2, '/');
-		if (!pos)
-			return 0; /* Error: malformed unc path */
-
-		do {
-			pos++;
-		} while (*pos && pos[0] != '/');
-	}
-	return pos + is_dir_sep(*pos) - path;
-}
diff --git a/compat/cygwin.h b/compat/cygwin.h
deleted file mode 100644
index 8e52de4644..0000000000
--- a/compat/cygwin.h
+++ /dev/null
@@ -1,2 +0,0 @@ 
-int cygwin_offset_1st_component(const char *path);
-#define offset_1st_component cygwin_offset_1st_component
diff --git a/compat/mingw.c b/compat/mingw.c
index 34b3880b29..27e397f268 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -350,7 +350,7 @@  static inline int needs_hiding(const char *path)
 		return 0;
 
 	/* We cannot use basename(), as it would remove trailing slashes */
-	mingw_skip_dos_drive_prefix((char **)&path);
+	win_path_utils_skip_dos_drive_prefix((char **)&path);
 	if (!*path)
 		return 0;
 
@@ -2275,33 +2275,6 @@  pid_t waitpid(pid_t pid, int *status, int options)
 	return -1;
 }
 
-int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
-
-int mingw_offset_1st_component(const char *path)
-{
-	char *pos = (char *)path;
-
-	/* unc paths */
-	if (!skip_dos_drive_prefix(&pos) &&
-			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
-		/* skip server name */
-		pos = strpbrk(pos + 2, "\\/");
-		if (!pos)
-			return 0; /* Error: malformed unc path */
-
-		do {
-			pos++;
-		} while (*pos && !is_dir_sep(*pos));
-	}
-
-	return pos + is_dir_sep(*pos) - path;
-}
-
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
 {
 	int upos = 0, wpos = 0;
diff --git a/compat/mingw.h b/compat/mingw.h
index 8c24ddaa3e..30d9fb3e36 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -443,32 +443,12 @@  HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) \
-	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-int mingw_skip_dos_drive_prefix(char **path);
-#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
-static inline int mingw_is_dir_sep(int c)
-{
-	return c == '/' || c == '\\';
-}
-#define is_dir_sep mingw_is_dir_sep
-static inline char *mingw_find_last_dir_sep(const char *path)
-{
-	char *ret = NULL;
-	for (; *path; ++path)
-		if (is_dir_sep(*path))
-			ret = (char *)path;
-	return ret;
-}
 static inline void convert_slashes(char *path)
 {
 	for (; *path; path++)
 		if (*path == '\\')
 			*path = '/';
 }
-#define find_last_dir_sep mingw_find_last_dir_sep
-int mingw_offset_1st_component(const char *path);
-#define offset_1st_component mingw_offset_1st_component
 #define PATH_SEP ';'
 extern char *mingw_query_user_email(void);
 #define query_user_email mingw_query_user_email
diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c
new file mode 100644
index 0000000000..6cb9a6a745
--- /dev/null
+++ b/compat/win32/path-utils.c
@@ -0,0 +1,28 @@ 
+#include "../../git-compat-util.h"
+
+int win_path_utils_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+
+int win_path_utils_offset_1st_component(const char *path)
+{
+	char *pos = (char *)path;
+
+	/* unc paths */
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+		/* skip server name */
+		pos = strpbrk(pos + 2, "\\/");
+		if (!pos)
+			return 0; /* Error: malformed unc path */
+
+		do {
+			pos++;
+		} while (*pos && !is_dir_sep(*pos));
+	}
+
+	return pos + is_dir_sep(*pos) - path;
+}
diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
new file mode 100644
index 0000000000..c931b2a890
--- /dev/null
+++ b/compat/win32/path-utils.h
@@ -0,0 +1,20 @@ 
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+int win_path_utils_skip_dos_drive_prefix(char **path);
+#define skip_dos_drive_prefix win_path_utils_skip_dos_drive_prefix
+static inline int win_path_utils_is_dir_sep(int c)
+{
+	return c == '/' || c == '\\';
+}
+#define is_dir_sep win_path_utils_is_dir_sep
+static inline char *win_path_utils_find_last_dir_sep(const char *path)
+{
+	char *ret = NULL;
+	for (; *path; ++path)
+		if (is_dir_sep(*path))
+			ret = (char *)path;
+	return ret;
+}
+#define find_last_dir_sep win_path_utils_find_last_dir_sep
+int win_path_utils_offset_1st_component(const char *path);
+#define offset_1st_component win_path_utils_offset_1st_component
diff --git a/config.mak.uname b/config.mak.uname
index 3ee7da0e23..60876e26f4 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -187,7 +187,7 @@  ifeq ($(uname_O),Cygwin)
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
-	COMPAT_OBJS += compat/cygwin.o
+	COMPAT_OBJS += compat/win32/path-utils.o
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),FreeBSD)
@@ -527,6 +527,7 @@  ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/winansi.o \
+		compat/win32/path-utils.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
 		compat/win32/dirent.o
 	BASIC_CFLAGS += -DWIN32 -DPROTECT_NTFS_DEFAULT=1
diff --git a/git-compat-util.h b/git-compat-util.h
index 09b0102cae..5702556c89 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -193,10 +193,11 @@ 
 #endif
 
 #if defined(__CYGWIN__)
-#include "compat/cygwin.h"
+#include "compat/win32/path-utils.h"
 #endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
+#include "compat/win32/path-utils.h"
 #include "compat/mingw.h"
 #elif defined(_MSC_VER)
 #include "compat/msvc.h"