Cygwin Git with Windows paths

Cygwin Git with Windows paths

[Steven Penny]

Cygwin programs can handle Unix form paths:

    $ ls /var
    cache  lib  log  run  tmp

and also Windows form paths:

    $ ls 'C:\cygwin64\var'
    cache  lib  log  run  tmp

However current Cygwin Git cannot:

    $ git clone git://github.com/benhoyt/goawk 'C:\cygwin64\tmp\goawk'
    Cloning into 'C:\cygwin64\tmp\goawk'...
    fatal: Invalid path '/home/Steven/C:\cygwin64\tmp\goawk': No such file or
    directory

It seems the problem is that Git thinks the Windows form path is relative
because it does not start with "/". A Git Bisect reveals this:

05b458c104708141d2fad211d79703b3b99cc5a8 is the first bad commit
commit 05b458c104708141d2fad211d79703b3b99cc5a8
Author: Brandon Williams <bmwill@google.com>
Date:   Mon Dec 12 10:16:52 2016 -0800

    real_path: resolve symlinks by hand

    The current implementation of real_path uses chdir() in order to resolve
    symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
    process as a whole and not just an individual thread.  Instead perform
    the symlink resolution by hand so that the calls to chdir() can be
    removed, making real_path one step closer to being reentrant.

    Signed-off-by: Brandon Williams <bmwill@google.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

This causes problems for any non-Cygwin tools that might call Git:

http://github.com/golang/go/issues/23155

Re: Cygwin Git with Windows paths

[Torsten Bögershausen]

On Sun, Nov 18, 2018 at 07:21:58AM -0800, Steven Penny wrote:
> Cygwin programs can handle Unix form paths:
> 
>    $ ls /var
>    cache  lib  log  run  tmp
> 
> and also Windows form paths:
> 
>    $ ls 'C:\cygwin64\var'
>    cache  lib  log  run  tmp
> 
> However current Cygwin Git cannot:
> 
>    $ git clone git://github.com/benhoyt/goawk 'C:\cygwin64\tmp\goawk'
>    Cloning into 'C:\cygwin64\tmp\goawk'...
>    fatal: Invalid path '/home/Steven/C:\cygwin64\tmp\goawk': No such file or
>    directory
> 
> It seems the problem is that Git thinks the Windows form path is relative
> because it does not start with "/". A Git Bisect reveals this:
> 
> 05b458c104708141d2fad211d79703b3b99cc5a8 is the first bad commit
> commit 05b458c104708141d2fad211d79703b3b99cc5a8
> Author: Brandon Williams <bmwill@google.com>
> Date:   Mon Dec 12 10:16:52 2016 -0800
> 
>    real_path: resolve symlinks by hand
> 
>    The current implementation of real_path uses chdir() in order to resolve
>    symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
>    process as a whole and not just an individual thread.  Instead perform
>    the symlink resolution by hand so that the calls to chdir() can be
>    removed, making real_path one step closer to being reentrant.
> 
>    Signed-off-by: Brandon Williams <bmwill@google.com>
>    Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> This causes problems for any non-Cygwin tools that might call Git:
> 
> http://github.com/golang/go/issues/23155
> 

Thanks for the report
It seams as if "C:" is not recognized as an absolute path under
cygwin.
May be it should ?

Does the following help ? (fully untested)


diff --git a/compat/cygwin.h b/compat/cygwin.h
index 8e52de4644..12814e1edb 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -1,2 +1,4 @@
 int cygwin_offset_1st_component(const char *path);
 #define offset_1st_component cygwin_offset_1st_component
+#define has_dos_drive_prefix(path) \
+       (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)

Re: Cygwin Git with Windows paths

[Steven Penny]

On Sun, Nov 18, 2018 at 9:41 AM Torsten Bögershausen wrote:
> Thanks for the report
> It seams as if "C:" is not recognized as an absolute path under
> cygwin.
> May be it should ?
>
> Does the following help ? (fully untested)

that looks promising - but its not getting pulled in where it needs to be.
perhaps another file need to be modified to utilize that macro?

Re: Cygwin Git with Windows paths

[Torsten Bögershausen]

On Sun, Nov 18, 2018 at 10:23:19AM -0600, Steven Penny wrote:
> On Sun, Nov 18, 2018 at 9:41 AM Torsten Bögershausen wrote:
> > Thanks for the report
> > It seams as if "C:" is not recognized as an absolute path under
> > cygwin.
> > May be it should ?
> >
> > Does the following help ? (fully untested)
> 
> that looks promising - but its not getting pulled in where it needs to be.
> perhaps another file need to be modified to utilize that macro?

The macro should be utilized, see git-compat-util.h:

#if defined(__CYGWIN__)
#include "compat/cygwin.h"
#endif

And further down

#ifndef has_dos_drive_prefix
static inline int git_has_dos_drive_prefix(const char *path)
{
	return 0;
}
#define has_dos_drive_prefix git_has_dos_drive_prefix
#endif

#ifndef skip_dos_drive_prefix
static inline int git_skip_dos_drive_prefix(char **path)
{
	return 0;
}
--------------------
But it may be that we need to pull in more stuff, similar to mingw,
to get the C: stuff working, see
"skip_dos_drive_prefix"

And it may even be that we need a special handling for the "\" to be treated
as "/".

If you implement "skip_dos_drive_prefix" similar to mingw,
(rename mingw into cygwin) does

git clone <source> C:/my/dir/
work ?

That would be a progress, I think.

Re: Cygwin Git with Windows paths

[Steven Penny]

On Sun, Nov 18, 2018 at 11:15 AM Torsten Bögershausen wrote:
> But it may be that we need to pull in more stuff, similar to mingw,
> to get the C: stuff working, see
> "skip_dos_drive_prefix"
>
> And it may even be that we need a special handling for the "\" to be treated
> as "/".
>
> If you implement "skip_dos_drive_prefix" similar to mingw,
> (rename mingw into cygwin) does
>
> git clone <source> C:/my/dir/
> work ?

I added this to "compat/cygwin.h":

    #define has_dos_drive_prefix(path) \
      (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
    int mingw_skip_dos_drive_prefix(char **path);
    #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix

and added this to "compat/cygwin.c":

    int mingw_skip_dos_drive_prefix(char **path) {
      int ret = has_dos_drive_prefix(*path);
      *path += ret;
      return ret;
    }

but still, these dont work:

    git clone <source> C:/my/dir
    git clone <source> 'C:\my\dir'

Re: Cygwin Git with Windows paths

[Torsten Bögershausen]

On Sun, Nov 18, 2018 at 11:34:04AM -0600, Steven Penny wrote:
> On Sun, Nov 18, 2018 at 11:15 AM Torsten Bögershausen wrote:
> > But it may be that we need to pull in more stuff, similar to mingw,
> > to get the C: stuff working, see
> > "skip_dos_drive_prefix"
> >
> > And it may even be that we need a special handling for the "\" to be treated
> > as "/".
> >
> > If you implement "skip_dos_drive_prefix" similar to mingw,
> > (rename mingw into cygwin) does
> >
> > git clone <source> C:/my/dir/
> > work ?
> 
> I added this to "compat/cygwin.h":
> 
>     #define has_dos_drive_prefix(path) \
>       (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
>     int mingw_skip_dos_drive_prefix(char **path);
>     #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
> 
> and added this to "compat/cygwin.c":
> 
>     int mingw_skip_dos_drive_prefix(char **path) {
>       int ret = has_dos_drive_prefix(*path);
>       *path += ret;
>       return ret;
>     }
> 
> but still, these dont work:
> 
>     git clone <source> C:/my/dir
>     git clone <source> 'C:\my\dir'

Thanks for testing.
It looks as if there is more work to be done then just a simple patch.

My last question for today:
Does 

git clone <source> '/cgdrive/c/my/dir'

work ?

Re: Cygwin Git with Windows paths

[Steven Penny]

On Sun, Nov 18, 2018 at 12:28 PM Torsten Bögershausen wrote:
> Thanks for testing.
> It looks as if there is more work to be done then just a simple patch.
>
> My last question for today:
> Does
>
> git clone <source> '/cgdrive/c/my/dir'
>
> work ?

yes - these all work and resolve to same path:

   git clone <source> /tmp/goawk
   git clone <source> /cygdrive/c/cygwin64/tmp/goawk
   git clone <source> /proc/cygdrive/c/cygwin64/tmp/goawk

however i would caution that any fix should not rely on "C:", as users are
allowed to install to other volumes such as "D:". Perhaps a better solution
would be for Git to just take the path as is, rather than converting it to an
absolute path?

Re: Cygwin Git with Windows paths

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> And it may even be that we need a special handling for the "\" to be treated
> as "/".

I do not do Windows, but is_dir_sep() needs to be tweaked if you
want to do that.

RE: Cygwin Git with Windows paths

[Randall S. Becker]

> -----Original Message-----
> From: git-owner@vger.kernel.org <git-owner@vger.kernel.org> On Behalf Of
> Junio C Hamano
> Sent: November 18, 2018 19:07
> To: Torsten Bögershausen <tboegi@web.de>
> Cc: Steven Penny <svnpenn@gmail.com>; git@vger.kernel.org
> Subject: Re: Cygwin Git with Windows paths
> 
> Torsten Bögershausen <tboegi@web.de> writes:
> 
> > And it may even be that we need a special handling for the "\" to be
> > treated as "/".
> 
> I do not do Windows, but is_dir_sep() needs to be tweaked if you want to do
> that.

Heavy Cygwin user here. It is used in my environment for cross-compilation. Everything should be done using / separators in Cygwin, not \. So /cygdrive/c, /cygdrive/d always prefaces the path rather than C:\ or D:\, which won't parse. It is, essentially, a bash environment, including that git completions work properly. Backslash ends up doing what it would in bash.

Re: Cygwin Git with Windows paths

[Junio C Hamano]

"Randall S. Becker" <rsbecker@nexbridge.com> writes:

>> Torsten Bögershausen <tboegi@web.de> writes:
>> 
>> > And it may even be that we need a special handling for the "\" to be
>> > treated as "/".
>> 
>> I do not do Windows, but is_dir_sep() needs to be tweaked if you want to do
>> that.
>
> Heavy Cygwin user here. It is used in my environment for
> cross-compilation. Everything should be done using / separators in
> Cygwin, not \. So /cygdrive/c, /cygdrive/d always prefaces the
> path rather than C:\ or D:\, which won't parse. It is,
> essentially, a bash environment, including that git completions
> work properly. Backslash ends up doing what it would in bash.

In short, in your opinion, the original message in this thread
expresses an invalid wish, as C:\path\to\dir\ is not a valid way to
spell the path to the directory, and it should be written as
/cygdrive/c/path/to/dir instead?

How well does this argument work in the real world, when another
claim in the original message

    This causes problems for any non-Cygwin tools that might call Git:

    http://github.com/golang/go/issues/23155

is taken into account, I wonder, though?

Re: Cygwin Git with Windows paths

[Torsten Bögershausen]

On 2018-11-19 04:33, Junio C Hamano wrote:
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
>>> Torsten Bögershausen <tboegi@web.de> writes:
>>>
>>>> And it may even be that we need a special handling for the "\" to be
>>>> treated as "/".
>>>
>>> I do not do Windows, but is_dir_sep() needs to be tweaked if you want to do
>>> that.
>>
>> Heavy Cygwin user here. It is used in my environment for
>> cross-compilation. Everything should be done using / separators in
>> Cygwin, not \. So /cygdrive/c, /cygdrive/d always prefaces the
>> path rather than C:\ or D:\, which won't parse. It is,
>> essentially, a bash environment, including that git completions
>> work properly. Backslash ends up doing what it would in bash.
> 
> In short, in your opinion, the original message in this thread
> expresses an invalid wish, as C:\path\to\dir\ is not a valid way to
> spell the path to the directory, and it should be written as
> /cygdrive/c/path/to/dir instead?
> 
> How well does this argument work in the real world, when another
> claim in the original message
> 
>     This causes problems for any non-Cygwin tools that might call Git:
> 
>     http://github.com/golang/go/issues/23155
> 
> is taken into account, I wonder, though?
> 


Back to the original email, where the path embedded in ''
and the bash does not interpret the "\", I think.

>   $ git clone git://github.com/benhoyt/goawk 'C:\cygwin64\tmp\goawk'
>   Cloning into 'C:\cygwin64\tmp\goawk'...
>   fatal: Invalid path '/home/Steven/C:\cygwin64\tmp\goawk': No such file or
>   directory

>It seems the problem is that Git thinks the Windows form path is relative
>because it does not start with "/".

>A Git Bisect reveals this:
>05b458c104708141d2fad211d79703b3b99cc5a8 is the first bad commit
>commit 05b458c104708141d2fad211d79703b3b99cc5a8
>Author: Brandon Williams <bmwill@google.com>
>Date:   Mon Dec 12 10:16:52 2016 -0800


The first question is, does this work under Git for Windows ?

Looking into 05b458c104708141d2fad, it seems as if the following functions
need to be "overridden" for cygwin, similar as we do it for mingw:
 is_dir_sep()
 offset_1st_component()
 find_last_dir_sep()


If nothing works,
it may help to add some fprintf(stderr,...) in the functions used
by 05b458c104708141d2f:

strip_last_component(),
get_next_component()
real_path_internal()

Re: Cygwin Git with Windows paths

[Steven Penny]

On Sun, Nov 18, 2018 at 11:21 PM Torsten Bögershausen wrote:
> If nothing works,
> it may help to add some fprintf(stderr,...) in the functions used
> by 05b458c104708141d2f:
>
> strip_last_component(),
> get_next_component()
> real_path_internal()

I didnt see any "real_path_internal" in the current codebase - however i added
some "printf" to the other 2 and got this:

$ git clone git://github.com/benhoyt/goawk 'C:\cygwin64\tmp\goawk'
get_next_component, next, []
get_next_component, remaining, [C:\cygwin64\tmp\goawk]
Cloning into 'C:\cygwin64\tmp\goawk'...
get_next_component, next, []
get_next_component, remaining, [C:\cygwin64\tmp\goawk/.git]
fatal: Invalid path '/usr/local/cache/git/C:\cygwin64\tmp\goawk': No such file
or directory

Re: Cygwin Git with Windows paths

[Torsten Bögershausen]

On 20.11.18 01:17, Steven Penny wrote:
> On Sun, Nov 18, 2018 at 11:21 PM Torsten Bögershausen wrote:
>> If nothing works,
>> it may help to add some fprintf(stderr,...) in the functions used
>> by 05b458c104708141d2f:
>>
>> strip_last_component(),
>> get_next_component()
>> real_path_internal()
> 
> I didnt see any "real_path_internal" in the current codebase - however i added
> some "printf" to the other 2 and got this:
> 
> $ git clone git://github.com/benhoyt/goawk 'C:\cygwin64\tmp\goawk'
> get_next_component, next, []
> get_next_component, remaining, [C:\cygwin64\tmp\goawk]
> Cloning into 'C:\cygwin64\tmp\goawk'...
> get_next_component, next, []
> get_next_component, remaining, [C:\cygwin64\tmp\goawk/.git]
> fatal: Invalid path '/usr/local/cache/git/C:\cygwin64\tmp\goawk': No such file
> or directory
> 

Could you please post a "git diff" of your instrumented code,
so that I/we can follow the debugging, especially what the printouts mean?

I think we need to understand what is going on in abspath.c

Re: Cygwin Git with Windows paths

[Steven Penny]

On Tue, Nov 20, 2018 at 4:36 AM Torsten Bögershausen wrote:
> Could you please post a "git diff" of your instrumented code,
> so that I/we can follow the debugging, especially what the printouts mean?
>
> I think we need to understand what is going on in abspath.c
>

diff --git a/abspath.c b/abspath.c
index 9857985..09548e5 100644
--- a/abspath.c
+++ b/abspath.c
@@ -14,6 +14,7 @@ int is_directory(const char *path)
 /* removes the last path component from 'path' except if 'path' is root */
 static void strip_last_component(struct strbuf *path)
 {
+       printf("strip_last_component, path, [%s]\n", path->buf);
        size_t offset = offset_1st_component(path->buf);
        size_t len = path->len;

@@ -30,6 +31,8 @@ static void strip_last_component(struct strbuf *path)
 /* get (and remove) the next component in 'remaining' and place it in 'next' */
 static void get_next_component(struct strbuf *next, struct strbuf *remaining)
 {
+       printf("get_next_component, next, [%s]\n", next->buf);
+       printf("get_next_component, remaining, [%s]\n", remaining->buf);
        char *start = NULL;
        char *end = NULL;

RE: Cygwin Git with Windows paths

[Randall S. Becker]

On November 18, 2018 22:33, Junio C Hamano wrote:
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> >> Torsten Bögershausen <tboegi@web.de> writes:
> >>
> >> > And it may even be that we need a special handling for the "\" to
> >> > be treated as "/".
> >>
> >> I do not do Windows, but is_dir_sep() needs to be tweaked if you want
> >> to do that.
> >
> > Heavy Cygwin user here. It is used in my environment for
> > cross-compilation. Everything should be done using / separators in
> > Cygwin, not \. So /cygdrive/c, /cygdrive/d always prefaces the path
> > rather than C:\ or D:\, which won't parse. It is, essentially, a bash
> > environment, including that git completions work properly. Backslash
> > ends up doing what it would in bash.
> 
> In short, in your opinion, the original message in this thread expresses an
> invalid wish, as C:\path\to\dir\ is not a valid way to spell the path to the
> directory, and it should be written as /cygdrive/c/path/to/dir instead?
> 
> How well does this argument work in the real world, when another claim in
> the original message
> 
>     This causes problems for any non-Cygwin tools that might call Git:
> 
>     http://github.com/golang/go/issues/23155
> 
> is taken into account, I wonder, though?

The solution to this that I ended up with is a blend of many different implementations of git (3) on my dev box. EGit in ECLIPSE, standard windows git for working in a CMD prompt, the Cygwin git in Cygwin bash. On another dev box I also have git bash and Ming, which make things easier, but I have to work in Cygwin for some subsystems. I end up using relative paths with / instead of \ in all cases, and git seems happy, except for /cygdrive/c. Absolute Windows and Cygwin paths simply do not work consistently, from my experience from Cygwin because of the way bash passes arguments to non-cygwin tools. You need to be very careful and properly escape "\" to do so, and account for cygwin drive handling otherwise. So a non-cygwin tool from Cygwin can process c:\ but Cygwin itself will not. My own expectations are that git built for Cygwin would understand this, but git built for Windows would not, and that I should account for this through my PATH, selecting Cygwin git in Cygwin and Windows git elsewhere. Reality may not reflect this, so I use relative paths in all cases (non-reality being that the git port for Cygwin is different than the git port for Windows, which it does not appear to be).

Cheers,
Randall

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

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

A regression for cygwin users was introduced with commit 05b458c,
 "real_path: resolve symlinks by hand".

In the the commit message we read:
  The current implementation of real_path uses chdir() in order to resolve
  symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
  process as a whole...

The old (and non-thread-save) OS calls chdir()/pwd() had been
replaced by a string operation.
The cygwin layer "knows" that "C:\cygwin" is an absolute path,
but the new string operation does not.

"git clone <url> C:\cygwin\home\USER\repo" fails like this:
fatal: Invalid path '/home/USER/repo/C:\cygwin\home\USER\repo'

The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]

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(-)

diff --git a/abspath.c b/abspath.c
index 9857985329..77a281f789 100644
--- a/abspath.c
+++ b/abspath.c
@@ -55,7 +55,7 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
 
 	strbuf_reset(resolved);
 	strbuf_add(resolved, remaining->buf, offset);
-#ifdef GIT_WINDOWS_NATIVE
+#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
 	convert_slashes(resolved->buf);
 #endif
 	strbuf_remove(remaining, 0, offset);
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;
+}
+
 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])) {
 		/* skip server name */
-		pos = strchr(pos + 2, '/');
+		pos = strpbrk(pos + 2, "\\/");
 		if (!pos)
 			return 0; /* Error: malformed unc path */
 
 		do {
 			pos++;
-		} while (*pos && pos[0] != '/');
+		} while (*pos && !is_dir_sep(*pos));
 	}
+
 	return pos + is_dir_sep(*pos) - path;
 }
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)
+
+
+int cygwin_offset_1st_component(const char *path);
+#define offset_1st_component cygwin_offset_1st_component
+
+
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+int cygwin_skip_dos_drive_prefix(char **path);
+#define skip_dos_drive_prefix cygwin_skip_dos_drive_prefix
+static inline int cygwin_is_dir_sep(int c)
+{
+	return c == '/' || c == '\\';
+}
+#define is_dir_sep cygwin_is_dir_sep
+static inline char *cygwin_find_last_dir_sep(const char *path)
+{
+	char *ret = NULL;
+	for (; *path; ++path)
+		if (is_dir_sep(*path))
+			ret = (char *)path;
+	return ret;
+}
+static inline void convert_slashes(char *path)
+{
+	for (; *path; path++)
+		if (*path == '\\')
+			*path = '/';
+}
+#define find_last_dir_sep cygwin_find_last_dir_sep
 int cygwin_offset_1st_component(const char *path);
 #define offset_1st_component cygwin_offset_1st_component
-- 
2.19.0.271.gfe8321ec05

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

[Steven Penny]

On Mon, Nov 26, 2018 at 11:32 AM wrote:
> This is the first vesion of a patch.
> Is there a chance that you test it ?

I can confirm that this fixes the issue.

Thank you!

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

[Junio C Hamano]

tboegi@web.de writes:

> 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).

> 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.

>  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.

>  		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 ;-)

> 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).

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

[Steven Penny]

On Mon, Nov 26, 2018 at 7:16 PM Junio C Hamano wrote:
> 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.

With full paths, Cygwin can traverse drives:

    $ cd 'C:\Users'
    $ pwd
    /cygdrive/c/Users

    $ cd 'D:\Testing'
    $ pwd
    /cygdrive/d/Testing

If you strip the drive, you can still navigate within the same drive:

    $ cd 'C:\Users'
    $ pwd
    /cygdrive/c/Users

    $ cd '\Windows'
    $ pwd
    /cygdrive/c/Windows

but you can no longer traverse drives:

    $ cd '\Testing'
    sh: cd: \Testing: No such file or directory

So a good first question for me would be: why are we stripping "C:" or similar
in the first place?

>  - 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).

I would say these could be merged into a "win.h" or similar. Cygwin typically
leans toward the "/unix/style" while MINGW has been more tolerant of
"C:\Windows\Style" and "C:/Mixed/Style" paths, i dont see that changing.

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

[Junio C Hamano]

Steven Penny <svnpenn@gmail.com> writes:

> If you strip the drive, you can still navigate within the same drive:
>
>     $ cd 'C:\Users'
>     $ pwd
>     /cygdrive/c/Users
>
>     $ cd '\Windows'
>     $ pwd
>     /cygdrive/c/Windows
>
> but you can no longer traverse drives:
>
>     $ cd '\Testing'
>     sh: cd: \Testing: No such file or directory

Sorry, but I fail to see the point the last example wants to make.
If it were

    $ cd /cygdrive/d/Testing
    $ cd /cygdrive/c/Users
    $ cd ../../d/Testing

and the last step fails, then I would suspect it would make sense to
treat /cygdrive/$X exactly like how we would treat $C:, because

    $ cd C:Users
    $ cd ../D:Testing

would not make sense, either, which is an indication that these two
are quite similar.  On the other hand, if "cd ../../d/Testing" above
does not fail and does what non-DOS users would expect, then that
strongly argues that treating /cygdrive/$X any specially is a mistake.

> So a good first question for me would be: why are we stripping "C:" or similar
> in the first place?

Sorry, but I do not see the connection to this question and the
above example.  The reason why we strip C: is because the letter
that comes after that colon determines if we are talking about
absolute path (in other words, the current directory does not play a
role in determining which directory the path refers to), unlike the
POSIX codepath where it is always the first letter in the pathname.

C:\Users is a directory whose name is Users at the top level of the
C: drive. C0\Users tells us that in the current directory, there is
a directory whose name is C0 and in it, there is a filesystem entity
whose name is Users.  So the colon that follows an alpha (in this
case, C:) is quite special, compared to other letters (in this
example, I used '0' to contrast its effect with ':').  So it is very
understandable why we want to have has_dos_prefix() and
skip_dos_prefix().

> I would say these could be merged into a "win.h" or similar. Cygwin typically
> leans toward the "/unix/style" while MINGW has been more tolerant of
> "C:\Windows\Style" and "C:/Mixed/Style" paths, i dont see that changing.

I'd defer to Windows folks to decide if a unified win.h is a good
idea.

Thanks.

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

[Steven Penny]

On Mon, Nov 26, 2018 at 11:23 PM Junio C Hamano wrote:
> Sorry, but I do not see the connection to this question and the
> above example.  The reason why we strip C: is because the letter
> that comes after that colon determines if we are talking about
> absolute path (in other words, the current directory does not play a
> role in determining which directory the path refers to), unlike the
> POSIX codepath where it is always the first letter in the pathname.

while it is true that "C:" and similar do not have a bearing on a path being
absolute versus relative, it does have a bearing on what drive the entry is to
be found.

That is to say "C:\tmp\file.txt" does not equal "D:\tmp\file.txt".

Starting with an absolute path like "C:\tmp\file.txt", after stripping that
would yield "\tmp\file.txt" or "/tmp/file.txt". Starting with a relative path
like "C:tmp\file.txt", after stripping that would yield "tmp\file.txt" or
"tmp/file.txt".

However in all cases we have lost the concept of what drive the file is located
on, and Windows will assume the file exists on the current drive.

So I would expect "git clone URL D:\tmp" to fail if the current directory is on
"C:". Upon testing cross drive clones work fine though with this patch, so maybe
the drive is added back at another place in the code.

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

[Johannes Schindelin]

Hi Junio,

On Tue, 27 Nov 2018, Junio C Hamano wrote:

> Steven Penny <svnpenn@gmail.com> writes:
> 
> > If you strip the drive, you can still navigate within the same drive:
> >
> >     $ cd 'C:\Users'
> >     $ pwd
> >     /cygdrive/c/Users
> >
> >     $ cd '\Windows'
> >     $ pwd
> >     /cygdrive/c/Windows
> >
> > but you can no longer traverse drives:
> >
> >     $ cd '\Testing'
> >     sh: cd: \Testing: No such file or directory
> 
> Sorry, but I fail to see the point the last example wants to make.

I agree. For me, the real test is this:

me@work ~
$ cd /cygdrive

me@work /cygdrive
$ ls
c  d

So `/cygdrive` *is* a valid directory in Cygwin.

> > I would say these could be merged into a "win.h" or similar. Cygwin typically
> > leans toward the "/unix/style" while MINGW has been more tolerant of
> > "C:\Windows\Style" and "C:/Mixed/Style" paths, i dont see that changing.
> 
> I'd defer to Windows folks to decide if a unified win.h is a good
> idea.

We already have such a thing, but it is not just `win.h`, it is
`compat/win32/`. I would think that the best idea would be to move the
MINGW variants to `compat/win32/path-utils.c` and declare them in
`compat/win32/path-utils.h`, renaming them from `mingw_*()` to
`win32_*()`.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Sorry, but I fail to see the point the last example wants to make.
>
> I agree. For me, the real test is this:
>
> me@work ~
> $ cd /cygdrive
>
> me@work /cygdrive
> $ ls
> c  d
>
> So `/cygdrive` *is* a valid directory in Cygwin.

That supports the code that does not special case a path that begins
with /cygdrive/ and simply treats it as a full path and freely use
relative path, I guess.  Very good point.

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

[J.H. van de Water]

> > me@work /cygdrive
> > $ ls
> > c  d
> >
> > So `/cygdrive` *is* a valid directory in Cygwin.
> 
> That supports the code that does not special case a path that begins
> with /cygdrive/ and simply treats it as a full path and freely use
> relative path, I guess.  Very good point.

Please read

    https://cygwin.com/cygwin-ug-net/using.html#cygdrive
    ( The cygdrive path prefix )

.... you can access arbitary drives on your system by using the cygdrive path
prefix. The default value for this prefix is /cygdrive ...
....

The cygdrive prefix is a >>> virtual directory <<< under which all drives on
a system are subsumed ...
....

The cygdrive prefix may be CHANGED in the fstab file as outlined above !!!!!
....

To simplify scripting, Cygwin also provides a /proc/cygdrive symlink, ...

=====

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

[Johannes Schindelin]

Hi J.H.,

On Wed, 28 Nov 2018, J.H. van de Water wrote:

> > > me@work /cygdrive
> > > $ ls
> > > c  d
> > >
> > > So `/cygdrive` *is* a valid directory in Cygwin.
> > 
> > That supports the code that does not special case a path that begins
> > with /cygdrive/ and simply treats it as a full path and freely use
> > relative path, I guess.  Very good point.
> 
> Please read
> 
>     https://cygwin.com/cygwin-ug-net/using.html#cygdrive
>     ( The cygdrive path prefix )
> 
> .... you can access arbitary drives on your system by using the cygdrive path
> prefix. The default value for this prefix is /cygdrive ...
> ....
> 
> The cygdrive prefix is a >>> virtual directory <<< under which all drives on
> a system are subsumed ...
> ....
> 
> The cygdrive prefix may be CHANGED in the fstab file as outlined above !!!!!
> ....
> 
> To simplify scripting, Cygwin also provides a /proc/cygdrive symlink, ...
> 
> =====

That's all very interesting, but I fail to see the relevance with regards
to the issue at hand, namely whether to special-case `/cygdrive` as a
special prefix that cannot be treated as directory in Git.

I still maintain that it should not be special-cased, no matter whether it
is a virtual directory or whether it can be renamed to `/jh-likes-cygwin`
or whatever.

Ciao,
Dscho

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

[Houder]

On 2018-11-28 09:46, Johannes Schindelin wrote:
> Hi J.H.,
> 
> On Wed, 28 Nov 2018, J.H. van de Water wrote:
> 
>> > > me@work /cygdrive
>> > > $ ls
>> > > c  d
>> > >
>> > > So `/cygdrive` *is* a valid directory in Cygwin.
>> >
>> > That supports the code that does not special case a path that begins
>> > with /cygdrive/ and simply treats it as a full path and freely use
>> > relative path, I guess.  Very good point.
>> 
>> Please read
>> 
>>     https://cygwin.com/cygwin-ug-net/using.html#cygdrive
>>     ( The cygdrive path prefix )
>> 
>> .... you can access arbitary drives on your system by using the 
>> cygdrive path
>> prefix. The default value for this prefix is /cygdrive ...
>> ....
>> 
>> The cygdrive prefix is a >>> virtual directory <<< under which all 
>> drives on
>> a system are subsumed ...
>> ....
>> 
>> The cygdrive prefix may be CHANGED in the fstab file as outlined above 
>> !!!!!
>> ....
>> 
>> To simplify scripting, Cygwin also provides a /proc/cygdrive symlink, 
>> ...
>> 
>> =====
> 
> That's all very interesting, but I fail to see the relevance with 
> regards
> to the issue at hand, namely whether to special-case `/cygdrive` as a
> special prefix that cannot be treated as directory in Git.
> 
> I still maintain that it should not be special-cased, no matter whether 
> it
> is a virtual directory or whether it can be renamed to 
> `/jh-likes-cygwin`
> or whatever.

Ok. Sorry about the noise.

 From your post I got the impression that you believed that there will 
always
be a directory called /cygdrive on Cygwin.

My point: it can have a different name.

Regards,

Henri

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

[Johannes Schindelin]

Hi J.H.

On Wed, 28 Nov 2018, Houder wrote:

> On 2018-11-28 09:46, Johannes Schindelin wrote:
> > 
> > On Wed, 28 Nov 2018, J.H. van de Water wrote:
> > 
> > > > > me@work /cygdrive
> > > > > $ ls
> > > > > c  d
> > > > >
> > > > > So `/cygdrive` *is* a valid directory in Cygwin.
> > > >
> > > > That supports the code that does not special case a path that begins
> > > > with /cygdrive/ and simply treats it as a full path and freely use
> > > > relative path, I guess.  Very good point.
> > > 
> > > Please read
> > > 
> > >     https://cygwin.com/cygwin-ug-net/using.html#cygdrive
> > >     ( The cygdrive path prefix )
> > > 
> > > .... you can access arbitary drives on your system by using the cygdrive
> > > path
> > > prefix. The default value for this prefix is /cygdrive ...
> > > ....
> > > 
> > > The cygdrive prefix is a >>> virtual directory <<< under which all drives
> > > on
> > > a system are subsumed ...
> > > ....
> > > 
> > > The cygdrive prefix may be CHANGED in the fstab file as outlined above
> > > !!!!!
> > > ....
> > > 
> > > To simplify scripting, Cygwin also provides a /proc/cygdrive symlink, ...
> > > 
> > > =====
> > 
> > That's all very interesting, but I fail to see the relevance with regards
> > to the issue at hand, namely whether to special-case `/cygdrive` as a
> > special prefix that cannot be treated as directory in Git.
> > 
> > I still maintain that it should not be special-cased, no matter whether it
> > is a virtual directory or whether it can be renamed to `/jh-likes-cygwin`
> > or whatever.
> 
> Ok. Sorry about the noise.
> 
> From your post I got the impression that you believed that there will always
> be a directory called /cygdrive on Cygwin.

I know it can be different. In MSYS2 it is set to `/` via this line in
`/etc/fstab`:

	none / cygdrive binary,posix=0,noacl,user 0 0

Which is just to say that I am fully aware of the option to rename it.

> My point: it can have a different name.

Indeed.

And whatever name you give it, Cygwin can handle it as if it were a
regular directory. So we *must not* special-case it in Git.

Ciao,
Johannes

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

[Achim Gratz]

Junio C Hamano writes:
> 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.

The cygdrive prefix can be configured by the user to something
arbitrarily different, so if you're hoping to simplify the string
handling this way you'll most likely be disappointed.  It is exactly
that fact that led to the introduction of /proc/cygdrive as an
alternative prefix which doesn't depend on any configuration.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

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

[Johannes Schindelin]

Hi Torsten,

On Mon, 26 Nov 2018, tboegi@web.de wrote:

> 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;
> +}
> +
>  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])) {

It takes a little folding and knotting of the brain to understand that
this `!skip_dos_drive_prefix(&pos)` has *nothing* to do with the comment
`unc paths` nor with the test whether the paths starts with two directory
separators.

As a consequence, I would highly suggest to turn this into:

	if (skip_dos_drive_prefix(&pos))
		; /* absolute path with DOS drive prefix */
  	/* unc paths */
	else if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {

That makes the code a lot easier to understand, and as a consequence a lot
harder to mess up in the future.

Thanks,
Dscho

>  		/* skip server name */
> -		pos = strchr(pos + 2, '/');
> +		pos = strpbrk(pos + 2, "\\/");
>  		if (!pos)
>  			return 0; /* Error: malformed unc path */
>  
>  		do {
>  			pos++;
> -		} while (*pos && pos[0] != '/');
> +		} while (*pos && !is_dir_sep(*pos));
>  	}
> +
>  	return pos + is_dir_sep(*pos) - path;
>  }
> 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)
> +
> +
> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component
> +
> +
> +#define has_dos_drive_prefix(path) \
> +	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> +int cygwin_skip_dos_drive_prefix(char **path);
> +#define skip_dos_drive_prefix cygwin_skip_dos_drive_prefix
> +static inline int cygwin_is_dir_sep(int c)
> +{
> +	return c == '/' || c == '\\';
> +}
> +#define is_dir_sep cygwin_is_dir_sep
> +static inline char *cygwin_find_last_dir_sep(const char *path)
> +{
> +	char *ret = NULL;
> +	for (; *path; ++path)
> +		if (is_dir_sep(*path))
> +			ret = (char *)path;
> +	return ret;
> +}
> +static inline void convert_slashes(char *path)
> +{
> +	for (; *path; path++)
> +		if (*path == '\\')
> +			*path = '/';
> +}
> +#define find_last_dir_sep cygwin_find_last_dir_sep
>  int cygwin_offset_1st_component(const char *path);
>  #define offset_1st_component cygwin_offset_1st_component
> -- 
> 2.19.0.271.gfe8321ec05
> 
> 

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It takes a little folding and knotting of the brain to understand that
> this `!skip_dos_drive_prefix(&pos)` has *nothing* to do with the comment
> `unc paths` nor with the test whether the paths starts with two directory
> separators.
>
> As a consequence, I would highly suggest to turn this into:
>
> 	if (skip_dos_drive_prefix(&pos))
> 		; /* absolute path with DOS drive prefix */
>   	/* unc paths */
> 	else if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
>
> That makes the code a lot easier to understand, and as a consequence a lot
> harder to mess up in the future.

Excellent.  With or without "unc paths" comment, the separation does
make the logic more clear.

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

[Achim Gratz]

tboegi@web.de writes:
> The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
> is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
> in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]

Please use the Cygwin API path conversion functions for C code and the
cygpath program for shell code instead of trying to re-implement your
own handling (which is prone to introduce subtle bugs or at least
different heuristics from what cygwin itself uses).

https://cygwin.com/cygwin-api/cygwin-functions.html#func-cygwin-path


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

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

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

A regression for cygwin users was introduced with commit 05b458c,
 "real_path: resolve symlinks by hand".

In the the commit message we read:
  The current implementation of real_path uses chdir() in order to resolve
    symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
      process as a whole...

The old (and non-thread-save) OS calls chdir()/pwd() had been
replaced by a string operation.
The cygwin layer "knows" that "C:\cygwin" is an absolute path,
but the new string operation does not.

"git clone <url> C:\cygwin\home\USER\repo" fails like this:
fatal: Invalid path '/home/USER/repo/C:\cygwin\home\USER\repo'

The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]

Instead of duplicating the code, it is extracted into compat/mingw-cygwin.[ch]
Some need for refactoring and cleanup came up in the review, they are adressed
in a seperate commit.

Reported-By: Steven Penny <svnpenn@gmail.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 compat/cygwin.c       | 19 -------------------
 compat/cygwin.h       |  2 --
 compat/mingw-cygwin.c | 28 ++++++++++++++++++++++++++++
 compat/mingw-cygwin.h | 20 ++++++++++++++++++++
 compat/mingw.c        | 29 +----------------------------
 compat/mingw.h        | 20 --------------------
 config.mak.uname      |  4 ++--
 git-compat-util.h     |  3 ++-
 8 files changed, 53 insertions(+), 72 deletions(-)
 delete mode 100644 compat/cygwin.c
 delete mode 100644 compat/cygwin.h
 create mode 100644 compat/mingw-cygwin.c
 create mode 100644 compat/mingw-cygwin.h

diff --git a/compat/cygwin.c b/compat/cygwin.c
deleted file mode 100644
index b9862d606d..0000000000
--- a/compat/cygwin.c
+++ /dev/null
@@ -1,19 +0,0 @@
-#include "../git-compat-util.h"
-#include "../cache.h"
-
-int cygwin_offset_1st_component(const char *path)
-{
-	const char *pos = path;
-	/* unc paths */
-	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
-		/* skip server name */
-		pos = strchr(pos + 2, '/');
-		if (!pos)
-			return 0; /* Error: malformed unc path */
-
-		do {
-			pos++;
-		} while (*pos && pos[0] != '/');
-	}
-	return pos + is_dir_sep(*pos) - path;
-}
diff --git a/compat/cygwin.h b/compat/cygwin.h
deleted file mode 100644
index 8e52de4644..0000000000
--- a/compat/cygwin.h
+++ /dev/null
@@ -1,2 +0,0 @@
-int cygwin_offset_1st_component(const char *path);
-#define offset_1st_component cygwin_offset_1st_component
diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c
new file mode 100644
index 0000000000..c63d7acb9c
--- /dev/null
+++ b/compat/mingw-cygwin.c
@@ -0,0 +1,28 @@
+#include "../git-compat-util.h"
+
+int mingw_cygwin_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+
+int mingw_cygwin_offset_1st_component(const char *path)
+{
+	char *pos = (char *)path;
+
+	/* unc paths */
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+		/* skip server name */
+		pos = strpbrk(pos + 2, "\\/");
+		if (!pos)
+			return 0; /* Error: malformed unc path */
+
+		do {
+			pos++;
+		} while (*pos && !is_dir_sep(*pos));
+	}
+
+	return pos + is_dir_sep(*pos) - path;
+}
diff --git a/compat/mingw-cygwin.h b/compat/mingw-cygwin.h
new file mode 100644
index 0000000000..66ccc909ae
--- /dev/null
+++ b/compat/mingw-cygwin.h
@@ -0,0 +1,20 @@
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+int mingw_cygwin_skip_dos_drive_prefix(char **path);
+#define skip_dos_drive_prefix mingw_cygwin_skip_dos_drive_prefix
+static inline int mingw_cygwin_is_dir_sep(int c)
+{
+	return c == '/' || c == '\\';
+}
+#define is_dir_sep mingw_cygwin_is_dir_sep
+static inline char *mingw_cygwin_find_last_dir_sep(const char *path)
+{
+	char *ret = NULL;
+	for (; *path; ++path)
+		if (is_dir_sep(*path))
+			ret = (char *)path;
+	return ret;
+}
+#define find_last_dir_sep mingw_cygwin_find_last_dir_sep
+int mingw_cygwin_offset_1st_component(const char *path);
+#define offset_1st_component mingw_cygwin_offset_1st_component
diff --git a/compat/mingw.c b/compat/mingw.c
index 34b3880b29..038e96af9d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -350,7 +350,7 @@ static inline int needs_hiding(const char *path)
 		return 0;
 
 	/* We cannot use basename(), as it would remove trailing slashes */
-	mingw_skip_dos_drive_prefix((char **)&path);
+	mingw_cygwin_skip_dos_drive_prefix((char **)&path);
 	if (!*path)
 		return 0;
 
@@ -2275,33 +2275,6 @@ pid_t waitpid(pid_t pid, int *status, int options)
 	return -1;
 }
 
-int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
-
-int mingw_offset_1st_component(const char *path)
-{
-	char *pos = (char *)path;
-
-	/* unc paths */
-	if (!skip_dos_drive_prefix(&pos) &&
-			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
-		/* skip server name */
-		pos = strpbrk(pos + 2, "\\/");
-		if (!pos)
-			return 0; /* Error: malformed unc path */
-
-		do {
-			pos++;
-		} while (*pos && !is_dir_sep(*pos));
-	}
-
-	return pos + is_dir_sep(*pos) - path;
-}
-
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
 {
 	int upos = 0, wpos = 0;
diff --git a/compat/mingw.h b/compat/mingw.h
index 8c24ddaa3e..30d9fb3e36 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -443,32 +443,12 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) \
-	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-int mingw_skip_dos_drive_prefix(char **path);
-#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
-static inline int mingw_is_dir_sep(int c)
-{
-	return c == '/' || c == '\\';
-}
-#define is_dir_sep mingw_is_dir_sep
-static inline char *mingw_find_last_dir_sep(const char *path)
-{
-	char *ret = NULL;
-	for (; *path; ++path)
-		if (is_dir_sep(*path))
-			ret = (char *)path;
-	return ret;
-}
 static inline void convert_slashes(char *path)
 {
 	for (; *path; path++)
 		if (*path == '\\')
 			*path = '/';
 }
-#define find_last_dir_sep mingw_find_last_dir_sep
-int mingw_offset_1st_component(const char *path);
-#define offset_1st_component mingw_offset_1st_component
 #define PATH_SEP ';'
 extern char *mingw_query_user_email(void);
 #define query_user_email mingw_query_user_email
diff --git a/config.mak.uname b/config.mak.uname
index 3ee7da0e23..9346f67922 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -187,7 +187,7 @@ ifeq ($(uname_O),Cygwin)
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
-	COMPAT_OBJS += compat/cygwin.o
+	COMPAT_OBJS += compat/mingw-cygwin.o
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),FreeBSD)
@@ -526,7 +526,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	DEFAULT_HELP_FORMAT = html
 	COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
-	COMPAT_OBJS += compat/mingw.o compat/winansi.o \
+	COMPAT_OBJS += compat/mingw.o compat/mingw-cygwin.o compat/winansi.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
 		compat/win32/dirent.o
 	BASIC_CFLAGS += -DWIN32 -DPROTECT_NTFS_DEFAULT=1
diff --git a/git-compat-util.h b/git-compat-util.h
index 09b0102cae..7ece969b22 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -193,10 +193,11 @@
 #endif
 
 #if defined(__CYGWIN__)
-#include "compat/cygwin.h"
+#include "compat/mingw-cygwin.h"
 #endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
+#include "compat/mingw-cygwin.h"
 #include "compat/mingw.h"
 #elif defined(_MSC_VER)
 #include "compat/msvc.h"
-- 
2.19.0.271.gfe8321ec05

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

[Johannes Schindelin]

Hi Torsten,

On Fri, 7 Dec 2018, tboegi@web.de wrote:

>  compat/mingw-cygwin.c | 28 ++++++++++++++++++++++++++++
>  compat/mingw-cygwin.h | 20 ++++++++++++++++++++

Please use compat/win32/path.c (or .../path-utils.c) instead. This is not
so much about MINGW or Cygwin or MSys or MSYS2 or Visual C++, but about
Windows.

Thanks,
Johannes

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

[Steven Penny]

On Fri, Dec 7, 2018 at 11:04 AM wrote:
> The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
> is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
> in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]
>
> Instead of duplicating the code, it is extracted into compat/mingw-cygwin.[ch]
> Some need for refactoring and cleanup came up in the review, they are adressed
> in a seperate commit.

i have applied the 3 patches against current master, and my original test
passes, so looks good to me.

however like Johannes i take issue with the naming. as he said "mingw-cygwin"
really isnt appropriate. ideally it would be "windows.h", but as that is
conspicuously in use, something like these:

- pc-windows
- pc-win
- win

i disagree with him on using "win32" - that doesnt really make sense, as
obviously you can compile 64-bit Git for Windows. if you wanted to go that route
you would want to use something like:

- windows-api
- win-api
- winapi

further - i disagree with the "DOS" moniker being used at all. DOS is a defunkt
operating system that i dont think Git has *ever* supported, so it doesnt make
sense to be referring to it this way. again, a more approriate name would be
something like "win_drive_prefix".

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

[Johannes Schindelin]

Hi Steven,

please stop dropping me from the Cc: list. Thanks.

On Fri, 7 Dec 2018, Steven Penny wrote:

> On Fri, Dec 7, 2018 at 11:04 AM wrote:
>
> > The solution is to implement has_dos_drive_prefix(),
> > skip_dos_drive_prefix() is_dir_sep(), offset_1st_component() and
> > convert_slashes() for cygwin in the same way as it is done in 'Git for
> > Windows' in compat/mingw.[ch]
> >
> > Instead of duplicating the code, it is extracted into
> > compat/mingw-cygwin.[ch] Some need for refactoring and cleanup came up
> > in the review, they are adressed in a seperate commit.
> 
> i have applied the 3 patches against current master, and my original
> test passes, so looks good to me.
> 
> however like Johannes i take issue with the naming. as he said "mingw-cygwin"
> really isnt appropriate. ideally it would be "windows.h", but as that is
> conspicuously in use, something like these:
> 
> - pc-windows
> - pc-win
> - win

I find all of those horrible.

> i disagree with him on using "win32" - that doesnt really make sense,

... except if you take into account that this has been our convention for,
what, almost 9 years (since 44626dc7d5 (MSVC: Windows-native
implementation for subset of Pthreads API, 2010-01-15), to be precise)? In
that case, it makes a ton of sense, and one might be tempted to ask who
the person wanting to change that thinks they are...

> as obviously you can compile 64-bit Git for Windows. if you wanted to go
> that route you would want to use something like:
> 
> - windows-api
> - win-api
> - winapi
> 
> further - i disagree with the "DOS" moniker being used at all. DOS is a
> defunkt operating system that i dont think Git has *ever* supported, so
> it doesnt make sense to be referring to it this way. again, a more
> approriate name would be something like "win_drive_prefix".

You may disagree all you want, but given that Torsten has been a lot more
active on this code than you have been so far, I'll go with Torsten's
taste. Which incidentally happens to match my tastes, so that's an added
bonus.

Ciao,
Johannes

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

[Steven Penny]

On Mon, Dec 10, 2018 at 2:46 AM Johannes Schindelin wrote:
> please stop dropping me from the Cc: list. Thanks.

i dropped you specifically because i knew you were going to flame like you just
did below. oh well, i guess you cant avoid the inevitable.

> > - pc-windows
> > - pc-win
> > - win
>
> I find all of those horrible.

they arent great, but "win32" simply isnt valid. as soon as we started compiling
for 64-bit and using 64-bit APIs it didnt make sense to keep using it. if you
want to refer to all versions of the Microsoft OS you say "Windows", not
"Windows XP", as that would be confusing for people using Windows 10. In the
same vein you shouldnt refer to the current Windows API as "Win32" because its
no longer just 32-bit.

> ... except if you take into account that this has been our convention for,
> what, almost 9 years (since 44626dc7d5 (MSVC: Windows-native
> implementation for subset of Pthreads API, 2010-01-15), to be precise)? In
> that case, it makes a ton of sense, and one might be tempted to ask who
> the person wanting to change that thinks they are...

"That's the way it's always been done" is not a good reason to keep doing
something. I would say the justification goes the other way, as to why we should
keep using an old moniker when its past making sense.

> You may disagree all you want, but given that Torsten has been a lot more
> active on this code than you have been so far, I'll go with Torsten's
> taste. Which incidentally happens to match my tastes, so that's an added
> bonus.

in the end i dont really care what your taste is, or Torsten for that matter. I
care that the issue be fixed.

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

[Johannes Schindelin]

Hi Steven,

On Mon, 10 Dec 2018, Steven Penny wrote:

> On Mon, Dec 10, 2018 at 2:46 AM Johannes Schindelin wrote:
> > please stop dropping me from the Cc: list. Thanks.
> 
> i dropped you specifically because i knew you were going to flame like
> you just did below. oh well, i guess you cant avoid the inevitable.

I have no intention of flaming anybody. That is simply a
misrepresentation.

> > > - pc-windows
> > > - pc-win
> > > - win
> >
> > I find all of those horrible.
> 
> they arent great, but "win32" simply isnt valid.

It is a long established convention to talk about the Win32 API as the set
of functions developed for Windows NT and backported to Windows 95.

There is no benefit in abandoning that convention just to please you.

> > ... except if you take into account that this has been our convention
> > for, what, almost 9 years (since 44626dc7d5 (MSVC: Windows-native
> > implementation for subset of Pthreads API, 2010-01-15), to be
> > precise)? In that case, it makes a ton of sense, and one might be
> > tempted to ask who the person wanting to change that thinks they
> > are...
> 
> "That's the way it's always been done" is not a good reason to keep
> doing something. I would say the justification goes the other way, as to
> why we should keep using an old moniker when its past making sense.

If you want to change something that has been in use for a long time, you
have to have good reasons. None of your arguments convinces me so far that
you have any good reason to change these.

Let's hear some good argument in a well-prepared patch, or alternatively
let's just not discuss these hypotheticals anymore.

> > You may disagree all you want, but given that Torsten has been a lot
> > more active on this code than you have been so far, I'll go with
> > Torsten's taste. Which incidentally happens to match my tastes, so
> > that's an added bonus.
> 
> in the end i dont really care what your taste is, or Torsten for that
> matter. I care that the issue be fixed.

If anyone truly cares about an issue to be fixed, I would expect more
assisting, and less distracting, to do wonders.

Ciao,
Johannes

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

[Steven Penny]

On Tue, Dec 11, 2018 at 7:39 AM Johannes Schindelin wrote:
> I have no intention of flaming anybody. That is simply a
> misrepresentation.

you may see yourself "through a glass darkly", but i dont. this language is not
constructive:

> > - pc-windows
> > - pc-win
> > - win
>
> I find all of those horrible.

one windows triplet in use is "x86_64-pc-windows", used by Rust:

https://forge.rust-lang.org/other-installation-methods.html

which is how i came about my suggestions - again they arent great but they arent
misleading as "Win32" is.

> It is a long established convention to talk about the Win32 API as the set
> of functions developed for Windows NT and backported to Windows 95.
>
> There is no benefit in abandoning that convention just to please you.

Quoting from Wikipedia (emphasis mine):

> The **Windows API**, informally **WinAPI**, is Microsoft's core set of
> application programming interfaces (APIs) available in the Microsoft Windows
> operating systems. The name **Windows API** collectively refers to several
> different platform implementations that are often referred to by their own
> names (for example, **Win32** API)

and:

> Microsoft eventually changed the name of the then current **Win32** API family
> into **Windows API**, and made it into a catch-all term for both past and
> future API versions.

http://wikipedia.org/wiki/Windows_API

and quoting directly from Microsoft:

> The **Windows API** can be used in all Windows-based desktop applications, and
> the same functions are generally supported on 32-bit and 64-bit Windows.

http://docs.microsoft.com/windows/desktop/apiindex/api-index-portal

So again, "Win32" refers specifically to the old 32-bit only version of the API,
while:

- windows-api
- win-api
- winapi

refer to the current version.

> If you want to change something that has been in use for a long time, you
> have to have good reasons. None of your arguments convinces me so far that
> you have any good reason to change these.

i am certainly not interested in convincing you. i figured you wouldve gleaned
this from the fact that i removed you from the CC. Nevertheless, see above
links.

> If anyone truly cares about an issue to be fixed, I would expect more
> assisting, and less distracting, to do wonders.

hmm:

- http://public-inbox.org/git/CAAXzdLXSJU5bC_D1Q_gCWqKG7mcdcAvRkiYzano-VsrRRxazDQ@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLXmJ1YKiTF17b=ZfkM3HtJCNkvVMQNU=riW8R42VLid_Q@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLWByGC+B_XdDiJwounoPgMAsMq=EuOSx9bdV-f5vQUhnA@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLXCEeZdkCXT+-0n=Fn7_=Nz5cm+6xr0w-cd6B1om028uA@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLU3dsCabgYKnD9c7iWZcXx1cfO3tisJ7r0dNjiiTHk1mA@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLWBSD5coxqbyRN_d9B1e4AA-Q6VQ7iRo8BPuhBKDicMRQ@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLX4jU7+i1W1A_Q1LpPFa1D4FYVPW5rcMnqr_tDHEJn+tw@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLWtDw09umyr23qZkv2jQ6_mTeFXbktgb-f6S2w6Zf1Egg@mail.gmail.com

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

[Johannes Sixt]

Am 12.12.18 um 01:42 schrieb Steven Penny:
> On Tue, Dec 11, 2018 at 7:39 AM Johannes Schindelin wrote:
>>> - pc-windows
>>> - pc-win
>>> - win
>>
>> I find all of those horrible.
> 
> one windows triplet in use is "x86_64-pc-windows", used by Rust:
> 
> https://forge.rust-lang.org/other-installation-methods.html
> 
> which is how i came about my suggestions - again they arent great but they arent
> misleading as "Win32" is.

As long as C:\Windows\System32 on my Windows computer contains only 
64-Bit binaries, I consider the characters "3" and "2" next to each 
other in this order just noise and without any form of information. The 
important part of the name is "win".

-- Hannes

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

[Steven Penny]

On Wed, Dec 12, 2018 at 1:29 AM Johannes Sixt wrote:
> As long as C:\Windows\System32 on my Windows computer contains only
> 64-Bit binaries, I consider the characters "3" and "2" next to each
> other in this order just noise and without any form of information. The
> important part of the name is "win".

sorry friend, but thats a logical fallacy :(

http://yourlogicalfallacyis.com/no-true-scotsman

just because the name "System32" is still in use (wrongly, I might add) doesnt
mean that "Win32" should still be in use.

Each name is a separate argument. The "Win32" name has been changed by Microsoft
and shouldnt be used by Git if possible. Its an easy change and I could send
a pull request myself. However just because Microsoft hasnt changed "Sytem32"
doesnt mean that they wont or shouldnt - as you said its just as misleading. We
should fix ambiguities where we can, not embrace them.

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

[Junio C Hamano]

Steven Penny <svnpenn@gmail.com> writes:

> On Wed, Dec 12, 2018 at 1:29 AM Johannes Sixt wrote:
>> As long as C:\Windows\System32 on my Windows computer contains only
>> 64-Bit binaries, I consider the characters "3" and "2" next to each
>> other in this order just noise and without any form of information. The
>> important part of the name is "win".
>
> sorry friend, but thats a logical fallacy :(
>
> http://yourlogicalfallacyis.com/no-true-scotsman
>
> just because the name "System32" is still in use (wrongly, I might add) doesnt
> mean that "Win32" should still be in use.
>
> Each name is a separate argument. The "Win32" name has been changed by Microsoft
> and shouldnt be used by Git if possible. Its an easy change and I could send
> a pull request myself. However just because Microsoft hasnt changed "Sytem32"
> doesnt mean that they wont or shouldnt - as you said its just as misleading. We
> should fix ambiguities where we can, not embrace them.

FWIW, in the context of the Git development ecosystem, whatever
Dscho wants to do in compat/ that is limited to Windows is
"officially endorsed by Microsoft" enough.

Also I do not think J6t felt 32 in System32 or in win32 was
misleading.  At least I didn't read the above that way.  

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

[Johannes Schindelin]

Hi Hannes,

On Wed, 12 Dec 2018, Johannes Sixt wrote:

> Am 12.12.18 um 01:42 schrieb Steven Penny:
> > On Tue, Dec 11, 2018 at 7:39 AM Johannes Schindelin wrote:
> > > > - pc-windows
> > > > - pc-win
> > > > - win
> > >
> > > I find all of those horrible.
> > 
> > one windows triplet in use is "x86_64-pc-windows", used by Rust:
> > 
> > https://forge.rust-lang.org/other-installation-methods.html
> > 
> > which is how i came about my suggestions - again they arent great but they
> > arent
> > misleading as "Win32" is.
> 
> As long as C:\Windows\System32 on my Windows computer contains only 64-Bit
> binaries, I consider the characters "3" and "2" next to each other in this
> order just noise and without any form of information. The important part of
> the name is "win".

FWIW I agree with you.

Thanks,
Dscho

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

[Elijah Newren]

On Fri, Dec 7, 2018 at 4:51 PM Steven Penny <svnpenn@gmail.com> wrote:
>
> On Fri, Dec 7, 2018 at 11:04 AM wrote:
> > The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
> > is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
> > in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]
> >
> > Instead of duplicating the code, it is extracted into compat/mingw-cygwin.[ch]
> > Some need for refactoring and cleanup came up in the review, they are adressed
> > in a seperate commit.
>
> i have applied the 3 patches against current master, and my original test
> passes, so looks good to me.
>
> however like Johannes i take issue with the naming. as he said "mingw-cygwin"
> really isnt appropriate. ideally it would be "windows.h", but as that is
> conspicuously in use, something like these:
>
> - pc-windows
> - pc-win
> - win
>
> i disagree with him on using "win32" - that doesnt really make sense, as
> obviously you can compile 64-bit Git for Windows. if you wanted to go that route
> you would want to use something like:
>
> - windows-api
> - win-api
> - winapi
>
> further - i disagree with the "DOS" moniker being used at all. DOS is a defunkt
> operating system that i dont think Git has *ever* supported, so it doesnt make
> sense to be referring to it this way. again, a more approriate name would be
> something like "win_drive_prefix".

You seem to want internal function and filenames to be based on the
product or marketing names of currently targetted systems.  I don't
see why this is desirable; could you explain why it is?

I admit that I seem to see things more from Dscho's angle.  However, I
know much less about Windows than either of you.  Perhaps my best
understanding of the situation might help, limited as it is:
  - Using currently targetted system names means future code churn --
we may have to rename functions and files for absolutely no useful
gain, muddying the history, making it harder for developers to
remember how to find things, etc., simply because an external party
renamed their libraries or introduced a new product.
  - For people less familiar with windows,
"windows-api/win-api/winapi" and "win_drive_prefix" may sound like
something only available in newer systems, making them wonder if the
file name or function name is referring to some new windows concept
they are unfamiliar with
  - Mentioning an anchor point where the relevant concept originally
targetted or where it came from (win32/path.c, dos_drive_prefix)
avoids or greatly reduces both problems.


I'm worried based on other emails in this thread that there is a
fundamental difference in frame of reference leading to a
misunderstanding about rationale for naming, and worse that folks
might not even realize where the misunderstanding is coming from,
attributing it to different motives rather than different frames of
reference.  If that's true, I hope this email can begin the process of
clearing up the differences of understanding.  If I'm wrong, then I
apologize for the noise; just ignore me.

Best wishes,
Elijah

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

[Johannes Schindelin]

Hi Elijah,

On Tue, 11 Dec 2018, Elijah Newren wrote:

> I'm worried based on other emails in this thread that there is a
> fundamental difference in frame of reference leading to a
> misunderstanding about rationale for naming, and worse that folks might
> not even realize where the misunderstanding is coming from, attributing
> it to different motives rather than different frames of reference.  If
> that's true, I hope this email can begin the process of clearing up the
> differences of understanding.  If I'm wrong, then I apologize for the
> noise; just ignore me.

I think you brought up quite a few good points (also in the part that I
did not quote).

The part I quoted brings up one particular aspect that I would like to
drive home a little more: the purpose of naming, and the historical
reality. ("hysterical raisins" comes to mind.)

In Git, we have an awful lot of references to MINGW, which is the name of
a project that tried to allow compiling software targeting pure Windows
(i.e. the Win32 API, without any POSIX compatibility layer) with the GNU C
compiler.

As many open source projects require more than just the GNU C compiler
(e.g. a Bash to run ./configure), there is also MSys, which is a minimal
fork of a then-current version of Cygwin, originally intended for the sole
purpose to support building MINGW software.

To make things more confusing, at some stage the mingw-w64 project was
started (not as a fork of MINGW, AFAIU), to address the notable lack of
64-bit support in MINGW, and later the MSYS2 project was started, based on
mingw-w64, to address the same issue with MSys (also not forking, but
instead starting from scratch).

Back in 2006, when I started to port Git to Windows, I made use of MINGW
and MSys (and I abused MSys by shipping their Bash with Git, which was
distinctly not intended a usage of their Bash). Hannes Sixt picked up when
I stopped having access to a fast Windows machine, and kept my naming:
compat/mingw.c.

Now, Philip Oakley, Jeff Hostetler and a few other developers spent quite
a bit of effort to make Git compile also with Visual C, and of course the
reused parts of compat/mingw.c (whose name now does not make too much
sense anymore, except in historical context).

Likewise, when I switched to MSYS2/mingw-w64 with the major version jump
to Git for Windows 2.x in 2015, I no longer use MINGW to compile
*anything*.

I hope that this illustrated a little bit how our names came about.

Of course, we could now spend some time to change the names to reflect
more the product and brand names involved. There does not seem to be any
really compelling reason to do so, though. And I'd rather spend my time
developing exciting features. But that's my preference for my time, so if
anybody comes along, making a strong case for renaming, in a well-crafted
patch series, who am I to say no to that.

Ciao,
Dscho

[PATCH v2 2/3] offset_1st_component(), dos_drive_prefix() return size_t

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

Change the return value for offset_1st_component(),
has_dos_drive_prefix() and skip_dos_drive_prefix from int into size_t,
which is the natural type for length of data in memory.

While at it, remove possible "parameter not used" warnings in for the
non-Windows builds in git-compat-util.h

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 abspath.c             | 2 +-
 compat/mingw-cygwin.c | 6 +++---
 compat/mingw-cygwin.h | 4 ++--
 git-compat-util.h     | 8 +++++---
 setup.c               | 4 ++--
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/abspath.c b/abspath.c
index 9857985329..12055a1d8f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -51,7 +51,7 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
 /* copies root part from remaining to resolved, canonicalizing it on the way */
 static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
 {
-	int offset = offset_1st_component(remaining->buf);
+	size_t offset = offset_1st_component(remaining->buf);
 
 	strbuf_reset(resolved);
 	strbuf_add(resolved, remaining->buf, offset);
diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c
index c63d7acb9c..5552c3ac20 100644
--- a/compat/mingw-cygwin.c
+++ b/compat/mingw-cygwin.c
@@ -1,13 +1,13 @@
 #include "../git-compat-util.h"
 
-int mingw_cygwin_skip_dos_drive_prefix(char **path)
+size_t mingw_cygwin_skip_dos_drive_prefix(char **path)
 {
-	int ret = has_dos_drive_prefix(*path);
+	size_t ret = has_dos_drive_prefix(*path);
 	*path += ret;
 	return ret;
 }
 
-int mingw_cygwin_offset_1st_component(const char *path)
+size_t mingw_cygwin_offset_1st_component(const char *path)
 {
 	char *pos = (char *)path;
 
diff --git a/compat/mingw-cygwin.h b/compat/mingw-cygwin.h
index 66ccc909ae..0e8a0c9074 100644
--- a/compat/mingw-cygwin.h
+++ b/compat/mingw-cygwin.h
@@ -1,6 +1,6 @@
 #define has_dos_drive_prefix(path) \
 	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-int mingw_cygwin_skip_dos_drive_prefix(char **path);
+size_t mingw_cygwin_skip_dos_drive_prefix(char **path);
 #define skip_dos_drive_prefix mingw_cygwin_skip_dos_drive_prefix
 static inline int mingw_cygwin_is_dir_sep(int c)
 {
@@ -16,5 +16,5 @@ static inline char *mingw_cygwin_find_last_dir_sep(const char *path)
 	return ret;
 }
 #define find_last_dir_sep mingw_cygwin_find_last_dir_sep
-int mingw_cygwin_offset_1st_component(const char *path);
+size_t mingw_cygwin_offset_1st_component(const char *path);
 #define offset_1st_component mingw_cygwin_offset_1st_component
diff --git a/git-compat-util.h b/git-compat-util.h
index 7ece969b22..65eaaf0d50 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -355,16 +355,18 @@ static inline int noop_core_config(const char *var, const char *value, void *cb)
 #endif
 
 #ifndef has_dos_drive_prefix
-static inline int git_has_dos_drive_prefix(const char *path)
+static inline size_t git_has_dos_drive_prefix(const char *path)
 {
+	(void)path;
 	return 0;
 }
 #define has_dos_drive_prefix git_has_dos_drive_prefix
 #endif
 
 #ifndef skip_dos_drive_prefix
-static inline int git_skip_dos_drive_prefix(char **path)
+static inline size_t git_skip_dos_drive_prefix(char **path)
 {
+	(void)path;
 	return 0;
 }
 #define skip_dos_drive_prefix git_skip_dos_drive_prefix
@@ -379,7 +381,7 @@ static inline int git_is_dir_sep(int c)
 #endif
 
 #ifndef offset_1st_component
-static inline int git_offset_1st_component(const char *path)
+static inline size_t git_offset_1st_component(const char *path)
 {
 	return is_dir_sep(path[0]);
 }
diff --git a/setup.c b/setup.c
index 1be5037f12..538bc1ff99 100644
--- a/setup.c
+++ b/setup.c
@@ -29,7 +29,7 @@ static int abspath_part_inside_repo(char *path)
 	size_t len;
 	size_t wtlen;
 	char *path0;
-	int off;
+	size_t off;
 	const char *work_tree = get_git_work_tree();
 
 	if (!work_tree)
@@ -800,7 +800,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
 				      struct repository_format *repo_fmt,
 				      int *nongit_ok)
 {
-	int root_len;
+	size_t root_len;
 
 	if (check_repository_format_gently(".", repo_fmt, nongit_ok))
 		return NULL;
-- 
2.19.0.271.gfe8321ec05

[PATCH v2 3/3] Refactor mingw_cygwin_offset_1st_component()

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

The Windows version of offset_1st_component() needs to hande 3 cases:
- The path is an UNC path, starting with "//" or "\\\\".
  Skip the servername and the name of the share.
- The path is a DOS drive, starting with e.g. "X:"
  The driver letter and the ':' must be skipped
- The path is pointing to a subdirectory somewhere in the path and the
  directory seperator needs to be skipped ('/' or '\\').

Refactor the code to make it easier to read.

Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 compat/mingw-cygwin.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c
index 5552c3ac20..c379a72775 100644
--- a/compat/mingw-cygwin.c
+++ b/compat/mingw-cygwin.c
@@ -10,10 +10,8 @@ size_t mingw_cygwin_skip_dos_drive_prefix(char **path)
 size_t mingw_cygwin_offset_1st_component(const char *path)
 {
 	char *pos = (char *)path;
-
-	/* unc paths */
-	if (!skip_dos_drive_prefix(&pos) &&
-			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+		/* unc path */
 		/* skip server name */
 		pos = strpbrk(pos + 2, "\\/");
 		if (!pos)
@@ -22,7 +20,8 @@ size_t mingw_cygwin_offset_1st_component(const char *path)
 		do {
 			pos++;
 		} while (*pos && !is_dir_sep(*pos));
+	} else {
+		skip_dos_drive_prefix(&pos);
 	}
-
 	return pos + is_dir_sep(*pos) - path;
 }
-- 
2.19.0.271.gfe8321ec05

Re: [PATCH v2 3/3] Refactor mingw_cygwin_offset_1st_component()

[Johannes Schindelin]

Hi Torsten,

On Fri, 7 Dec 2018, tboegi@web.de wrote:

> diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c
> index 5552c3ac20..c379a72775 100644
> --- a/compat/mingw-cygwin.c
> +++ b/compat/mingw-cygwin.c
> @@ -10,10 +10,8 @@ size_t mingw_cygwin_skip_dos_drive_prefix(char **path)
>  size_t mingw_cygwin_offset_1st_component(const char *path)
>  {
>  	char *pos = (char *)path;
> -
> -	/* unc paths */

This comment is still useful (and now even more correct), and should stay.

> -	if (!skip_dos_drive_prefix(&pos) &&
> -			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> +	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> +		/* unc path */
>  		/* skip server name */
>  		pos = strpbrk(pos + 2, "\\/");
>  		if (!pos)
> @@ -22,7 +20,8 @@ size_t mingw_cygwin_offset_1st_component(const char *path)
>  		do {
>  			pos++;
>  		} while (*pos && !is_dir_sep(*pos));
> +	} else {
> +		skip_dos_drive_prefix(&pos);
>  	}
> -

Why remove this empty line? It structures the code quite nicely.

The rest looks correct to me,
Johannes

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

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

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

A regression for cygwin users was introduced with commit 05b458c,
 "real_path: resolve symlinks by hand".

In the the commit message we read:
  The current implementation of real_path uses chdir() in order to resolve
    symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
      process as a whole...

The old (and non-thread-save) OS calls chdir()/pwd() had been
replaced by a string operation.
The cygwin layer "knows" that "C:\cygwin" is an absolute path,
but the new string operation does not.

"git clone <url> C:\cygwin\home\USER\repo" fails like this:
fatal: Invalid path '/home/USER/repo/C:\cygwin\home\USER\repo'

The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]

Extract the needed code into compat/win32/path-utils.[ch] and use it
for cygwin as well.

Reported-by: Steven Penny <svnpenn@gmail.com>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
Changes since V2:
- Settled on a better name:
  The common code is in compat/win32/path-utils.c/h
- Skip the 2 patches which "only" do a cleanup (for a moment)
  put those cleanups onto the "todo stack".
- The "DOS" moniker is still used for 2 reasons:
  Windows inherited the "drive letter" concept from DOS,
  and everybody (tm) familar with the code and the path handling
  in Git is used to that wording.
  Even if there was a better name, it needed to be addressed
  in a patch series different from this one.
  Here I want to fix a reported regression.
   
And, before any cleanup is done, I sould like to ask if anybody
can build the code with VS and confirm that it works, please ?

Thanks for the reviews, testing and comment.

compat/cygwin.c           | 19 -------------------
 compat/cygwin.h           |  2 --
 compat/mingw.c            | 29 +----------------------------
 compat/mingw.h            | 20 --------------------
 compat/win32/path-utils.c | 28 ++++++++++++++++++++++++++++
 compat/win32/path-utils.h | 20 ++++++++++++++++++++
 config.mak.uname          |  3 ++-
 git-compat-util.h         |  3 ++-
 8 files changed, 53 insertions(+), 71 deletions(-)
 delete mode 100644 compat/cygwin.c
 delete mode 100644 compat/cygwin.h
 create mode 100644 compat/win32/path-utils.c
 create mode 100644 compat/win32/path-utils.h

diff --git a/compat/cygwin.c b/compat/cygwin.c
deleted file mode 100644
index b9862d606d..0000000000
--- a/compat/cygwin.c
+++ /dev/null
@@ -1,19 +0,0 @@
-#include "../git-compat-util.h"
-#include "../cache.h"
-
-int cygwin_offset_1st_component(const char *path)
-{
-	const char *pos = path;
-	/* unc paths */
-	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
-		/* skip server name */
-		pos = strchr(pos + 2, '/');
-		if (!pos)
-			return 0; /* Error: malformed unc path */
-
-		do {
-			pos++;
-		} while (*pos && pos[0] != '/');
-	}
-	return pos + is_dir_sep(*pos) - path;
-}
diff --git a/compat/cygwin.h b/compat/cygwin.h
deleted file mode 100644
index 8e52de4644..0000000000
--- a/compat/cygwin.h
+++ /dev/null
@@ -1,2 +0,0 @@
-int cygwin_offset_1st_component(const char *path);
-#define offset_1st_component cygwin_offset_1st_component
diff --git a/compat/mingw.c b/compat/mingw.c
index 34b3880b29..27e397f268 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -350,7 +350,7 @@ static inline int needs_hiding(const char *path)
 		return 0;
 
 	/* We cannot use basename(), as it would remove trailing slashes */
-	mingw_skip_dos_drive_prefix((char **)&path);
+	win_path_utils_skip_dos_drive_prefix((char **)&path);
 	if (!*path)
 		return 0;
 
@@ -2275,33 +2275,6 @@ pid_t waitpid(pid_t pid, int *status, int options)
 	return -1;
 }
 
-int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
-
-int mingw_offset_1st_component(const char *path)
-{
-	char *pos = (char *)path;
-
-	/* unc paths */
-	if (!skip_dos_drive_prefix(&pos) &&
-			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
-		/* skip server name */
-		pos = strpbrk(pos + 2, "\\/");
-		if (!pos)
-			return 0; /* Error: malformed unc path */
-
-		do {
-			pos++;
-		} while (*pos && !is_dir_sep(*pos));
-	}
-
-	return pos + is_dir_sep(*pos) - path;
-}
-
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
 {
 	int upos = 0, wpos = 0;
diff --git a/compat/mingw.h b/compat/mingw.h
index 8c24ddaa3e..30d9fb3e36 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -443,32 +443,12 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) \
-	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-int mingw_skip_dos_drive_prefix(char **path);
-#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
-static inline int mingw_is_dir_sep(int c)
-{
-	return c == '/' || c == '\\';
-}
-#define is_dir_sep mingw_is_dir_sep
-static inline char *mingw_find_last_dir_sep(const char *path)
-{
-	char *ret = NULL;
-	for (; *path; ++path)
-		if (is_dir_sep(*path))
-			ret = (char *)path;
-	return ret;
-}
 static inline void convert_slashes(char *path)
 {
 	for (; *path; path++)
 		if (*path == '\\')
 			*path = '/';
 }
-#define find_last_dir_sep mingw_find_last_dir_sep
-int mingw_offset_1st_component(const char *path);
-#define offset_1st_component mingw_offset_1st_component
 #define PATH_SEP ';'
 extern char *mingw_query_user_email(void);
 #define query_user_email mingw_query_user_email
diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c
new file mode 100644
index 0000000000..6cb9a6a745
--- /dev/null
+++ b/compat/win32/path-utils.c
@@ -0,0 +1,28 @@
+#include "../../git-compat-util.h"
+
+int win_path_utils_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+
+int win_path_utils_offset_1st_component(const char *path)
+{
+	char *pos = (char *)path;
+
+	/* unc paths */
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+		/* skip server name */
+		pos = strpbrk(pos + 2, "\\/");
+		if (!pos)
+			return 0; /* Error: malformed unc path */
+
+		do {
+			pos++;
+		} while (*pos && !is_dir_sep(*pos));
+	}
+
+	return pos + is_dir_sep(*pos) - path;
+}
diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
new file mode 100644
index 0000000000..c931b2a890
--- /dev/null
+++ b/compat/win32/path-utils.h
@@ -0,0 +1,20 @@
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+int win_path_utils_skip_dos_drive_prefix(char **path);
+#define skip_dos_drive_prefix win_path_utils_skip_dos_drive_prefix
+static inline int win_path_utils_is_dir_sep(int c)
+{
+	return c == '/' || c == '\\';
+}
+#define is_dir_sep win_path_utils_is_dir_sep
+static inline char *win_path_utils_find_last_dir_sep(const char *path)
+{
+	char *ret = NULL;
+	for (; *path; ++path)
+		if (is_dir_sep(*path))
+			ret = (char *)path;
+	return ret;
+}
+#define find_last_dir_sep win_path_utils_find_last_dir_sep
+int win_path_utils_offset_1st_component(const char *path);
+#define offset_1st_component win_path_utils_offset_1st_component
diff --git a/config.mak.uname b/config.mak.uname
index 3ee7da0e23..60876e26f4 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -187,7 +187,7 @@ ifeq ($(uname_O),Cygwin)
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
-	COMPAT_OBJS += compat/cygwin.o
+	COMPAT_OBJS += compat/win32/path-utils.o
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),FreeBSD)
@@ -527,6 +527,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/winansi.o \
+		compat/win32/path-utils.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
 		compat/win32/dirent.o
 	BASIC_CFLAGS += -DWIN32 -DPROTECT_NTFS_DEFAULT=1
diff --git a/git-compat-util.h b/git-compat-util.h
index 09b0102cae..5702556c89 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -193,10 +193,11 @@
 #endif
 
 #if defined(__CYGWIN__)
-#include "compat/cygwin.h"
+#include "compat/win32/path-utils.h"
 #endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
+#include "compat/win32/path-utils.h"
 #include "compat/mingw.h"
 #elif defined(_MSC_VER)
 #include "compat/msvc.h"
-- 
2.19.0.271.gfe8321ec05

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

[Steven Penny]

On Sat, Dec 8, 2018 at 9:11 AM wrote:
> Changes since V2:

latest patch still fixes original issue - thanks

> - Settled on a better name:
>   The common code is in compat/win32/path-utils.c/h
>   [...]
> - The "DOS" moniker is still used for 2 reasons:
>   Windows inherited the "drive letter" concept from DOS,
>   and everybody (tm) familar with the code and the path handling
>   in Git is used to that wording.
>   Even if there was a better name, it needed to be addressed
>   in a patch series different from this one.
>   Here I want to fix a reported regression.

i still disagree with this - but i understand if naming argument is out of scope
for thread

> And, before any cleanup is done, I sould like to ask if anybody
> can build the code with VS and confirm that it works, please ?

sorry but i am decidedly *not* interested in doing this. i use cygwin
specifically so that i can avoid VS. hopefully someone else will be able to
test. cheers

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

[Junio C Hamano]

tboegi@web.de writes:

> - The "DOS" moniker is still used for 2 reasons:
>   Windows inherited the "drive letter" concept from DOS,
>   and everybody (tm) familar with the code and the path handling
>   in Git is used to that wording.

Yeah, for the same reason as win32 can refer to their API that is
used on platforms that are 64-bit, the fact that the "drive letter"
concept came from DOS is so widely ingrained, I do not think it is a
beter change to deviate from it.

> And, before any cleanup is done, I sould like to ask if anybody
> can build the code with VS and confirm that it works, please ?

Yup, that is much more important.

Thanks.

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

[Johannes Schindelin]

Hi Torsten,

On Sat, 8 Dec 2018, tboegi@web.de wrote:

> And, before any cleanup is done, I sould like to ask if anybody
> can build the code with VS and confirm that it works, please ?

Can you give me an easy-to-fetch branch?

Thanks,
Dscho

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

[Torsten Bögershausen]

On Mon, Dec 10, 2018 at 09:32:03AM +0100, Johannes Schindelin wrote:
> Hi Torsten,
> 
> On Sat, 8 Dec 2018, tboegi@web.de wrote:
> 
> > And, before any cleanup is done, I sould like to ask if anybody
> > can build the code with VS and confirm that it works, please ?
> 
> Can you give me an easy-to-fetch branch?
> 
> Thanks,
> Dscho

@Dscho: The branch should be here:
  https://github.com/tboegi/git/tree/tb.181208_0844_cygwin-dos-drive
  (or fetch it from Junio, please see below:)

@Junio:
  Please keep tb/use-common-win32-pathfuncs-on-cygwin
  in pu for a while. I need to send a V4 to fix t5601 for cygwin.
 

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

Hi Torsten,

On Tue, 11 Dec 2018, Torsten Bögershausen wrote:

> On Mon, Dec 10, 2018 at 09:32:03AM +0100, Johannes Schindelin wrote:
> > 
> > On Sat, 8 Dec 2018, tboegi@web.de wrote:
> > 
> > > And, before any cleanup is done, I sould like to ask if anybody
> > > can build the code with VS and confirm that it works, please ?
> > 
> > Can you give me an easy-to-fetch branch?
> > 
> > Thanks,
> > Dscho
> 
> @Dscho: The branch should be here:
>   https://github.com/tboegi/git/tree/tb.181208_0844_cygwin-dos-drive
>   (or fetch it from Junio, please see below:)

I fetched it from you, as Junio frequently applies patches anywhere except
where they were developed. I'd rather see what you see. For the record,
this is the commit I tested: cc1e08eeb83b.

It builds fine here, and some cursory tests reveal that it works as
advertised (I ran t0001, t0060 and t5580).

However.

Can you please replace the rather unnecessary, very, very long
`win_path_utils_` function name prefix by the much better prefix `win32_`,
to keep in line with the current, already existing, surrounding files'
convention? Thanks a bunch.

Ciao,
Dscho

> @Junio:
>   Please keep tb/use-common-win32-pathfuncs-on-cygwin
>   in pu for a while. I need to send a V4 to fix t5601 for cygwin.
>  
> 

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

[Torsten Bögershausen]

> 
> Can you please replace the rather unnecessary, very, very long
> `win_path_utils_` function name prefix by the much better prefix `win32_`,
> to keep in line with the current, already existing, surrounding files'
> convention? Thanks a bunch.
> 

That makes sense - thanks for the suggestion & testing.

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

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

A regression for cygwin users was introduced with commit 05b458c,
 "real_path: resolve symlinks by hand".

In the the commit message we read:
  The current implementation of real_path uses chdir() in order to resolve
    symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
      process as a whole...

The old (and non-thread-save) OS calls chdir()/pwd() had been
replaced by a string operation.
The cygwin layer "knows" that "C:\cygwin" is an absolute path,
but the new string operation does not.

"git clone <url> C:\cygwin\home\USER\repo" fails like this:
fatal: Invalid path '/home/USER/repo/C:\cygwin\home\USER\repo'

The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]

Extract the needed code into compat/win32/path-utils.[ch] and use it
for cygwin as well.

Reported-by: Steven Penny <svnpenn@gmail.com>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>

Changes since v3:
  Rename e.g. mingw_skip_dos_drive_prefix() into
              win32_skip_dos_drive_prefix()
       as suggested by Dscho, thanls for that.
  Add a tweak in t5601 for cygwin.

The test suite passes now on cygwin.
The "Git for Windows" build was tested was tested on
the gfw/master, with this commit cherry-picked on top.


---
 compat/cygwin.c           | 19 -------------------
 compat/cygwin.h           |  2 --
 compat/mingw.c            | 29 +----------------------------
 compat/mingw.h            | 20 --------------------
 compat/win32/path-utils.c | 28 ++++++++++++++++++++++++++++
 compat/win32/path-utils.h | 20 ++++++++++++++++++++
 config.mak.uname          |  3 ++-
 git-compat-util.h         |  3 ++-
 t/t5601-clone.sh          |  2 +-
 9 files changed, 54 insertions(+), 72 deletions(-)
 delete mode 100644 compat/cygwin.c
 delete mode 100644 compat/cygwin.h
 create mode 100644 compat/win32/path-utils.c
 create mode 100644 compat/win32/path-utils.h

diff --git a/compat/cygwin.c b/compat/cygwin.c
deleted file mode 100644
index b9862d606d..0000000000
--- a/compat/cygwin.c
+++ /dev/null
@@ -1,19 +0,0 @@
-#include "../git-compat-util.h"
-#include "../cache.h"
-
-int cygwin_offset_1st_component(const char *path)
-{
-	const char *pos = path;
-	/* unc paths */
-	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
-		/* skip server name */
-		pos = strchr(pos + 2, '/');
-		if (!pos)
-			return 0; /* Error: malformed unc path */
-
-		do {
-			pos++;
-		} while (*pos && pos[0] != '/');
-	}
-	return pos + is_dir_sep(*pos) - path;
-}
diff --git a/compat/cygwin.h b/compat/cygwin.h
deleted file mode 100644
index 8e52de4644..0000000000
--- a/compat/cygwin.h
+++ /dev/null
@@ -1,2 +0,0 @@
-int cygwin_offset_1st_component(const char *path);
-#define offset_1st_component cygwin_offset_1st_component
diff --git a/compat/mingw.c b/compat/mingw.c
index 34b3880b29..b459e1a291 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -350,7 +350,7 @@ static inline int needs_hiding(const char *path)
 		return 0;
 
 	/* We cannot use basename(), as it would remove trailing slashes */
-	mingw_skip_dos_drive_prefix((char **)&path);
+	win32_skip_dos_drive_prefix((char **)&path);
 	if (!*path)
 		return 0;
 
@@ -2275,33 +2275,6 @@ pid_t waitpid(pid_t pid, int *status, int options)
 	return -1;
 }
 
-int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
-
-int mingw_offset_1st_component(const char *path)
-{
-	char *pos = (char *)path;
-
-	/* unc paths */
-	if (!skip_dos_drive_prefix(&pos) &&
-			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
-		/* skip server name */
-		pos = strpbrk(pos + 2, "\\/");
-		if (!pos)
-			return 0; /* Error: malformed unc path */
-
-		do {
-			pos++;
-		} while (*pos && !is_dir_sep(*pos));
-	}
-
-	return pos + is_dir_sep(*pos) - path;
-}
-
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
 {
 	int upos = 0, wpos = 0;
diff --git a/compat/mingw.h b/compat/mingw.h
index 8c24ddaa3e..30d9fb3e36 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -443,32 +443,12 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) \
-	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-int mingw_skip_dos_drive_prefix(char **path);
-#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
-static inline int mingw_is_dir_sep(int c)
-{
-	return c == '/' || c == '\\';
-}
-#define is_dir_sep mingw_is_dir_sep
-static inline char *mingw_find_last_dir_sep(const char *path)
-{
-	char *ret = NULL;
-	for (; *path; ++path)
-		if (is_dir_sep(*path))
-			ret = (char *)path;
-	return ret;
-}
 static inline void convert_slashes(char *path)
 {
 	for (; *path; path++)
 		if (*path == '\\')
 			*path = '/';
 }
-#define find_last_dir_sep mingw_find_last_dir_sep
-int mingw_offset_1st_component(const char *path);
-#define offset_1st_component mingw_offset_1st_component
 #define PATH_SEP ';'
 extern char *mingw_query_user_email(void);
 #define query_user_email mingw_query_user_email
diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c
new file mode 100644
index 0000000000..d9d3641de8
--- /dev/null
+++ b/compat/win32/path-utils.c
@@ -0,0 +1,28 @@
+#include "../../git-compat-util.h"
+
+int win32_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+
+int win32_offset_1st_component(const char *path)
+{
+	char *pos = (char *)path;
+
+	/* unc paths */
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+		/* skip server name */
+		pos = strpbrk(pos + 2, "\\/");
+		if (!pos)
+			return 0; /* Error: malformed unc path */
+
+		do {
+			pos++;
+		} while (*pos && !is_dir_sep(*pos));
+	}
+
+	return pos + is_dir_sep(*pos) - path;
+}
diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
new file mode 100644
index 0000000000..0f70d43920
--- /dev/null
+++ b/compat/win32/path-utils.h
@@ -0,0 +1,20 @@
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+int win32_skip_dos_drive_prefix(char **path);
+#define skip_dos_drive_prefix win32_skip_dos_drive_prefix
+static inline int win32_is_dir_sep(int c)
+{
+	return c == '/' || c == '\\';
+}
+#define is_dir_sep win32_is_dir_sep
+static inline char *win32_find_last_dir_sep(const char *path)
+{
+	char *ret = NULL;
+	for (; *path; ++path)
+		if (is_dir_sep(*path))
+			ret = (char *)path;
+	return ret;
+}
+#define find_last_dir_sep win32_find_last_dir_sep
+int win32_offset_1st_component(const char *path);
+#define offset_1st_component win32_offset_1st_component
diff --git a/config.mak.uname b/config.mak.uname
index 3ee7da0e23..60876e26f4 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -187,7 +187,7 @@ ifeq ($(uname_O),Cygwin)
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
-	COMPAT_OBJS += compat/cygwin.o
+	COMPAT_OBJS += compat/win32/path-utils.o
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),FreeBSD)
@@ -527,6 +527,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/winansi.o \
+		compat/win32/path-utils.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
 		compat/win32/dirent.o
 	BASIC_CFLAGS += -DWIN32 -DPROTECT_NTFS_DEFAULT=1
diff --git a/git-compat-util.h b/git-compat-util.h
index 09b0102cae..5702556c89 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -193,10 +193,11 @@
 #endif
 
 #if defined(__CYGWIN__)
-#include "compat/cygwin.h"
+#include "compat/win32/path-utils.h"
 #endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
+#include "compat/win32/path-utils.h"
 #include "compat/mingw.h"
 #elif defined(_MSC_VER)
 #include "compat/msvc.h"
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8bbc7068ac..d6948cbdab 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -487,7 +487,7 @@ test_clone_url () {
 	expect_ssh "$@"
 }
 
-test_expect_success !MINGW 'clone c:temp is ssl' '
+test_expect_success !MINGW,!CYGWIN 'clone c:temp is ssl' '
 	test_clone_url c:temp c temp
 '
 
-- 
2.20.0

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

[Achim Gratz]

[Trying to revive that discussion]

tboegi@web.de writes:
> The cygwin layer "knows" that "C:\cygwin" is an absolute path,
> but the new string operation does not.

Then use the Cygwin API to produce the corresponding POSIX path and use
that.  Also, why does Git not use POSIX realpath on systems where it's
available?

> "git clone <url> C:\cygwin\home\USER\repo" fails like this:
> fatal: Invalid path '/home/USER/repo/C:\cygwin\home\USER\repo'
>
> The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
> is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
> in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]

No.  I don't care what MinGW does, but keep these heuristics out of Cygwin.

> Extract the needed code into compat/win32/path-utils.[ch] and use it
> for cygwin as well.

Can you please remove that Win32 stuff from Cygwin again?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

DIY Stuff:
http://Synth.Stromeko.net/DIY.html