Re: [PATCH] Handle UNC paths everywhere

[Robin Rosenberg <robin.rosenberg@dewire.com>]

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