[PATCH] Handle UNC paths everywhere

[PATCH] Handle UNC paths everywhere

[Robin Rosenberg]

>From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Mon, 25 Jan 2010 01:41:03 +0100
Subject: [PATCH] Handle UNC paths everywhere

In Windows paths beginning with // are knows as UNC paths. They are
absolute paths, usually referring to a shared resource on a server.

Examples of legal UNC paths

	\\hub\repos\repo
	\\?\unc\hub\repos
	\\?\d:\repo

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 cache.h           |    2 +-
 compat/basename.c |    2 +-
 compat/mingw.h    |    8 +++++++-
 connect.c         |    2 +-
 git-compat-util.h |    9 +++++++++
 path.c            |    2 +-
 setup.c           |    2 +-
 sha1_file.c       |   20 ++++++++++++++++++++
 transport.c       |    2 +-
 9 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 767a50e..8f63640 100644
--- a/cache.h
+++ b/cache.h
@@ -648,7 +648,7 @@ int safe_create_leading_directories_const(const char 
*path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
-	return path[0] == '/' || has_dos_drive_prefix(path);
+	return path[0] == '/' || has_win32_abs_prefix(path);
 }
 int is_directory(const char *);
 const char *make_absolute_path(const char *path);
diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..c1d81f6 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -5,7 +5,7 @@ char *gitbasename (char *path)
 {
 	const char *base;
 	/* Skip over the disk name in MSDOS pathnames. */
-	if (has_dos_drive_prefix(path))
+	if (has_win32_abs_prefix(path))
 		path += 2;
 	for (base = path; *path; path++) {
 		if (is_dir_sep(*path))
diff --git a/compat/mingw.h b/compat/mingw.h
index 1b528da..d1aa8be 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -210,7 +210,13 @@ int winansi_fprintf(FILE *stream, const char *format, 
...) __attribute__((format
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':')
+#define has_unc_prefix(path) \
+	(is_dir_sep((path)[0]) && is_dir_sep((path)[1]))
+#define has_win32_abs_prefix(path) \
+	(has_dos_drive_prefix(path) || has_unc_prefix(path))
+
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 #define PATH_SEP ';'
 #define PRIuMAX "I64u"
diff --git a/connect.c b/connect.c
index 7945e38..9d4556c 100644
--- a/connect.c
+++ b/connect.c
@@ -535,7 +535,7 @@ struct child_process *git_connect(int fd[2], const char 
*url_orig,
 		end = host;
 
 	path = strchr(end, c);
-	if (path && !has_dos_drive_prefix(end)) {
+	if (path && !has_win32_abs_prefix(end)) {
 		if (c == ':') {
 			protocol = PROTO_SSH;
 			*path++ = '\0';
diff --git a/git-compat-util.h b/git-compat-util.h
index ef60803..0de9dac 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -170,6 +170,15 @@ extern char *gitbasename(char *);
 #define has_dos_drive_prefix(path) 0
 #endif
 
+#ifndef has_unc_prefix
+#define has_unc_prefix(path) 0
+#endif
+
+#ifndef has_win32_abs_prefix
+#error no abs
+#define has_win32_abs_prefix(path) 0
+#endif
+
 #ifndef is_dir_sep
 #define is_dir_sep(c) ((c) == '/')
 #endif
diff --git a/path.c b/path.c
index 047fdb0..79451a2 100644
--- a/path.c
+++ b/path.c
@@ -409,7 +409,7 @@ int normalize_path_copy(char *dst, const char *src)
 {
 	char *dst0;
 
-	if (has_dos_drive_prefix(src)) {
+	if (has_win32_abs_prefix(src)) {
 		*dst++ = *src++;
 		*dst++ = *src++;
 	}
diff --git a/setup.c b/setup.c
index 029371e..4f72817 100644
--- a/setup.c
+++ b/setup.c
@@ -342,7 +342,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		die_errno("Unable to read current working directory");
 
 	ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
-	if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
+	if (ceil_offset < 0 && has_win32_abs_prefix(cwd))
 		ceil_offset = 1;
 
 	/*
diff --git a/sha1_file.c b/sha1_file.c
index 4cc8939..f1ad3f5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -87,6 +87,26 @@ static inline int offset_1st_component(const char *path)
 {
 	if (has_dos_drive_prefix(path))
 		return 2 + (path[2] == '/');
+	if (has_unc_prefix(path)) {
+		int p = 2;
+		int skip;
+		if (path[p] == '?') {
+			if (path[p+1] && !has_dos_drive_prefix(path+p+2)) {
+				skip = 3;
+			} else {
+				skip = 2;
+			}
+		} else
+			skip = 2;
+
+		while (skip && path[p]) {
+			if (is_dir_sep(path[p]))
+				--skip;
+			++p;
+		}
+		printf("Left with %s\n", path+p);
+		return p;
+	}
 	return *path == '/';
 }
 
diff --git a/transport.c b/transport.c
index 644a30a..9f5b24e 100644
--- a/transport.c
+++ b/transport.c
@@ -797,7 +797,7 @@ static int is_local(const char *url)
 	const char *colon = strchr(url, ':');
 	const char *slash = strchr(url, '/');
 	return !colon || (slash && slash < colon) ||
-		has_dos_drive_prefix(url);
+		has_win32_abs_prefix(url);
 }
 
 static int is_file(const char *url)
-- 
1.6.4.msysgit.0.598.g37a74

Re: [PATCH] Handle UNC paths everywhere

[Sverre Rabbelier]

Heya,

On Mon, Jan 25, 2010 at 01:55, Robin Rosenberg
<robin.rosenberg@dewire.com> wrote:
> In Windows paths beginning with // are knows as UNC paths. They are
> absolute paths, usually referring to a shared resource on a server.

Cute, but will it actually work? I've tried to use them // paths on
windows before with MSysGit, and it's never worked, probably due to
the same reason why it doesn't work in the cmd prompt (whatever reason
that may be).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Handle UNC paths everywhere

[Robin Rosenberg]

måndagen den 25 januari 2010 10.36.59 skrev  Sverre Rabbelier:
> Heya,
> 
> On Mon, Jan 25, 2010 at 01:55, Robin Rosenberg
> 
> <robin.rosenberg@dewire.com> wrote:
> > In Windows paths beginning with // are knows as UNC paths. They are
> > absolute paths, usually referring to a shared resource on a server.
> 
> Cute, but will it actually work? I've tried to use them // paths on
> windows before with MSysGit, and it's never worked, probably due to
Works here (tm). Latest msygit + rebuilt git binaries on Windows 2003.

The only program I know in the msysgit installation that didn't accept UNC 
paths prior to this patch was in fact git. 

> the same reason why it doesn't work in the cmd prompt (whatever reason
> that may be).

Any code of the this form will of course not support UNC paths:

if (!has_drive_letter(path))
	boom();

-- robin

Re: [PATCH] Handle UNC paths everywhere

[Erik Faye-Lund]

On Mon, Jan 25, 2010 at 10:36 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Mon, Jan 25, 2010 at 01:55, Robin Rosenberg
> <robin.rosenberg@dewire.com> wrote:
>> In Windows paths beginning with // are knows as UNC paths. They are
>> absolute paths, usually referring to a shared resource on a server.
>
> Cute, but will it actually work? I've tried to use them // paths on
> windows before with MSysGit, and it's never worked, probably due to
> the same reason why it doesn't work in the cmd prompt (whatever reason
> that may be).
>

This works for me, Vista 64-bit:

C:\Users\kusma>dir \\mongo\code
The request is not supported.

C:\Users\kusma>explorer \\mongo\code
<login on the gui>

C:\Users\kusma>dir \\mongo\code
 Volume in drive \\mongo\code is Code
 Volume Serial Number is 04C3-0225

 Directory of \\mongo\code

10.01.2010  21:39    <DIR>          .
04.01.2010  01:28    <DIR>          ..
04.01.2010  00:51    <DIR>          qt-rocket
10.01.2010  21:38    <DIR>          very_last_64k_ever
11.01.2010  19:57    <DIR>          scratch
               0 File(s)              0 bytes
               5 Dir(s)  958 499 627 008 bytes free

C:\Users\kusma>

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] Handle UNC paths everywhere

[Sverre Rabbelier]

Heya,

On Mon, Jan 25, 2010 at 11:11, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> C:\Users\kusma>dir \\mongo\code
> The request is not supported.
>
> C:\Users\kusma>explorer \\mongo\code
> <login on the gui>
>
> C:\Users\kusma>dir \\mongo\code
>  Volume in drive \\mongo\code is Code
>  Volume Serial Number is 04C3-0225

Ah, that's very interesting. Not sure that will help MSysGit a lot though.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Handle UNC paths everywhere

[Robin Rosenberg]

måndagen den 25 januari 2010 11.22.19 skrev  Sverre Rabbelier:
> Heya,
> 
> On Mon, Jan 25, 2010 at 11:11, Erik Faye-Lund <kusmabite@googlemail.com> 
wrote:
> > C:\Users\kusma>dir \\mongo\code
> > The request is not supported.
> >
> > C:\Users\kusma>explorer \\mongo\code
> > <login on the gui>
> >
> > C:\Users\kusma>dir \\mongo\code
> >  Volume in drive \\mongo\code is Code
> >  Volume Serial Number is 04C3-0225
> 
> Ah, that's very interesting. Not sure that will help MSysGit a lot though.
> 

Could you perhaps *try* it before claiming it won't work? I suggest you
use forward slashes to avoid quoting problems.

-- robin

Re: [PATCH] Handle UNC paths everywhere

[Sverre Rabbelier]

Heya,

On Mon, Jan 25, 2010 at 12:01, Robin Rosenberg
<robin.rosenberg@dewire.com> wrote:
>> Ah, that's very interesting. Not sure that will help MSysGit a lot though.
>>
>
> Could you perhaps *try* it before claiming it won't work? I suggest you
> use forward slashes to avoid quoting problems.

Actually, I can't, cos I don't have a MSysGit build environment, I'm
just saying that in the environment I tested it in, I didn't have to
log in, so I suspect that it won't work, I'm not claiming anything.


-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Handle UNC paths everywhere

[Erik Faye-Lund]

On Mon, Jan 25, 2010 at 12:06 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Mon, Jan 25, 2010 at 12:01, Robin Rosenberg
> <robin.rosenberg@dewire.com> wrote:
>>> Ah, that's very interesting. Not sure that will help MSysGit a lot though.
>>>
>>
>> Could you perhaps *try* it before claiming it won't work? I suggest you
>> use forward slashes to avoid quoting problems.
>
> Actually, I can't, cos I don't have a MSysGit build environment, I'm
> just saying that in the environment I tested it in, I didn't have to
> log in, so I suspect that it won't work, I'm not claiming anything.
>

For shares that doesn't require login, I can list files without doing
anything. Remember that Windows might try some saved password in the
background when you open the folder in explorer, so not seeing a
password-prompt might not be the same thing as the share not needing
login.

Perhaps "net use \\mongo\code" would have been a better example than
using explorer? I don't have the setup where I am right now to test
that it does what I expect it to, though.

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] Handle UNC paths everywhere

[Johannes Schindelin]

Hi,

On Mon, 25 Jan 2010, Robin Rosenberg wrote:

> >From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
> From: Robin Rosenberg <robin.rosenberg@dewire.com>
> Date: Mon, 25 Jan 2010 01:41:03 +0100
> Subject: [PATCH] Handle UNC paths everywhere
> 
> In Windows paths beginning with // are knows as UNC paths. They are
> absolute paths, usually referring to a shared resource on a server.

And even a simple "cd" with them does not work.

> Examples of legal UNC paths
> 
> 	\\hub\repos\repo
> 	\\?\unc\hub\repos
> 	\\?\d:\repo
> 
> Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> ---
>  cache.h           |    2 +-
>  compat/basename.c |    2 +-
>  compat/mingw.h    |    8 +++++++-
>  connect.c         |    2 +-
>  git-compat-util.h |    9 +++++++++
>  path.c            |    2 +-
>  setup.c           |    2 +-
>  sha1_file.c       |   20 ++++++++++++++++++++
>  transport.c       |    2 +-
>  9 files changed, 42 insertions(+), 7 deletions(-)

Ouch.  You should know better than to clutter non-Windows-specific parts 
with that ugly kludge.

> diff --git a/cache.h b/cache.h
> index 767a50e..8f63640 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -648,7 +648,7 @@ int safe_create_leading_directories_const(const char 
> *path);
>  char *enter_repo(char *path, int strict);
>  static inline int is_absolute_path(const char *path)
>  {
> -	return path[0] == '/' || has_dos_drive_prefix(path);
> +	return path[0] == '/' || has_win32_abs_prefix(path);

Why?  We can still keep the name.  Well, maybe not, see below.

> diff --git a/compat/mingw.h b/compat/mingw.h
> index 1b528da..d1aa8be 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -210,7 +210,13 @@ int winansi_fprintf(FILE *stream, const char *format, 
> ...) __attribute__((format
>   * git specific compatibility
>   */
>  
> -#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
> +#define has_dos_drive_prefix(path) \
> +	(isalpha(*(path)) && (path)[1] == ':')

Why?

> +#define has_unc_prefix(path) \
> +	(is_dir_sep((path)[0]) && is_dir_sep((path)[1]))
> +#define has_win32_abs_prefix(path) \
> +	(has_dos_drive_prefix(path) || has_unc_prefix(path))

"c:hello.txt" is not an absolute path.

> diff --git a/connect.c b/connect.c
> index 7945e38..9d4556c 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -535,7 +535,7 @@ struct child_process *git_connect(int fd[2], const char 
> *url_orig,
>  		end = host;
>  
>  	path = strchr(end, c);
> -	if (path && !has_dos_drive_prefix(end)) {
> +	if (path && !has_win32_abs_prefix(end)) {
>  		if (c == ':') {

Why?  Do we really have to exclude UNC paths from that ":" handling?

> diff --git a/git-compat-util.h b/git-compat-util.h
> index ef60803..0de9dac 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -170,6 +170,15 @@ extern char *gitbasename(char *);
>  #define has_dos_drive_prefix(path) 0
>  #endif
>  
> +#ifndef has_unc_prefix
> +#define has_unc_prefix(path) 0
> +#endif
> +
> +#ifndef has_win32_abs_prefix
> +#error no abs

Yeah, sure.  I do have abs, thank you very much.

In general, I am _very_ worried about your patch.  It does not acknowledge 
that there is a fundamental difference between DOS drive prefixes and UNC 
paths, and not being able to "cd" to the latter is just a symptom.

I am also not quite sure if you can get away with having the same offset 
for both: if I have "C:\blah" and strip off "C:", I always have a 
directory separator to bounce against, whereas I do not have that if I 
strip off the two "\\" of a UNC path.  Besides, I maintain that the host 
name, and maybe even the share name, should not ever be stripped off!

At least a discussion is missing from the commit message.

Ciao,
Dscho

Re: [PATCH] Handle UNC paths everywhere

[Erik Faye-Lund]

On Mon, Jan 25, 2010 at 6:34 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Mon, 25 Jan 2010, Robin Rosenberg wrote:
>
>> >From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
>> From: Robin Rosenberg <robin.rosenberg@dewire.com>
>> Date: Mon, 25 Jan 2010 01:41:03 +0100
>> Subject: [PATCH] Handle UNC paths everywhere
>>
>> In Windows paths beginning with // are knows as UNC paths. They are
>> absolute paths, usually referring to a shared resource on a server.
>
> And even a simple "cd" with them does not work.
>

But it does, at least for me - both in bash and cmd.exe. I just need
to log on to the server first, unless it's a public share.

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] Handle UNC paths everywhere

[Johannes Schindelin]

Hi,

On Mon, 25 Jan 2010, Erik Faye-Lund wrote:

> On Mon, Jan 25, 2010 at 6:34 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Mon, 25 Jan 2010, Robin Rosenberg wrote:
> >
> >> >From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
> >> From: Robin Rosenberg <robin.rosenberg@dewire.com>
> >> Date: Mon, 25 Jan 2010 01:41:03 +0100
> >> Subject: [PATCH] Handle UNC paths everywhere
> >>
> >> In Windows paths beginning with // are knows as UNC paths. They are
> >> absolute paths, usually referring to a shared resource on a server.
> >
> > And even a simple "cd" with them does not work.
> >
> 
> But it does, at least for me - both in bash and cmd.exe. I just need
> to log on to the server first, unless it's a public share.

I love it when people say "it works for me, so let's do it".

_My_ _only_ instance of Windows cmd says this:

	C:\Blah> cd \\localhost
	'\\localhost'
	CMD does not support UNC paths as current directories.
	
	C:\Blah>

So.

Besides, the patch was not in a form where I can say that it was obviously 
fixing the issue. It was rather in a form where I would have to have set 
aside a substantial amount of time to verify that nothing undesired was 
introduced as a side effect.

Ciao,
Dscho

Re: [PATCH] Handle UNC paths everywhere

[Robin Rosenberg]

måndagen den 25 januari 2010 18.57.06 skrev  Erik Faye-Lund:
> On Mon, Jan 25, 2010 at 6:34 PM, Johannes Schindelin
> 
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Mon, 25 Jan 2010, Robin Rosenberg wrote:
> >> >From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
> >>
> >> From: Robin Rosenberg <robin.rosenberg@dewire.com>
> >> Date: Mon, 25 Jan 2010 01:41:03 +0100
> >> Subject: [PATCH] Handle UNC paths everywhere
> >>
> >> In Windows paths beginning with // are knows as UNC paths. They are
> >> absolute paths, usually referring to a shared resource on a server.
> >
> > And even a simple "cd" with them does not work.
> 
> But it does, at least for me - both in bash and cmd.exe. I just needt.

In cmd,exe surprises me a bit. pushd \\server\share is not the same
as it maps a drive and then uses it to cd.

-- robin

Re: [PATCH] Handle UNC paths everywhere

[Robin Rosenberg]

måndagen den 25 januari 2010 18.34.01 skrev  Johannes Schindelin:
> Hi,
> 
> On Mon, 25 Jan 2010, Robin Rosenberg wrote:
> > >From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
> >
> > From: Robin Rosenberg <robin.rosenberg@dewire.com>
> > Date: Mon, 25 Jan 2010 01:41:03 +0100
> > Subject: [PATCH] Handle UNC paths everywhere
> >
> > In Windows paths beginning with // are knows as UNC paths. They are
> > absolute paths, usually referring to a shared resource on a server.
> 
> And even a simple "cd" with them does not work.
> 
> > Examples of legal UNC paths
> >
> > 	\\hub\repos\repo
> > 	\\?\unc\hub\repos
> > 	\\?\d:\repo
> >
> > Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> > ---
> >  cache.h           |    2 +-
> >  compat/basename.c |    2 +-
> >  compat/mingw.h    |    8 +++++++-
> >  connect.c         |    2 +-
> >  git-compat-util.h |    9 +++++++++
> >  path.c            |    2 +-
> >  setup.c           |    2 +-
> >  sha1_file.c       |   20 ++++++++++++++++++++
> >  transport.c       |    2 +-
> >  9 files changed, 42 insertions(+), 7 deletions(-)
> 
> Ouch.  You should know better than to clutter non-Windows-specific parts
> with that ugly kludge.
> 
> > diff --git a/cache.h b/cache.h
> > index 767a50e..8f63640 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -648,7 +648,7 @@ int safe_create_leading_directories_const(const char
> > *path);
> >  char *enter_repo(char *path, int strict);
> >  static inline int is_absolute_path(const char *path)
> >  {
> > -	return path[0] == '/' || has_dos_drive_prefix(path);
> > +	return path[0] == '/' || has_win32_abs_prefix(path);
> 
> Why?  We can still keep the name.  Well, maybe not, see below.

I do think function names should imply something about their behaviour.

> 
> > diff --git a/compat/mingw.h b/compat/mingw.h
> > index 1b528da..d1aa8be 100644
> > --- a/compat/mingw.h
> > +++ b/compat/mingw.h
> > @@ -210,7 +210,13 @@ int winansi_fprintf(FILE *stream, const char
> > *format, ...) __attribute__((format
> >   * git specific compatibility
> >   */
> >
> > -#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] ==
> > ':') +#define has_dos_drive_prefix(path) \
> > +	(isalpha(*(path)) && (path)[1] == ':')
> 
> Why?

To avoid very long lines and format this (now) set of related macros 
uniformely.

> > +#define has_unc_prefix(path) \
> > +	(is_dir_sep((path)[0]) && is_dir_sep((path)[1]))
> > +#define has_win32_abs_prefix(path) \
> > +	(has_dos_drive_prefix(path) || has_unc_prefix(path))
> 
> "c:hello.txt" is not an absolute path.
Ok. Nevertheless that was how it was treated before, It's not relative,
either, but some quasirelative thing. has_win32_quasi_abs_prefix?

> > diff --git a/connect.c b/connect.c
> > index 7945e38..9d4556c 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -535,7 +535,7 @@ struct child_process *git_connect(int fd[2], const
> > char *url_orig,
> >  		end = host;
> >
> >  	path = strchr(end, c);
> > -	if (path && !has_dos_drive_prefix(end)) {
> > +	if (path && !has_win32_abs_prefix(end)) {
> >  		if (c == ':') {
> 
> Why?  Do we really have to exclude UNC paths from that ":" handling?

That colon is about URL-ish things... Right.

> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index ef60803..0de9dac 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -170,6 +170,15 @@ extern char *gitbasename(char *);
> >  #define has_dos_drive_prefix(path) 0
> >  #endif
> >
> > +#ifndef has_unc_prefix
> > +#define has_unc_prefix(path) 0
> > +#endif
> > +
> > +#ifndef has_win32_abs_prefix
> > +#error no abs
ouch, a leftover from trying to figure out a complation message. 

> 
> Yeah, sure.  I do have abs, thank you very much.
> 
> In general, I am _very_ worried about your patch.  It does not acknowledge
> that there is a fundamental difference between DOS drive prefixes and UNC
> paths, and not being able to "cd" to the latter is just a symptom.

As I said. Most programs including bash, but excluding cmd.exe can set the
working directory to an UNC path. I cannot fix cmd.exe and rarely use it
with git, but the patch helps even if you cannot cd from a UNC challenged
shell.

> I am also not quite sure if you can get away with having the same offset
> for both: if I have "C:\blah" and strip off "C:", I always have a
> directory separator to bounce against, whereas I do not have that if I
> strip off the two "\\" of a UNC path.  Besides, I maintain that the host
> name, and maybe even the share name, should not ever be stripped off!

When creating directoties you only strip them off for the purpose of finding
paths to mkdir. The server and share part you cannot mkdir anyway, they
must exist before attempting to create a directory, hence I skip past those  
portions. As for the \-less path beginning with a drive I'll reconsider. I did
not test that one.

-- robin

Re: [PATCH] Handle UNC paths everywhere

[Erik Faye-Lund]

On Mon, Jan 25, 2010 at 7:19 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Mon, 25 Jan 2010, Erik Faye-Lund wrote:
>
>> On Mon, Jan 25, 2010 at 6:34 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > Hi,
>> >
>> > On Mon, 25 Jan 2010, Robin Rosenberg wrote:
>> >
>> >> >From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
>> >> From: Robin Rosenberg <robin.rosenberg@dewire.com>
>> >> Date: Mon, 25 Jan 2010 01:41:03 +0100
>> >> Subject: [PATCH] Handle UNC paths everywhere
>> >>
>> >> In Windows paths beginning with // are knows as UNC paths. They are
>> >> absolute paths, usually referring to a shared resource on a server.
>> >
>> > And even a simple "cd" with them does not work.
>> >
>>
>> But it does, at least for me - both in bash and cmd.exe. I just need
>> to log on to the server first, unless it's a public share.
>
> I love it when people say "it works for me, so let's do it".
>
> _My_ _only_ instance of Windows cmd says this:
>
>        C:\Blah> cd \\localhost
>        '\\localhost'
>        CMD does not support UNC paths as current directories.
>
>        C:\Blah>
>
> So.

Actually, you're right about cmd.exe - I somehow mixed that up.
However, it works fine in bash (and simply by doing chdir() from a
normal C-program), as long as I've logged on in advance.

This applies to my Vista 64 and XP installations.

>
> Besides, the patch was not in a form where I can say that it was obviously
> fixing the issue. It was rather in a form where I would have to have set
> aside a substantial amount of time to verify that nothing undesired was
> introduced as a side effect.
>

This I can agree on. I just wanted to clear up the situation about
cd'ing. But I failed - hopefully that's corrected now.


-- 
Erik "kusma" Faye-Lund

Re: [PATCH] Handle UNC paths everywhere

[Erik Faye-Lund]

On Mon, Jan 25, 2010 at 8:37 PM, Robin Rosenberg
<robin.rosenberg@dewire.com> wrote:
> måndagen den 25 januari 2010 18.57.06 skrev  Erik Faye-Lund:
>> On Mon, Jan 25, 2010 at 6:34 PM, Johannes Schindelin
>>
>> <Johannes.Schindelin@gmx.de> wrote:
>> >
>> > And even a simple "cd" with them does not work.
>>
>> But it does, at least for me - both in bash and cmd.exe. I just needt.
>
> In cmd,exe surprises me a bit. pushd \\server\share is not the same
> as it maps a drive and then uses it to cd.
>
> -- robin
>

My guess would be that Dscho mentioned this because git internally
does a chdir() to the path that is cloned, so currently chdir'ing must
be supported for clone to work with a "local repo".

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] Handle UNC paths everywhere

[Robin Rosenberg]

måndagen den 25 januari 2010 18.34.01 skrev  Johannes Schindelin:
> I am also not quite sure if you can get away with having the same offset
> for both: if I have "C:\blah" and strip off "C:", I always have a
> directory separator to bounce against, whereas I do not have that if I
> strip off the two "\\" of a UNC path.  Besides, I maintain that the host
> name, and maybe even the share name, should not ever be stripped off!

Advices needed:

d:somedir (when cwd=d:\msysgit, == /) may be tricky to fix.
Msysgit seems confused by the syntax and treats it as d:\ 

roro@SIENA / (master)
$ cmd
Microsoft Windows [Version 5.2.3790]
(C) Copyright 1985-2003 Microsoft Corp.

D:\msysgit>exit

roro@SIENA / (master)
$ mkdir d:x
mkdir: cannot create directory `d:x': File exist

roro@SIENA / (master)
$ cd d:x

roro@SIENA /d
$ ls -l x
ls: x: No such file or directory

roro@SIENA /d
$

From that I think that even if we try to make git handle d:path, msys
will break regardless. We can fix truly absolute and normal relative paths.

-- robin

Re: [PATCH] Handle UNC paths everywhere

[Johannes Sixt]

On Montag, 25. Januar 2010, Robin Rosenberg wrote:
> In Windows paths beginning with // are knows as UNC paths. They are
> absolute paths, usually referring to a shared resource on a server.
>
> Examples of legal UNC paths
>
> 	\\hub\repos\repo
> 	\\?\unc\hub\repos
> 	\\?\d:\repo

I agree that that the problem that you are addressing needs a solution.

However, the solution is not a whole-sale replacement of 
have_dos_drive_prefix() by a function that is only a tiny bit fancier. 
Accompanying changes are needed, and perhaps more code locations need change.

> @@ -648,7 +648,7 @@ int safe_create_leading_directories_const(const char
> *path);
>  char *enter_repo(char *path, int strict);
>  static inline int is_absolute_path(const char *path)
>  {
> -	return path[0] == '/' || has_dos_drive_prefix(path);
> +	return path[0] == '/' || has_win32_abs_prefix(path);

Perhaps we need is_dir_sep(path[0]) here? But since I have not observed any 
breakage in connection with this code, I think that all callers feed only 
normalized paths (i.e. with forward slash). (Note that our getcwd() 
implementation converts backslashes to forward slashes.) This means that a 
full-fledged check is not needed.

> @@ -5,7 +5,7 @@ char *gitbasename (char *path)
>  {
>  	const char *base;
>  	/* Skip over the disk name in MSDOS pathnames. */
> -	if (has_dos_drive_prefix(path))
> +	if (has_win32_abs_prefix(path))
>  		path += 2;

This change is unnecessary; it really is only to skip an initial driver 
prefix. If you want to support \\?\X: style paths, more work is needed here  
so that you do not return X: or ? as the basename.

> +#define has_win32_abs_prefix(path) \

Do we really have to name everything "win32" when it is about Windows?

> @@ -535,7 +535,7 @@ struct child_process *git_connect(int fd[2], const char
> *url_orig,
>  		end = host;
>
>  	path = strchr(end, c);
> -	if (path && !has_dos_drive_prefix(end)) {
> +	if (path && !has_win32_abs_prefix(end)) {

This change is wrong because the check is really only about the drive prefix: 
It checks that we do not mistake c:/foo as a ssh connection to host c, 
path /foo. Yes, it does mean that on Windows we cannot have remotes to hosts 
whose name consists only of a single letter using the rcp notation (you must 
say ssh://c/foo if you mean it).

> @@ -409,7 +409,7 @@ int normalize_path_copy(char *dst, const char *src)
>  {
>  	char *dst0;
>
> -	if (has_dos_drive_prefix(src)) {
> +	if (has_win32_abs_prefix(src)) {
>  		*dst++ = *src++;
>  		*dst++ = *src++;
>  	}

Is skipping just two characters for \\ or \\?\whatever paths the right thing?

> @@ -342,7 +342,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  		die_errno("Unable to read current working directory");
>
>  	ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
> -	if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
> +	if (ceil_offset < 0 && has_win32_abs_prefix(cwd))
>  		ceil_offset = 1;

I doubt that this is correct. The purpose of this check is that "c:/" is the 
last directory that is checked (on Unix it would be "/") when path components 
are stripped from cwd. For UNC paths this must be adjusted depending on how 
you want to support \\server\share and \\?\c:\paths: You do not want to check 
whether \\server\.git or \\.git or \\?\.git are git directories.

> --- a/transport.c
> +++ b/transport.c
> @@ -797,7 +797,7 @@ static int is_local(const char *url)
>  	const char *colon = strchr(url, ':');
>  	const char *slash = strchr(url, '/');
>  	return !colon || (slash && slash < colon) ||
> -		has_dos_drive_prefix(url);
> +		has_win32_abs_prefix(url);

This check is again to not mistake c:/foo as rcp style connection. No change 
needed.

As I said, changes to other parts are perhaps also needed, most prominently, 
make_relative_path() that prompted this patch. What about 
make_absolute_path() and make_non_relative_path()?

-- Hannes

Re: [PATCH] Handle UNC paths everywhere

[Robin Rosenberg]

måndagen den 25 januari 2010 20.48.22 skrev  Erik Faye-Lund:
> On Mon, Jan 25, 2010 at 8:37 PM, Robin Rosenberg
> 
> <robin.rosenberg@dewire.com> wrote:
> > måndagen den 25 januari 2010 18.57.06 skrev  Erik Faye-Lund:
> >> On Mon, Jan 25, 2010 at 6:34 PM, Johannes Schindelin
> >>
> >> <Johannes.Schindelin@gmx.de> wrote:
> >> > And even a simple "cd" with them does not work.
> >>
> >> But it does, at least for me - both in bash and cmd.exe. I just needt.
> >
> > In cmd,exe surprises me a bit. pushd \\server\share is not the same
> > as it maps a drive and then uses it to cd.
> >
> > -- robin
> 
> My guess would be that Dscho mentioned this because git internally
> does a chdir() to the path that is cloned, so currently chdir'ing must

chdir doesn't call cmd.exe and does not suffer from cmd's limitations.

-- robin

Re: [PATCH] Handle UNC paths everywhere

[Robin Rosenberg]

måndagen den 25 januari 2010 21.07.44 skrev  Johannes Sixt:
> On Montag, 25. Januar 2010, Robin Rosenberg wrote:
> > In Windows paths beginning with // are knows as UNC paths. They are
> > absolute paths, usually referring to a shared resource on a server.
> >
> > Examples of legal UNC paths
> >
> > 	\\hub\repos\repo
> > 	\\?\unc\hub\repos
> > 	\\?\d:\repo
> 
> I agree that that the problem that you are addressing needs a solution.
> 
> However, the solution is not a whole-sale replacement of
> have_dos_drive_prefix() by a function that is only a tiny bit fancier.
> Accompanying changes are needed, and perhaps more code locations need
>  change.

I was hoping to get help in identifying these and perhaps more cases to test
than then ones I thought of at first.

> > @@ -648,7 +648,7 @@ int safe_create_leading_directories_const(const char
> > *path);
> >  char *enter_repo(char *path, int strict);
> >  static inline int is_absolute_path(const char *path)
> >  {
> > -	return path[0] == '/' || has_dos_drive_prefix(path);
> > +	return path[0] == '/' || has_win32_abs_prefix(path);
> 
> Perhaps we need is_dir_sep(path[0]) here? But since I have not observed any
> breakage in connection with this code, I think that all callers feed only
> normalized paths (i.e. with forward slash). (Note that our getcwd()
probably true.

> implementation converts backslashes to forward slashes.) This means that a
> full-fledged check is not needed.
ack.

> > @@ -5,7 +5,7 @@ char *gitbasename (char *path)
> >  {
> >  	const char *base;
> >  	/* Skip over the disk name in MSDOS pathnames. */
> > -	if (has_dos_drive_prefix(path))
> > +	if (has_win32_abs_prefix(path))
> >  		path += 2;
> 
> This change is unnecessary; it really is only to skip an initial driver
> prefix. If you want to support \\?\X: style paths, more work is needed here
> so that you do not return X: or ? as the basename.

late night hacks aren't always good.

> > +#define has_win32_abs_prefix(path) \
> 
> Do we really have to name everything "win32" when it is about Windows?
hmm

> > @@ -535,7 +535,7 @@ struct child_process *git_connect(int fd[2], const
> > char *url_orig,
> >  		end = host;
> >
> >  	path = strchr(end, c);
> > -	if (path && !has_dos_drive_prefix(end)) {
> > +	if (path && !has_win32_abs_prefix(end)) {
> 
> This change is wrong because the check is really only about the drive
>  prefix: It checks that we do not mistake c:/foo as a ssh connection to
>  host c, path /foo. Yes, it does mean that on Windows we cannot have
>  remotes to hosts whose name consists only of a single letter using the rcp
>  notation (you must say ssh://c/foo if you mean it).
right.

> > @@ -409,7 +409,7 @@ int normalize_path_copy(char *dst, const char *src)
> >  {
> >  	char *dst0;
> >
> > -	if (has_dos_drive_prefix(src)) {
> > +	if (has_win32_abs_prefix(src)) {
> >  		*dst++ = *src++;
> >  		*dst++ = *src++;
> >  	}
> 
> Is skipping just two characters for \\ or \\?\whatever paths the right
>  thing?
I shouldn't skip anything. I wasn't converting the first two \'s to //.

> > @@ -342,7 +342,7 @@ const char *setup_git_directory_gently(int
> > *nongit_ok) die_errno("Unable to read current working directory");
> >
> >  	ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
> > -	if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
> > +	if (ceil_offset < 0 && has_win32_abs_prefix(cwd))
> >  		ceil_offset = 1;
> 
> I doubt that this is correct. The purpose of this check is that "c:/" is
>  the last directory that is checked (on Unix it would be "/") when path
>  components are stripped from cwd. For UNC paths this must be adjusted
>  depending on how you want to support \\server\share and \\?\c:\paths: You
>  do not want to check whether \\server\.git or \\.git or \\?\.git are git
>  directories.

\\server\.git seems valid. Probably not a good idea, but who am I to judge?

> 
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -797,7 +797,7 @@ static int is_local(const char *url)
> >  	const char *colon = strchr(url, ':');
> >  	const char *slash = strchr(url, '/');
> >  	return !colon || (slash && slash < colon) ||
> > -		has_dos_drive_prefix(url);
> > +		has_win32_abs_prefix(url);
> 
> This check is again to not mistake c:/foo as rcp style connection. No
>  change needed.
> 
> As I said, changes to other parts are perhaps also needed, most
>  prominently, make_relative_path() that prompted this patch. What about
> make_absolute_path() and make_non_relative_path()?

Thanks for the feedback. 

-- robin

Re: [PATCH] Handle UNC paths everywhere

[Johannes Schindelin]

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

Hi,

thanks for Cc:ing me this time.

On Mon, 25 Jan 2010, Robin Rosenberg wrote:

> måndagen den 25 januari 2010 18.34.01 skrev  Johannes Schindelin:
> 
> > On Mon, 25 Jan 2010, Robin Rosenberg wrote:
> > > >From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
> > >
> > > From: Robin Rosenberg <robin.rosenberg@dewire.com>
> > > Date: Mon, 25 Jan 2010 01:41:03 +0100
> > > Subject: [PATCH] Handle UNC paths everywhere
> > >
> > > In Windows paths beginning with // are knows as UNC paths. They are
> > > absolute paths, usually referring to a shared resource on a server.
> > 
> > And even a simple "cd" with them does not work.
> > 
> > > Examples of legal UNC paths
> > >
> > > 	\\hub\repos\repo
> > > 	\\?\unc\hub\repos
> > > 	\\?\d:\repo
> > >
> > > Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> > > ---
> > >  cache.h           |    2 +-
> > >  compat/basename.c |    2 +-
> > >  compat/mingw.h    |    8 +++++++-
> > >  connect.c         |    2 +-
> > >  git-compat-util.h |    9 +++++++++
> > >  path.c            |    2 +-
> > >  setup.c           |    2 +-
> > >  sha1_file.c       |   20 ++++++++++++++++++++
> > >  transport.c       |    2 +-
> > >  9 files changed, 42 insertions(+), 7 deletions(-)
> > 
> > Ouch.  You should know better than to clutter non-Windows-specific parts
> > with that ugly kludge.

I did suspect that it would be better to handle unc paths differently than 
the dos prefix, but I did not have the time to form arguments like Hannes 
did.

So I think the first order of business is to add new code paths, rather 
than modify existing ones.  And then you can '#define 
network_path_prefix_length(name) 0' in git-compat-util.h so that on 
non-Windows platforms (i.e. our 1st class citizens), the code path can be 
optimized out.

> > > diff --git a/cache.h b/cache.h
> > > index 767a50e..8f63640 100644
> > > --- a/cache.h
> > > +++ b/cache.h
> > > @@ -648,7 +648,7 @@ int safe_create_leading_directories_const(const char
> > > *path);
> > >  char *enter_repo(char *path, int strict);
> > >  static inline int is_absolute_path(const char *path)
> > >  {
> > > -	return path[0] == '/' || has_dos_drive_prefix(path);
> > > +	return path[0] == '/' || has_win32_abs_prefix(path);
> > 
> > Why?  We can still keep the name.  Well, maybe not, see below.
> 
> I do think function names should imply something about their behaviour.

Actually, in this case, you do not even need to change anything, as Hannes 
pointed out.

> > > diff --git a/compat/mingw.h b/compat/mingw.h
> > > index 1b528da..d1aa8be 100644
> > > --- a/compat/mingw.h
> > > +++ b/compat/mingw.h
> > > @@ -210,7 +210,13 @@ int winansi_fprintf(FILE *stream, const char
> > > *format, ...) __attribute__((format
> > >   * git specific compatibility
> > >   */
> > >
> > > -#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] ==
> > > ':') +#define has_dos_drive_prefix(path) \
> > > +	(isalpha(*(path)) && (path)[1] == ':')
> > 
> > Why?
> 
> To avoid very long lines and format this (now) set of related macros 
> uniformely.

If you want your patch to go in (which you probably did not, you just 
forgot to prefix the subject with RFC or RFH), you need it to be reviewed.  
It is not a good idea to distract reviewers.  Such a change does so.

> > > +#define has_unc_prefix(path) \
> > > +	(is_dir_sep((path)[0]) && is_dir_sep((path)[1]))
> > > +#define has_win32_abs_prefix(path) \
> > > +	(has_dos_drive_prefix(path) || has_unc_prefix(path))
> > 
> > "c:hello.txt" is not an absolute path.
> Ok. Nevertheless that was how it was treated before, It's not relative,
> either, but some quasirelative thing. has_win32_quasi_abs_prefix?

No, none of this is good.  You should not even pretend that the unc prefix 
and the DOS drive prefix are the same.  Just leave the old code paths 
alone.

> > > diff --git a/git-compat-util.h b/git-compat-util.h
> > > index ef60803..0de9dac 100644
> > > --- a/git-compat-util.h
> > > +++ b/git-compat-util.h
> > > @@ -170,6 +170,15 @@ extern char *gitbasename(char *);
> > >  #define has_dos_drive_prefix(path) 0
> > >  #endif
> > >
> > > +#ifndef has_unc_prefix
> > > +#define has_unc_prefix(path) 0
> > > +#endif
> > > +
> > > +#ifndef has_win32_abs_prefix
> > > +#error no abs
> ouch, a leftover from trying to figure out a complation message. 

Thought so.

> > In general, I am _very_ worried about your patch.  It does not 
> > acknowledge that there is a fundamental difference between DOS drive 
> > prefixes and UNC paths, and not being able to "cd" to the latter is 
> > just a symptom.
> 
> As I said. Most programs including bash, but excluding cmd.exe can set 
> the working directory to an UNC path. I cannot fix cmd.exe and rarely 
> use it with git, but the patch helps even if you cannot cd from a UNC 
> challenged shell.

The point I tried to make is that you should treat UNC paths differently, 
and you can even leave the door open for non-Windows stuff.  Just call the 
function network_path_prefix_length() returning the length of the prefix.  
If it becomes standard, say, on Linux, to have support for smb:// style 
paths, we can always add support for that, too, and do not have to change 
the name yet again.

Plus, by having separate code paths, you can be sure that you do not break 
the existing ones.

And by defining network_path_prefix_length(path) to 0, the new code paths 
can be optimized out on platforms which do not support network paths.

> > I am also not quite sure if you can get away with having the same 
> > offset for both: if I have "C:\blah" and strip off "C:", I always have 
> > a directory separator to bounce against, whereas I do not have that if 
> > I strip off the two "\\" of a UNC path.  Besides, I maintain that the 
> > host name, and maybe even the share name, should not ever be stripped 
> > off!
> 
> When creating directoties you only strip them off for the purpose of 
> finding paths to mkdir. The server and share part you cannot mkdir 
> anyway, they must exist before attempting to create a directory, hence I 
> skip past those portions.

I must have missed that.  From what I saw, you treat the offset to be the 
same as for DOS drive paths: 2 characters, which is definitely not enough.

Unfortunately, this patch needs more revisions, so it will probably not 
make it into the upcoming Git for Windows, I am afraid.  But then, we do 
not need to let 3 months happen without a Git for Windows release.

Ciao,
Dscho

Re: [PATCH] Handle UNC paths everywhere

[Johannes Schindelin]

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

Hi,

On Mon, 25 Jan 2010, Robin Rosenberg wrote:

> måndagen den 25 januari 2010 18.34.01 skrev  Johannes Schindelin:
> > I am also not quite sure if you can get away with having the same offset
> > for both: if I have "C:\blah" and strip off "C:", I always have a
> > directory separator to bounce against, whereas I do not have that if I
> > strip off the two "\\" of a UNC path.  Besides, I maintain that the host
> > name, and maybe even the share name, should not ever be stripped off!
> 
> Advices needed:
> 
> d:somedir (when cwd=d:\msysgit, == /) may be tricky to fix.

This is a totally different issue from the UNC issue you brought up.  May 
I suggest to fix the UNC thing first, and if you feel inclined, take care 
of the relative DOS drive path afterwards?

The relative DOS drive path is also not something I find overly pressing, 
as I work from the Git bash, as you know, which does not translate 
POSIX-style paths into such relative DOS drive paths.

Ciao,
Dscho