Fix incorrect path conversion (trailing slash) #17
Conversation
The problem with the original approach is not that it is completely bogus. After all, when you pass the option --prefix=/tmp/ to Git, you do want to end up with a trailing slash. The real problem with the original approach is that it simply changed path_conv, completely oblivious and careless about other users of path_conv. That was really wrong, of course, and cost us time, sweat and tears. The appropriate approach is to *not* affect other users, but instead introduce a flag that we use in *our* caller, so that everybody gets what they want: - emulation of inodes on FAT by calculating the hash on the normalized path: works. - MSYS2's conversion of "POSIX" paths to Windows paths: works. Signed-off-by: Johannes Schindelin <[email protected]>
|
@mingwandroid could you have a look, please? |
|
IMHO I should've gone further and fixed the callers. Your fix for FAT should take place at the inode calculation site. I believe your 'not completely bogus' statement (thanks! almost a complement) shows that you are coming round to seeing this with a bit more clarity than before. A function should do the most obvious things by default when such a thing exists and this is a clear case of that. Adding a flag is ugly. So while I'll say LGTM I mean "ok it's better than the bug and I'm not going to have time to fix this issue as I think it should be fixed, at the site of the old assumption we want to break in the interests of code simplicity and sanity"
Disagree entirely. @Alexpux I'd go ahead and merge this anyway. |
@mingwandroid That assumes that it is the responsibility of the MSYS2 fork to take care of the inode calculation. But that is not the case. It is Cygwin's decision to define the behavior of the normalization. It's not even your decision, much as you pretend it to be.
You mean a "compliment", right? And I would not call it that. I am not "coming around" to see how your approach is correct. It still is not. I am coming around to understand the problem you tried to solve. The problem, as I see it, is that when you pass a parameter to a non-MSYS2 program that looks like it is a path prefix ( And this problem is simply introduced by MSYS2: it reuses Cygwin's And your workaround for the bug simply breaks too much to be a real fix. It breaks basic assumptions by Cygwin's code, and is therefore no good.
When you design an architecture, you are at liberty to define what a function should do, and to use your understanding of what is obvious and what is not to inform on that design. This design is neither yours, nor yours to change. Adding a flag is the least intrusive way to achieve what you want to achieve, when the original API was not designed to give you what you want.
Well, you broke the Cygwin runtime. That is totally wrong. You can't just simply change a central part of a piece of software to do things very differently than before, without adjusting all the callers to expect the new behavior. And you did not adjust even a single caller. The better way to change this is to not break anything else.
You misunderstand. I disagree with @Alexpux on too many things, including his terse (to the point of being unhelpful) communication, therefore I had to abandon my plans to integrate Git for Windows' changes into MSYS2. Therefore, Git for Windows maintains its own fork of the Cygwin runtime that is based on the MSYS2 runtime's patches, but diverges, and will probably diverge worse, still. I did not give up hope yet to collaborate with the Cygwin project on integrating at least a large part of the modifications (probably hidden behind options e.g. in |
|
@mingwandroid maybe we can turn this thread around to a much more productive direction, though. It is my fault, for not specifying precisely what I would like from you. Sorry about that. I would like you to have a look whether this workaround would match what you recall from the original problem (you seemed to vaguely remember something from GCC's build scripts, but I fail to see how that relates to this here problem, as nothing in GCC's build scripts shouts "MINGW" to me, therefore So: can you please try to remember the details of the bug, and then apply this to a review whether this patch is correct? I really do not need any style or design review, I really only need you to confirm that it does what it is supposed to, or in the alternative to point out to me where it goes wrong. Thank you. |
|
Yes I can. I was making native Windows cases Linux targeting cross compilers and glibc likes job backwards seps.
MSYS2s runtime is a fork of cygwin specifically so we can make such changes. You are right I should've fixed all the callsites though but I'm not interested in any moral rights style argument about design in relation to this at all. I've already said this could be merged though but I have less and less time for MSYS2 these days for which I'm truly sorry so I cannot do a good review of it. |
I am truly sorry: my English parsing elude me. I do not understand what that sentence means. If anybody can help me, that would be awesome. Be that as it may, I'll just merge this so that we can have the fix soon. |
This is intended to be squashed into 50159fe.
The problem with the original approach is not that it is completely
bogus. After all, when you pass the option --prefix=/tmp/ to Git, you do
want to end up with a trailing slash.
The real problem with the original approach is that it simply changed
path_conv, completely obvlivious and careless about other users of
path_conv.
That was really wrong, of course, and cost us time, sweat and tears. The
appropriate approach is to not affect other users, but instead
introduce a flag that we use in our caller, so that everybody gets
what they want:
emulation of inodes on FAT by calculating the hash on the normalized
path: works.
MSYS2's conversion of "POSIX" paths to Windows paths: works.