Cygwin Git with Windows paths

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

On Sun, Nov 18, 2018 at 07:21:58AM -0800, Steven Penny 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)

On Sun, Nov 18, 2018 at 9:41 AM Torsten Bögershausen wrote:

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?

On Sun, Nov 18, 2018 at 10:23:19AM -0600, Steven Penny wrote:

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.

On Sun, Nov 18, 2018 at 11:15 AM Torsten Bögershausen wrote:

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'

On Sun, Nov 18, 2018 at 11:34:04AM -0600, Steven Penny 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 ?

On Sun, Nov 18, 2018 at 12:28 PM Torsten Bögershausen wrote:

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?

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

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.

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

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?

On 2018-11-19 04:33, Junio C Hamano wrote:

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

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

On Sun, Nov 18, 2018 at 11:21 PM Torsten Bögershausen wrote:

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

On 20.11.18 01:17, Steven Penny 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

On Tue, Nov 20, 2018 at 4:36 AM Torsten Bögershausen wrote:

On November 18, 2018 22:33, Junio C Hamano wrote:

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

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

On Mon, Nov 26, 2018 at 11:32 AM wrote:

I can confirm that this fixes the issue.

Thank you!

tboegi@web.de writes:

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

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.

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.

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.

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

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

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

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

On Mon, Nov 26, 2018 at 7:16 PM Junio C Hamano wrote:

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?

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.

Steven Penny <svnpenn@gmail.com> writes:

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.

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'd defer to Windows folks to decide if a unified win.h is a good idea.

Thanks.

On Mon, Nov 26, 2018 at 11:23 PM Junio C Hamano wrote:

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.

Hi Junio,

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

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.

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

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

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

=====

Hi J.H.,

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

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

On 2018-11-28 09:46, Johannes Schindelin wrote:

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

Hi J.H.

On Wed, 28 Nov 2018, Houder wrote:

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.

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

Junio C Hamano writes:

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.

Hi Torsten,

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

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

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

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

tboegi@web.de writes:

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.

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

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

Hi Torsten,

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

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

On Fri, Dec 7, 2018 at 11:04 AM wrote:

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

Hi Steven,

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

On Fri, 7 Dec 2018, Steven Penny wrote:

I find all of those horrible.

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

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

On Mon, Dec 10, 2018 at 2:46 AM Johannes Schindelin wrote:

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.

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.

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

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

Hi Steven,

On Mon, 10 Dec 2018, Steven Penny wrote:

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

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.

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.

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

Ciao, Johannes

On Tue, Dec 11, 2018 at 7:39 AM Johannes Schindelin wrote:

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

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.

Quoting from Wikipedia (emphasis mine):

and:

http://wikipedia.org/wiki/Windows_API

and quoting directly from Microsoft:

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.

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.

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

Am 12.12.18 um 01:42 schrieb Steven Penny:

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

On Wed, Dec 12, 2018 at 1:29 AM Johannes Sixt wrote:

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.

Steven Penny <svnpenn@gmail.com> writes:

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.

Hi Hannes,

On Wed, 12 Dec 2018, Johannes Sixt wrote:

FWIW I agree with you.

Thanks, Dscho

On Fri, Dec 7, 2018 at 4:51 PM Steven Penny <svnpenn@gmail.com> wrote:

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

Hi Elijah,

On Tue, 11 Dec 2018, Elijah Newren wrote:

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

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

Hi Torsten,

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

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

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

The rest looks correct to me, Johannes

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

On Sat, Dec 8, 2018 at 9:11 AM wrote:

latest patch still fixes original issue - thanks

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

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

tboegi@web.de writes:

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.

Yup, that is much more important.

Thanks.

Hi Torsten,

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

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

Thanks, Dscho

On Mon, Dec 10, 2018 at 09:32:03AM +0100, Johannes Schindelin wrote:

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

Hi Torsten,

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

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

That makes sense - thanks for the suggestion & testing.

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

[Trying to revive that discussion]

tboegi@web.de writes:

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?

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

Can you please remove that Win32 stuff from Cygwin again?

Regards, Achim.