Re: [PATCH] Handle UNC paths everywhere

[Johannes Sixt <j6t@kdbg.org>]

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