Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

Johannes Sixt [off-list ref] writes:

This sounds good to me.  Dscho?

Hi Junio,

On Fri, 22 Jan 2016, Junio C Hamano wrote:

Johannes Sixt [off-list ref] writes:

quoted

I suggest to move the function definition out of line:

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1915,6 +1915,13 @@ 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;

diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 
 #define has_dos_drive_prefix(path) \
 	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
 #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)

This sounds good to me.  Dscho?

Yep, sounds good to me, too.

Personally, I have no inclination to add compatibility with the
now-safely-obsolete MSys to my responsibilities, but if Hannes wants to do
it, who am I to stand in his way? Especially when the fix is as trivial as
here.

Ciao,
Dscho

Am 23.01.2016 um 09:25 schrieb Johannes Schindelin:

Hi Junio,

On Fri, 22 Jan 2016, Junio C Hamano wrote:

quoted

Johannes Sixt [off-list ref] writes:

quoted

I suggest to move the function definition out of line:

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1915,6 +1915,13 @@ 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;

diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);

  #define has_dos_drive_prefix(path) \
  	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
  #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
  #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
  static inline char *mingw_find_last_dir_sep(const char *path)

This sounds good to me.  Dscho?

Yep, sounds good to me, too.

Personally, I have no inclination to add compatibility with the
now-safely-obsolete MSys to my responsibilities, but if Hannes wants to do
it, who am I to stand in his way? Especially when the fix is as trivial as
here.

This is not a matter of compatibility. I am VERY curious why you do not 
see an error (or warning) without my proposed fixup. As I mentioned, 
isalpha() is defined much later than the definition of 
mingw_skip_dos_drive_prefix(). Where does your build get a declaration 
of isalpha() from?

-- Hannes

Hi Hannes,

On Sat, 23 Jan 2016, Johannes Sixt wrote:

Am 23.01.2016 um 09:25 schrieb Johannes Schindelin:

quoted

On Fri, 22 Jan 2016, Junio C Hamano wrote:

quoted

Johannes Sixt [off-list ref] writes:

quoted

I suggest to move the function definition out of line:

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1915,6 +1915,13 @@ 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;

diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);

  #define has_dos_drive_prefix(path) \
  	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
  #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
  #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
  static inline char *mingw_find_last_dir_sep(const char *path)

This sounds good to me.  Dscho?

Yep, sounds good to me, too.

Personally, I have no inclination to add compatibility with the
now-safely-obsolete MSys to my responsibilities, but if Hannes wants to do
it, who am I to stand in his way? Especially when the fix is as trivial as
here.

This is not a matter of compatibility. I am VERY curious why you do not see
an error (or warning) without my proposed fixup. As I mentioned, isalpha() is
defined much later than the definition of mingw_skip_dos_drive_prefix().
Where does your build get a declaration of isalpha() from?

$ grep -w isalpha /mingw32/i686-w64-mingw32/include/*.h
/mingw32/i686-w64-mingw32/include/ctype.h:  _CRTIMP int __cdecl isalpha(int _C);
/mingw32/i686-w64-mingw32/include/ctype.h:#define __iscsymf(_c) (isalpha(_c) || ((_c)=='_'))

I guess that definition gets pulled in somehow.

Ciao,
Dscho

Am 24.01.2016 um 11:56 schrieb Johannes Schindelin:

$ grep -w isalpha /mingw32/i686-w64-mingw32/include/*.h
/mingw32/i686-w64-mingw32/include/ctype.h:  _CRTIMP int __cdecl isalpha(int _C);
/mingw32/i686-w64-mingw32/include/ctype.h:#define __iscsymf(_c) (isalpha(_c) || ((_c)=='_'))

I guess that definition gets pulled in somehow.

Ok, then, Junio, kindly replace js/dirname-basename~4 with this patch.
(I hope I get the patch headers right for git-am.)

---- 8< ----
From: Johannes Schindelin <redacted>
Subject: [PATCH] Refactor skipping DOS drive prefixes

Junio noticed that there is an implicit assumption in pretty much
all the code calling has_dos_drive_prefix(): it forces all of its
callsites to hardcode the knowledge that the DOS drive prefix is
always two bytes long.

While this assumption is pretty safe, we can still make the code
more readable and less error-prone by introducing a function that
skips the DOS drive prefix safely.

While at it, we change the has_dos_drive_prefix() return value: it
now returns the number of bytes to be skipped if there is a DOS
drive prefix.

[j6t: moved definition of mingw_skip_dos_drive_prefix() out of line]

Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Johannes Sixt <redacted>
---
 compat/basename.c |  4 +---
 compat/mingw.c    | 21 ++++++++++++---------
 compat/mingw.h    |  5 ++++-
 git-compat-util.h |  8 ++++++++
 path.c            | 14 +++++---------
 5 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..9f00421 100644
--- a/compat/basename.c
+++ b/compat/basename.c

@@ -4,9 +4,7 @@
 char *gitbasename (char *path)
 {
 	const char *base;
-	/* Skip over the disk name in MSDOS pathnames. */
-	if (has_dos_drive_prefix(path))
-		path += 2;
+	skip_dos_drive_prefix(&path);
 	for (base = path; *path; path++) {
 		if (is_dir_sep(*path))
 			base = path + 1;

diff --git a/compat/mingw.c b/compat/mingw.c
index f74da23..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1915,28 +1915,31 @@ 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)
 {
-	int offset = 0;
-	if (has_dos_drive_prefix(path))
-		offset = 2;
+	char *pos = (char *)path;
 
 	/* unc paths */
-	else if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
-
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
 		/* skip server name */
-		char *pos = strpbrk(path + 2, "\\/");
+		pos = strpbrk(pos + 2, "\\/");
 		if (!pos)
 			return 0; /* Error: malformed unc path */
 
 		do {
 			pos++;
 		} while (*pos && !is_dir_sep(*pos));
-
-		offset = pos - path;
 	}
 
-	return offset + is_dir_sep(path[offset]);
+	return pos + is_dir_sep(*pos) - path;
 }
 
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)

diff --git a/compat/mingw.h b/compat/mingw.h
index 738865c..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -358,7 +358,10 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#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
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
 {

diff --git a/git-compat-util.h b/git-compat-util.h
index 0feeae2..38397d7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -335,6 +335,14 @@ static inline int git_has_dos_drive_prefix(const char *path)
 #define has_dos_drive_prefix git_has_dos_drive_prefix
 #endif
 
+#ifndef skip_dos_drive_prefix
+static inline int git_skip_dos_drive_prefix(char **path)
+{
+	return 0;
+}
+#define skip_dos_drive_prefix git_skip_dos_drive_prefix
+#endif
+
 #ifndef is_dir_sep
 static inline int git_is_dir_sep(int c)
 {

diff --git a/path.c b/path.c
index 38f2ebd..747d6da 100644
--- a/path.c
+++ b/path.c

@@ -544,13 +544,10 @@ const char *relative_path(const char *in, const char *prefix,
 	else if (!prefix_len)
 		return in;
 
-	if (have_same_root(in, prefix)) {
+	if (have_same_root(in, prefix))
 		/* bypass dos_drive, for "c:" is identical to "C:" */
-		if (has_dos_drive_prefix(in)) {
-			i = 2;
-			j = 2;
-		}
-	} else {
+		i = j = has_dos_drive_prefix(in);
+	else {
 		return in;
 	}
 

@@ -703,11 +700,10 @@ const char *remove_leading_path(const char *in, const char *prefix)
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 {
 	char *dst0;
+	int i;
 
-	if (has_dos_drive_prefix(src)) {
+	for (i = has_dos_drive_prefix(src); i > 0; i--)
 		*dst++ = *src++;
-		*dst++ = *src++;
-	}
 	dst0 = dst;
 
 	if (is_dir_sep(*src)) {

-- 
2.7.0.118.g90056ae