POSIX-to-Windows path conversion discards trailing backslash?

[MarcelNehring]

• I was not able to find an open or closed issue matching what I'm seeing

Setup

• Which version of Git for Windows are you using? Is it 32-bit or 64-bit?

$ git --version --build-options

git version 2.17.1.windows.2
cpu: x86_64
built from commit: a60968cf435951d9411fc0f980a2e362d5cccea2
sizeof-long: 4

• Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?

$ cmd.exe /c ver

Microsoft Windows [Version 10.0.17134.48]

• What options did you set as part of the installation? Or did you choose the
  defaults?

$ cat /etc/install-options.txt

Editor Option: VIM
Path Option: Cmd
SSH Option: OpenSSH
CURL Option: OpenSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Disabled

Details

• Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

Bash

• What commands did you run to trigger this issue? If you can provide a
  Minimal, Complete, and Verifiable example
  this will help us understand the issue.

$ MY_TMP="$(mktemp -d)"
$ echo $MY_TMP
/tmp/tmp.GVpowzwlaC
$ git checkout-index --prefix="$MY_TMP/" -f -- "test.cpp"

• What did you expect to occur after running these commands?

The file test.cpp being checked out into /tmp/tmp.GVpowzwlaC/test.cpp

• What actually happened instead?

The file test.cpp being checked out into /tmp/tmp.GVpowzwlaCtest.cpp

Not sure if I do something wrong or if maybe POSIX-to-Windows path conversion discards a trailing backslash? The issue appeared after I upgraded from v2.16.0(2). Note that printing the variable seems to work as expected.

$ echo $MY_TMP/
/tmp/tmp.GVpowzwlaC/

[elieux]

Related? git-for-windows/msys2-runtime#17

[dscho]

Yep. Still under discussion, I pulled in @mingwandroid for a review, and so far we haven't gotten to the review part yet, but I am optimistic.

[dscho]

I forgot to mention the current workaround: use cygpath -am to convert to a Windows-style path with forward slashes, then append a slash, then pass it on to the non-MSYS2 executable. That side-steps the erroneous conversion.

[MarcelNehring]

Ok, thanks for the details. Good to see this is being worked on 😃

[dscho]

@MarcelNehring oh, I did not mean to let you off the hook. You owe me at least to test this. So far, I worked a lot, and I would like this to be a bit more fair than you just benefiting from my work. Ideally, you would review the patch and research enough to assess whether it is incomplete.

[MarcelNehring]

Its not just me benefiting from your work. All users of git for windows benefit a lot from your work. Testing it sure is no problem. I am not so sure about a review, though, because I am not sure if I understand enough of the problem and the code.

From what I understood there was this commit from 2016 which broke FAT inode emulation by modifying path_conv so that it keeps trailing slashes. This was because callers to path_conv relied on the fact that it would produce a normalized path. To fix the FAT problem you partly reverted the change. This, however, lead to the problem I am experiencing because other callers of path_conv relying on it to keep trailing slashes would get a normalized path with trailing slashes removed. To finally fix this situation you now introduced a flag to tell path_conv if it should keep trailing slashes. Is this understanding correct?

[MarcelNehring]

Ok, from what I understand your fix looks good to me. But I cannot tell which callers need to be adapted.

What I do not understand, however, is why initially the trailing slash was only re-appended if PC_NOFULL was set. I also do not understand why the path is normalized during conversion. But I guess there is a reason for that.

[mingwandroid]

Your assessment is incorrect.

I made path_conv do exactly what you would expect it to do. Convert the path while making minimal changes orthogonal to path conversion. I should've fixed up some call sites for sure though.

What I mean by this is that is you pass trailing slashes you get them back and if you don't you don't get them added.

In the cygwin codebase this behaviour differed depending on where the folder existed or not which is clearly a bug. This is infact the bug I really was trying to fix here but cygwin rejected this as 'not a bug'.

And now that's become some big deal for the the maintainer of Git for Windows who seems to consider it acceptable to use commit messages to present opinion as fact and to denegrate others and their work and in GitHub comments to ppost messages berating people who do not speak English and who are therefore very tetse. But whatever these are his issues to figure out and address.

[MarcelNehring]

I'd like to avoid being dragged into a fight here. As I said, my knowledge of the code base is non-existing. So I hope I did not offend anybody. I did not intend to.

I made path_conv do exactly what you would expect it to do. Convert the path while making minimal changes orthogonal to path conversion.

That is what I meant when I said I do not understand why conversion also performs normalization in some cases. For me this looks odd. Wether it is a bug or not I cannot tell, however. I don't know enough about the code.

[dscho]

I do not understand why conversion also performs normalization in some cases. For me this looks odd. Wether it is a bug or not I cannot tell, however. I don't know enough about the code.

It would be most helpful if you familiarized yourself enough with the code to be able to tell.

[dscho]

I made path_conv do exactly what you would expect it to do. Convert the path while making minimal changes orthogonal to path conversion. I should've fixed up some call sites for sure though.

What I mean by this is that is you pass trailing slashes you get them back and if you don't you don't get them added.

In the cygwin codebase this behaviour differed depending on where the folder existed or not which is clearly a bug. This is infact the bug I really was trying to fix here but cygwin rejected this as 'not a bug'.

I think you probably misunderstood why the Cygwin developers rejected your patch or bug report, or alternatively failed to make convincing enough a case. It does take time and effort to convince other people of intricate issues, but in my experience the Cygwin developers have been nothing but reasonable and eager to help.

denegrate others and their work and in GitHub comments to ppost messages berating people who do not speak English and who are therefore very tetse.

@mingwandroid I think this is a serious misrepresentation of what I said. It is unnecessarily hurtful and an insult.

Let me try to make myself clear: @Alexpux responded to my communication, where I spent hours and hours of my time to really drive home what my concerns are, with very, very short messages. They were so short, in fact, to not only be rude, but also really, really leading to a very unhealthy communication. They did nothing to answer my questions, nothing to help me understand what he wanted, nothing to help consolidate efforts into a common direction.

You can't do that in Open Source, not if you want to collaborate with others. Communication is key, and simply denying to communicate will shut that down pretty well (in a very negative way) and harm your connection and ability to collaborate. That is exactly what happened with the MSYS2 project when it comes to my contributions: I was really eager to help, and I was actively frustrated into giving up. That is what happened, and to blame this on me, when I communicated clearly and frequently and was responsive and clarified when asked and kept the communication alive, all of it a one-sided effort, is quite a bit fresh and very much insulting.

Now, I see you do not want to help this effort of mine, exemplified in this here PR, to make the MSYS2 runtime better, so I better just face reality and give up on trying to get productive on the MSYS2 runtime by communicating with you, too. Or are you actually interested in making this code good enough to eventually get integrated into Cygwin? In that case, I am still willing. You said you have little time to spend on MSYS2. Maybe if you could spend it on a productive review of my patches, we could get somewhere.

[MarcelNehring]

It would be most helpful if you familiarized yourself enough with the code to be able to tell.

Absolutely, but this would take quite some time. Time I am unable to spend right now, unfortunately. 😞 For me the code is very hard to grasp.

[PhilipOakley]

Hi Ray (@mingwandroid),

Thank you for clarifying your conversion policy and that cygwin had a different view (their 'not a bug' response - is there a bug ID for it?).

I'm guessing that the cygwin thing maybe due to percieved d/f issues should a file exist in place of a directory(folder), but I just don't know - it may be something else.

I guess that dscho, in this case, is stuck between a rock and hard place of two equally valid, but different views about the conversion, each of which has it's own consequences when used in different parts of the 'GfW' code base. Which then leads to the 'third way' (compromise?) solution of a flag as to which way to jump.

As an aside, the divergence of usage resulting in confict is an extension of the CVF (Competing Values Framework) which is/was used to model management styles. Software (itself) reportedly fits the Open System model / Adhocracy quadrant, though the conflict here probably starts at the Rational Goal, or even Internal Process quadrants.

Its sucess (CVF's) is the distillation down to a 2-way 2x2 dilemma, as seen here in the two approaches to normalisation while converting.

[MarcelNehring]

I tried to test the change using the pre-release v2.18.0-rc1.windows.1. Unfortunately git fails to start after running the installer. It complains about a missing libpcre2-8-0.dll. I tried the 32 and 64 bit installers and the 64 bit portable version.

-Edit-
I get the same error after trying to downgrade to 2.16.2. So seems to be a local problem.

-Edit 2-
Uninstalling git and then reinstalling 2.16.2 fixed the problem with the missing libpcre2-8-0.dll.

[mingwandroid]

@PhilipOakley, the discussion is here: https://sourceware.org/ml/cygwin/2016-01/msg00492.html and it continues on 2016-02 also but not a lot happens, it just petered out.

[dscho]

@MarcelNehring even v2.18.0-rc1(3), which fixes the libpcre2 problem, does not contain the fix for the issue discussed in this ticket. There is simply too much work left for only one person to complete. Everybody else bows out just like you did.

[dscho]

v2.18.0-rc2 works, in my hands at least.

[MarcelNehring]

Thanks for informing about the new RC. Unfortunately I cannot confirm that it is working for me. When executing the commands from my first post the file still gets checked-out to /tmp/tmp.GVpowzwlaCtest.cpp instead of /tmp/tmp.GVpowzwlaC/test.cpp. I tried the 64 bit installer. I also tried removing my old installation first.

$ git --version
git version 2.18.0.rc2.windows.1

Additionally I noticed that it seems $GIT_DIR is no longer set in a pre-commit hook.

[dscho]

Unfortunately I cannot confirm that it is working for me.

sigh Couldn't you have tested this earlier? Now v2.18.0 is out, with this.