Re: [PATCH] Handle UNC paths everywhere

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:

quoted

On Mon, 25 Jan 2010, Robin Rosenberg wrote:

quoted

quoted

From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001

From: Robin Rosenberg <redacted>
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.

quoted

Examples of legal UNC paths

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

Signed-off-by: Robin Rosenberg <redacted>
---
 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.

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

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.

quoted

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.

quoted

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