Re: [PATCH] Handle UNC paths everywhere

måndagen den 25 januari 2010 21.07.44 skrev  Johannes Sixt:

On Montag, 25. Januar 2010, Robin Rosenberg wrote:

quoted

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.

quoted

@@ -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.

quoted

@@ -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.

quoted

+#define has_win32_abs_prefix(path) \

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

hmm

quoted

@@ -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.

quoted

@@ -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 //.

quoted

@@ -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?

quoted

--- 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