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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html