path_conv: be a lot more careful when to keep a trailing slash by dscho · Pull Request #103 · msys2/msys2-runtime

dscho · 2022-09-20T19:51:50Z

On FAT file systems, the normalized path is used to fake inode numbers. As a result, Y:\directory\ and Y:\directory have different inode numbers!!!

One consequence of this bug has been reported on the Git for Windows: the find and rm commands can error out on FAT file systems with very confusing "No such file or directory" errors, for no good reason (see git-for-windows/git#1497 and git-for-windows/git#1536).

This bug had been fixed in Cygwin as early as 1997... and we reintroduced it in MSYS2.

Let's fix it again.

This patch has been carried in Git for Windows for several years, with no further bug reports.

While at it, replace the very sad, sad, grammatically-incorrect one-liner of a commit message with a detailed explanation of the context, the intent and the justification of the code change.

mati865 · 2022-09-20T20:31:29Z

While at it, replace the very sad, sad, grammatically-incorrect one-liner of a commit message with a detailed explanation of the context, the intent and the justification of the code change.

Seems the commit with this change is missing.

dscho · 2022-09-21T06:06:04Z

While at it, replace the very sad, sad, grammatically-incorrect one-liner of a commit message with a detailed explanation of the context, the intent and the justification of the code change.

Seems the commit with this change is missing.

@mati865 This is not a separate commit, and it is not missing, it is bb6e68e.

The special form amend! is generated by git commit --fixup reword:<commit> (which usually does not allow committing any actual code changes, but you can git commit --amend it and it works as intended, which is what I did here).

path_conv: special-case root directory to have trailing slash When converting `/c/` to `C:\`, the trailing slash is actually really necessary, as `C:` is not an absolute path. We must be very careful to do this only for root directories, though. If we kept the trailing slash also for, say, `/y/directory/`, we would run into the following issue: On FAT file systems, the normalized path is used to fake inode numbers. As a result, `Y:\directory\` and `Y:\directory` have different inode numbers!!! This would result in very non-obvious symptoms. Back when we were too careless about keeping the trailing slash, it was reported to the Git for Windows project that the `find` and `rm` commands can error out on FAT file systems with very confusing "No such file or directory" errors, for no good reason. During the original investigation, Vasil Minkov pointed out in git-for-windows/git#1497 (comment), that this bug had been fixed in Cygwin as early as 1997... and the bug was unfortunately reintroduced into early MSYS2 versions. Signed-off-by: Johannes Schindelin <[email protected]>

mati865 · 2022-09-21T16:24:40Z

Oh sorry, I was under impression you intended to change in-code comment.

dscho · 2022-09-21T20:29:37Z

Oh sorry, I was under impression you intended to change in-code comment.

@mati865 are you referring to this comment?

/* Do not add trailing \ to UNC device names like \\.\a: */

If so, this is actually a comment in Cygwin (i.e. not specific to MSYS2, and hence outside the scope of this here PR): https://github.com/cygwin/cygwin/blob/master/winsup/cygwin/path.cc#L1253

mati865 · 2022-09-21T21:43:23Z

Yeah, I have misunderstood the intention.

dscho · 2022-11-08T15:26:21Z

@lazka could you have a look, please?

lazka · 2022-11-08T18:23:36Z

Because I was confused too -> this amends 4676ec4

I'm afraid I don't quite understand why the restoring of the trailing slash has anything to do with PC_NOFULL. (and what does this function itself even do?.. and many more questions..)

But since it reduces the diff to cygwin while the originally intended behavior change is still there this looks good to me.

lazka · 2022-11-08T18:33:45Z

In light of #115, which is partly triggered by 4676ec4, do we know what 4676ec4 was trying to fix? i.e. why is / -> C:/msys64/ good and / -> C:/msys64 bad? I'm not trying to block this with my question though, just wondering..

dscho · 2022-11-09T08:23:19Z

Because I was confused too -> this amends 4676ec4

Good point, sorry for not linking to this myself. It is of course part of the context required to understand this change.

I'm afraid I don't quite understand why the restoring of the trailing slash has anything to do with PC_NOFULL. (and what does this function itself even do?.. and many more questions..)

PC_NOFULL tells path_conv to keep relative paths relative.

So a valid question is: why is this need_directory flag only heeded in that mode?

I'll dig deeper into this question in a moment, but let me first note that this is upstream Cygwin's current behavior: the need_directory flag is set if the user looked for a directory, is then used to append a slash if the file attributes could not be determined and eventually is used only in the case we want to convert a relative path to a relative path.

Note that this PC_NOFULL mode is triggered by CCP_RELATIVE which is used almost everywhere anyway.

Now, let me dig a bit deeper where that logic comes from. The monster patch 3f3bd10 touches that part, but really only changes indentation (check for yourself via git show --patience -w 3f3bd10104550243781f0b4d9248975e35d91ac7 -- winsup/cygwin/path.cc), likewise 1870c68 only reformats said code, same for 81adfe2. The parent commit still has the same logic: the slash is only appended if opt & PC_NOFULL and need_directory are true. Re-starting the analysis from there (via git log -w -L 1246,1261:winsup/cygwin/path.cc 81adfe28623b8a40d8b946c7e9355e3e78847cf8^), we are led to believe that d3c7e9d introduced that logic, but that's not so: it was moved from here to here. And if we try to dig deeper from there, we come to a hard stop at 1fd5e00, which is super unhelpful because it is a wholesale tar import from the year 2000 when we have been told that the bug fix happened in 1997.

So I guess that the best thing we can do is to trust Cygwin which has kept this need_directory handling guarded by the PC_NOFULL flag, and we should not deviate from it, in particular because we know that this deviation caused a bug where the inode calculation (necessary on non-NTFS drives) would be inconsistent causing find to be surprised to not find a file that it had just found.

But since it reduces the diff to cygwin while the originally intended behavior change is still there this looks good to me.

Indeed. That was my primary goal here.

In light of #115, which is partly triggered by 4676ec4, do we know what 4676ec4 was trying to fix? i.e. why is / -> C:/msys64/ good and / -> C:/msys64 bad? I'm not trying to block this with my question though, just wondering..

The earliest reference I can find is msys2/MSYS2-packages@c560737. It is painful to think of the reason why we cannot ask the author of that patch for more information, and the commit message does not really provide said information, either.

Having said that, this commit talks about the goal (without providing any motivating context) but does not move the check out of the PC_NOFULL clause. That move was done in msys2/MSYS2-packages@175489d513f62aa456d9d4699692d1d734a6ba74. And in that patch message we finally get the reasoning for moving the logic out of the PC_NOFULL clause:

This fixes path conversion of the root mount, e.g.: '/' -> 'C:/msys64' .. in this example, the result of converting '/' would be
'C:\msys64' when it clearly should be 'C:\msys64', I say clearly because it is obvious that converting a path that ends in / should result in a path ending in \ every time.

On the face of it, this is plausible. But it becomes a lot less plausible when you keep in mind that this path conversion is also used for the inode emulation that calculates a hash from the absolute path and therefore requires the conversion to be consistent whether the original path has the trailing slash or not. We do not want /directory to have "a different inode" than /directory/, after all.

dscho · 2022-11-11T13:34:29Z

@lazka what do you think? Run with this, reducing the diff relative to Cygwin?

lazka · 2022-11-11T13:53:56Z

Thanks for the thorough investigation (I tried to do something similar but gave up).

I'm fine with either applying this, or removing the original patch completely.

dscho · 2022-11-11T20:18:47Z

I'm fine with either applying this, or removing the original patch completely.

I am reluctant to remove the original patch entirely because I suspect that there was some merit in doing it in the first place.

lazka · 2022-11-19T19:48:29Z

I guess I found the reason why this was added in the first, place. The GCC build depends on slashes surviving: msys2/MINGW-packages#14163 Maybe it can be fixed there.

In theory this doesn't make a difference because posix_to_win32_path() is only called with rooted/absolute paths, but as pointed out in msys2#103 PC_NOFULL will preserve the trailing slash of unix paths (for some reason). See "cygpath -m /bin/" (preserved) vs "cygpath -am /bin/" (dropped) One use case where we need to trailing slashes to be preserved is the GCC build system: https://github.com/gcc-mirror/gcc/blob/6d82e0fea5f988e829912a/gcc/Makefile.in#L2314 The Makefile appends a slash to the prefixes and the C code doing relocation will treat the path as a directory if there is a trailing slash. See msys2/MINGW-packages#14173 for details. With this change all our MSYS2 path_conv tests pass again.

lazka · 2022-11-20T13:08:15Z

I guess I found the reason why this was added in the first, place. The GCC build depends on slashes surviving: msys2/MINGW-packages#14163 Maybe it can be fixed there.

A less invasive "fix": #117

In theory this doesn't make a difference because posix_to_win32_path() is only called with rooted/absolute paths, but as pointed out in #103 PC_NOFULL will preserve the trailing slash of unix paths (for some reason). See "cygpath -m /bin/" (preserved) vs "cygpath -am /bin/" (dropped) One use case where we need to trailing slashes to be preserved is the GCC build system: https://github.com/gcc-mirror/gcc/blob/6d82e0fea5f988e829912a/gcc/Makefile.in#L2314 The Makefile appends a slash to the prefixes and the C code doing relocation will treat the path as a directory if there is a trailing slash. See msys2/MINGW-packages#14173 for details. With this change all our MSYS2 path_conv tests pass again.

In theory this doesn't make a difference because posix_to_win32_path() is only called with rooted/absolute paths, but as pointed out in msys2/msys2-runtime#103 PC_NOFULL will preserve the trailing slash of unix paths (for some reason). See "cygpath -m /bin/" (preserved) vs "cygpath -am /bin/" (dropped) One use case where we need to trailing slashes to be preserved is the GCC build system: https://github.com/gcc-mirror/gcc/blob/6d82e0fea5f988e829912a/gcc/Makefile.in#L2314 The Makefile appends a slash to the prefixes and the C code doing relocation will treat the path as a directory if there is a trailing slash. See msys2/MINGW-packages#14173 for details. With this change all our MSYS2 path_conv tests pass again.

In theory this doesn't make a difference because posix_to_win32_path() is only called with rooted/absolute paths, but as pointed out in #103 PC_NOFULL will preserve the trailing slash of unix paths (for some reason). See "cygpath -m /bin/" (preserved) vs "cygpath -am /bin/" (dropped) One use case where we need to trailing slashes to be preserved is the GCC build system: https://github.com/gcc-mirror/gcc/blob/6d82e0fea5f988e829912a/gcc/Makefile.in#L2314 The Makefile appends a slash to the prefixes and the C code doing relocation will treat the path as a directory if there is a trailing slash. See msys2/MINGW-packages#14173 for details. With this change all our MSYS2 path_conv tests pass again.

In theory this doesn't make a difference because posix_to_win32_path() is only called with rooted/absolute paths, but as pointed out in msys2/msys2-runtime#103 PC_NOFULL will preserve the trailing slash of unix paths (for some reason). See "cygpath -m /bin/" (preserved) vs "cygpath -am /bin/" (dropped) One use case where we need to trailing slashes to be preserved is the GCC build system: https://github.com/gcc-mirror/gcc/blob/6d82e0fea5f988e829912a/gcc/Makefile.in#L2314 The Makefile appends a slash to the prefixes and the C code doing relocation will treat the path as a directory if there is a trailing slash. See msys2/MINGW-packages#14173 for details. With this change all our MSYS2 path_conv tests pass again.

In theory this doesn't make a difference because posix_to_win32_path() is only called with rooted/absolute paths, but as pointed out in #103 PC_NOFULL will preserve the trailing slash of unix paths (for some reason). See "cygpath -m /bin/" (preserved) vs "cygpath -am /bin/" (dropped) One use case where we need to trailing slashes to be preserved is the GCC build system: https://github.com/gcc-mirror/gcc/blob/6d82e0fea5f988e829912a/gcc/Makefile.in#L2314 The Makefile appends a slash to the prefixes and the C code doing relocation will treat the path as a directory if there is a trailing slash. See msys2/MINGW-packages#14173 for details. With this change all our MSYS2 path_conv tests pass again.

In theory this doesn't make a difference because posix_to_win32_path() is only called with rooted/absolute paths, but as pointed out in msys2#103 PC_NOFULL will preserve the trailing slash of unix paths (for some reason). See "cygpath -m /bin/" (preserved) vs "cygpath -am /bin/" (dropped) One use case where we need to trailing slashes to be preserved is the GCC build system: https://github.com/gcc-mirror/gcc/blob/6d82e0fea5f988e829912a/gcc/Makefile.in#L2314 The Makefile appends a slash to the prefixes and the C code doing relocation will treat the path as a directory if there is a trailing slash. See msys2/MINGW-packages#14173 for details. With this change all our MSYS2 path_conv tests pass again.

dscho self-assigned this Sep 20, 2022

dscho marked this pull request as ready for review September 20, 2022 20:17

dscho force-pushed the trailing-slash branch from bb6e68e to 81407a6 Compare September 21, 2022 06:22

dscho requested review from jeremyd2019 and lazka September 23, 2022 09:14

lazka approved these changes Nov 11, 2022

View reviewed changes

dscho merged commit 358b454 into msys2:msys2-3_3_6-release Nov 11, 2022

dscho deleted the trailing-slash branch November 11, 2022 20:19

lazka mentioned this pull request Nov 19, 2022

gcc: rebuild msys2/MINGW-packages#14163

Closed

lazka mentioned this pull request Nov 20, 2022

GCC build depends on trailing slashes surviving path conversion msys2/MINGW-packages#14173

Closed

lazka mentioned this pull request Nov 20, 2022

msys2_path_conv: pass PC_NOFULL to path_conv #117

Merged

Uh oh!

Conversation

dscho commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mati865 commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Sep 21, 2022

Uh oh!

mati865 commented Sep 21, 2022

Uh oh!

dscho commented Sep 21, 2022

Uh oh!

mati865 commented Sep 21, 2022

Uh oh!

dscho commented Nov 8, 2022

Uh oh!

lazka commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lazka commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Nov 9, 2022

Uh oh!

dscho commented Nov 11, 2022

Uh oh!

lazka commented Nov 11, 2022

Uh oh!

dscho commented Nov 11, 2022

Uh oh!

lazka commented Nov 19, 2022

Uh oh!

lazka commented Nov 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dscho commented Sep 20, 2022 •

edited

Loading

mati865 commented Sep 20, 2022 •

edited

Loading

lazka commented Nov 8, 2022 •

edited

Loading

lazka commented Nov 8, 2022 •

edited

Loading