Re: [PATCH] Handle UNC paths everywhere
From: Johannes Sixt <hidden>
Date: 2016-06-15 22:48:06
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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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).
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
--- 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