Re: [PATCH] Handle UNC paths everywhere

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

måndagen den 25 januari 2010 18.34.01 skrev  Johannes Schindelin:
> Hi,
> 
> On Mon, 25 Jan 2010, Robin Rosenberg wrote:
> > >From 37a74ccd395d91e5662665ca49d7f4ec49811de0 Mon Sep 17 00:00:00 2001
> >
> > From: Robin Rosenberg <robin.rosenberg@dewire.com>
> > Date: Mon, 25 Jan 2010 01:41:03 +0100
> > Subject: [PATCH] Handle UNC paths everywhere
> >
> > In Windows paths beginning with // are knows as UNC paths. They are
> > absolute paths, usually referring to a shared resource on a server.
> 
> And even a simple "cd" with them does not work.
> 
> > Examples of legal UNC paths
> >
> > 	\\hub\repos\repo
> > 	\\?\unc\hub\repos
> > 	\\?\d:\repo
> >
> > Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> > ---
> >  cache.h           |    2 +-
> >  compat/basename.c |    2 +-
> >  compat/mingw.h    |    8 +++++++-
> >  connect.c         |    2 +-
> >  git-compat-util.h |    9 +++++++++
> >  path.c            |    2 +-
> >  setup.c           |    2 +-
> >  sha1_file.c       |   20 ++++++++++++++++++++
> >  transport.c       |    2 +-
> >  9 files changed, 42 insertions(+), 7 deletions(-)
> 
> Ouch.  You should know better than to clutter non-Windows-specific parts
> with that ugly kludge.
> 
> > diff --git a/cache.h b/cache.h
> > index 767a50e..8f63640 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -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);
> 
> Why?  We can still keep the name.  Well, maybe not, see below.

I do think function names should imply something about their behaviour.

> 
> > diff --git a/compat/mingw.h b/compat/mingw.h
> > index 1b528da..d1aa8be 100644
> > --- a/compat/mingw.h
> > +++ b/compat/mingw.h
> > @@ -210,7 +210,13 @@ int winansi_fprintf(FILE *stream, const char
> > *format, ...) __attribute__((format
> >   * git specific compatibility
> >   */
> >
> > -#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] ==
> > ':') +#define has_dos_drive_prefix(path) \
> > +	(isalpha(*(path)) && (path)[1] == ':')
> 
> Why?

To avoid very long lines and format this (now) set of related macros 
uniformely.

> > +#define has_unc_prefix(path) \
> > +	(is_dir_sep((path)[0]) && is_dir_sep((path)[1]))
> > +#define has_win32_abs_prefix(path) \
> > +	(has_dos_drive_prefix(path) || has_unc_prefix(path))
> 
> "c:hello.txt" is not an absolute path.
Ok. Nevertheless that was how it was treated before, It's not relative,
either, but some quasirelative thing. has_win32_quasi_abs_prefix?

> > diff --git a/connect.c b/connect.c
> > index 7945e38..9d4556c 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -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)) {
> >  		if (c == ':') {
> 
> Why?  Do we really have to exclude UNC paths from that ":" handling?

That colon is about URL-ish things... Right.

> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index ef60803..0de9dac 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -170,6 +170,15 @@ extern char *gitbasename(char *);
> >  #define has_dos_drive_prefix(path) 0
> >  #endif
> >
> > +#ifndef has_unc_prefix
> > +#define has_unc_prefix(path) 0
> > +#endif
> > +
> > +#ifndef has_win32_abs_prefix
> > +#error no abs
ouch, a leftover from trying to figure out a complation message. 

> 
> Yeah, sure.  I do have abs, thank you very much.
> 
> In general, I am _very_ worried about your patch.  It does not acknowledge
> that there is a fundamental difference between DOS drive prefixes and UNC
> paths, and not being able to "cd" to the latter is just a symptom.

As I said. Most programs including bash, but excluding cmd.exe can set the
working directory to an UNC path. I cannot fix cmd.exe and rarely use it
with git, but the patch helps even if you cannot cd from a UNC challenged
shell.

> I am also not quite sure if you can get away with having the same offset
> for both: if I have "C:\blah" and strip off "C:", I always have a
> directory separator to bounce against, whereas I do not have that if I
> strip off the two "\\" of a UNC path.  Besides, I maintain that the host
> name, and maybe even the share name, should not ever be stripped off!

When creating directoties you only strip them off for the purpose of finding
paths to mkdir. The server and share part you cannot mkdir anyway, they
must exist before attempting to create a directory, hence I skip past those  
portions. As for the \-less path beginning with a drive I'll reconsider. I did
not test that one.

-- robin