Re: [PATCH v1/RFC 1/1] 'git clone <url> C:\cygwin\home\USER\repo' is working (again)

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 27, 2018, 01:16 UTC
Message-ID: <xmqqtvk3tj45.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181126173252.1558-1-tboegi@web.de>

tboegi@web.de writes:

Show 11 quoted lines

> Reported-By: Steven Penny <svnpenn@gmail.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>
> This is the first vesion of a patch.
> Is there a chance that you test it ?
>
> abspath.c       |  2 +-
>  compat/cygwin.c | 18 ++++++++++++++----
>  compat/cygwin.h | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+), 5 deletions(-)

I am hoping that the funny indentation above is merely an accidental touch on the delete key and not a sign of MUA eating the patch to make it unapplicable (and making it harder for those who want to test to test it).

Show 14 quoted lines

> diff --git a/compat/cygwin.c b/compat/cygwin.c
> index b9862d606d..c4a10cb5a1 100644
> --- a/compat/cygwin.c
> +++ b/compat/cygwin.c
> @@ -1,19 +1,29 @@
>  #include "../git-compat-util.h"
>  #include "../cache.h"
>  
> +int cygwin_skip_dos_drive_prefix(char **path)
> +{
> +	int ret = has_dos_drive_prefix(*path);
> +	*path += ret;
> +	return ret;
> +}

Mental note: this is exactly the same as mingw version.

I wonder if it makes the rest of the code simpler if we stripped things like /cygdrive/c here exactly the sam way as we strip C: For that, has_dos_drive_prefix() needs to know /cygdrive/[a-z], which may not be a bad thing, I guess. Let's read on.

Show 9 quoted lines

>  int cygwin_offset_1st_component(const char *path)
>  {
> -	const char *pos = path;
> +	char *pos = (char *)path;
> +
>  	/* unc paths */
> -	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> +	if (!skip_dos_drive_prefix(&pos) &&
> +			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {

When given C:\foo\bar, this strips prefix to leave \foo\bar in pos and then realizes that it cannot be unc path (because it has dos prefix) and goes on. What is returned from the function is "\foo\bar" + 1 - path, i.e. the offset in the original "C:\foo\bar" string of the 'f' in "foo", i.e. 3.

When given \foo\bar, pos stays the same as path, and it skips the first backslash and returns the offset in the original string of the 'f' in "foo", i.e. 1.

Both cases return the moreal equivalent --- the offset of the first component 'foo'. So this looks correct for these two cases.

>  		/* skip server name */
> -		pos = strchr(pos + 2, '/');
> +		pos = strpbrk(pos + 2, "\\/");

This is to allow \\server\path in addition to //server/path; the original looked only for '/' with strchr but we now look for either '/' or '\', whichever comes earlier. Both helpers return NULL when they find no separator, so we should be able to handle the returned pos from here on the same way as the original code.

Show 7 quoted lines

>  		if (!pos)
>  			return 0; /* Error: malformed unc path */
>  
>  		do {
>  			pos++;
> -		} while (*pos && pos[0] != '/');
> +		} while (*pos && !is_dir_sep(*pos));

And whenever we looked for '/', we consider '\' its equivalent.

>  	}
> +
>  	return pos + is_dir_sep(*pos) - path;
>  }

Looks good so far.

Wait, did I just waste time by not looking at mingw.c version? I suspect this would be exactly the same ;-)

Show 7 quoted lines

> diff --git a/compat/cygwin.h b/compat/cygwin.h
> index 8e52de4644..46f29c0a90 100644
> --- a/compat/cygwin.h
> +++ b/compat/cygwin.h
> @@ -1,2 +1,34 @@
> +#define has_dos_drive_prefix(path) \
> +	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)

Metanl note: this also looks the same as mingw version.

> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component
> +

So, my real questions are

 - Is there a point in having cygwin specific variant of these, or
   can we just borrow from mingw version (with some refactoring)?
   Is there a point in doing so (e.g. if mingw plans to move to
   reject forward slashes, attempting to share is pointless).

 - Would it make it better (or worse) to treat the /cygdrive/c thing
   as another way to spell dos-drive-prefix?  If the answer is "it
   is a good idea", then that answers the previous question
   automatically (we cannot gain much by sharing, as mingw side
   won't want to treat /cygdrive/c any differently).