Regression from pathconv changes

[naveen521kk]

Previously msys2-runtime converted the path separator from : to ; when passing to a Windows program. Now, for some reason, this doesn't work:

$ A=.:../test python -c 'import os; print(os.environ["A"])'
.:../test

Though if it's the PATH variable it seems to work.

$ PATH=".:../test" /mingw64/bin/python -c 'import os; print(os.environ["PATH"])'
.;C:\Users\test

Output from an old version of runtime for reference:

$ A=.:../test python -c 'import os; print(os.environ["A"])'
.;..\test

This is probably why rebuilding texlive errors out, see msys2/MINGW-packages#19921 (comment).

[Voxeles]

Seems like this is caused by 9944652
Any path with :. or :.. in it is ignored.

For example, this works:

$ A="/this:/there" python -c 'import os; print(os.environ["A"])'
C:\msys64\this;C:\msys64\there
$ A="./this:/there" python -c 'import os; print(os.environ["A"])'
.\this;C:\msys64\there

But this doesn't:

A="/this:./there" python -c 'import os; print(os.environ["A"])'
/this:./there
A="./this:./there" python -c 'import os; print(os.environ["A"])'
./this:./there

[reneparis]

@dscho It seems as your commit 9944652 broke the pathconv rules as described here. Unfortunately, the rules are not complete w.r.t. relative paths. The official documentation says:

$ python3 -c "import sys; print(sys.argv)" --dir=/foo
['-c', '--dir=C:/msys64/foo']
$ python3 -c "import sys; print(sys.argv)" --dir=/foo:/bla
['-c', '--dir=C:\\msys64\\foo;C:\\msys64\\bla']

Relative paths have not been mentioned so far, but the expected behavior should be something like this:

$ python3 -c "import sys; print(sys.argv)" --dir=/foo:./bla
['-c', '--dir=C:\\msys64\\foo;.\\bla']

With the current pathconv rules, we get a different - flawed - picture

$ python3 -c "import sys; print(sys.argv)" --dir=/foo:./bla
['-c', '--dir=/foo:./bla']

Note, that we get something completely different, if we do the following:

$ python3 -c "import sys; print(sys.argv)" --dir=./foo:/bla
['-c', '--dir=.\\foo;C:\\msys64\\bla']

The change seems to be incomplete and hence break some packages, such as mentioned here msys2/MINGW-packages#19921

Any thought how we could fix this again?

[ReinhardBiegelIntech]

Just wanted to add that MSYS2 already provides a mechanism to exclude specific command line arguments (and environment variables) from the path conversion, as is also described in the MSYS2 documentation under the same link that @reneparis posted (see MSYS2_ARG_CONV_EXCLand MSYS2_ENV_CONV_EXCL). Maybe using this mechanism would be a more consistent approach than trying to intercept the conversion in msys2_path_conv.cc (9944652), where no contextual information about the strings to be converted is available.

[dscho]

Any thought how we could fix this again?

Not completely.

As stated in the commit message of 9944652, the idea is to not confuse Git's <rev>:./<filename> for a path list (and incidentally, it would also help things like scp some.host:./file ./

Now, in your examples, the path lists all start with either / or ./. Would this always be the case? If yes, I think we can reliably discern between the cases I do not want to be converted and the cases you want to be converted.

[dscho]

Now, in your examples, the path lists all start with either / or ./. Would this always be the case? If yes, I think we can reliably discern between the cases I do not want to be converted and the cases you want to be converted.

The way I'd do that is by initializing something like bool is_potential_path_list = *it == '/' || (*it == '.' && *it == '/'); before that while() loop, and then prefix the if() condition testing for Git's <rev>:./name syntax with !is_potential_path_list.

@reneparis wanna give it a whirl? It might be a couple of days until I get back to this...

[reneparis]

Sorry, but this does not work, 'cause the example I gave was arbitrary. The broken package uses the following variable
WEBINPUTS=.:../../../texk/web2c (see msys2/MINGW-packages#19921 (comment)). I've looked into the build scripts, but it's not easy to change the assignment. On top, this would only hotfix a broken behavior.

The conversion skipping mechanism is not specific enough and in my opinion introduces a lot of unexpected behavior. The two requirements, git path resolution and arbitrary path conversion, do not go well together unless the converter knows that it is in the context of git or scp. Just operating on the characters is not enough. Can we leverage this somewhere?

[dscho]

The broken package uses the following variable
WEBINPUTS=.:../../../texk/web2c (see msys2/MINGW-packages#19921 (comment)).

Well, the prefix .: would be easy enough to catch: it is neither a valid scp target nor a <rev>:<path> notation.

[dscho]

The two requirements, git path resolution and arbitrary path conversion, do not go well together unless the converter knows that it is in the context of git or scp. Just operating on the characters is not enough. Can we leverage this somewhere?

At the moment, I don't think that the path conversion is privy of the information that it operates on a given command, in particular because it also operates on environment variables. It is an interesting idea, though.

[ReinhardBiegelIntech]

@dscho I'd be interested in the original intention of the change in 9944652. Was it to satisfy the test suite? Or are there real world examples (e.g. of broken packages in MSYS2)?

Also, the commit message states

... but path lists are expected to contain only absolute paths ...

which is directly related to our problem here. What is this statement based on? I find it to be quite common to use relative paths in path lists, especially in (C/C++) build systems.

At the moment, I don't think that the path conversion is privy of the information that it operates on a given command...

The exact point where this information is already available is when calling the command. Thus, as stated previously, I'd give MSYS2_ARG_CONV_EXCL a try. Did you look into that?

Maybe we can work out another solution for that together? In my opinion the current situation is not acceptable as the change breaks more things than it fixes and thus should be reverted (if we cannot come up with a better solution rather soon).

btw, even though this is IMO the wrong approach (as is just catches a special case): the code snippet from above has a bug in the expression and should maybe be: bool is_potential_path_list = *it == '/' || (*it == '.' && *(it+1) == '/'); (note the +1). But it would still be missing ../foo, or just let's assume /foo/HEAD:../baar for example.

[reneparis]

@lazka - what is your opinion on that?

[dscho]

I'd be interested in the original intention of the change in 9944652. Was it to satisfy the test suite?

While it does satisfy Git's test suite, the underlying reason for that fix is that it does address a class of real-world problems, as users such as myself use Git to inspect <rev>:<path> contents quite often, and similarly clone via SSH from <host>:<path>, where <path> may very well be a relative path (which is relative the current directory in the former case, and relative to the home directory on the SSH host in the latter).

Also, the commit message states

... but path lists are expected to contain only absolute paths ...

which is directly related to our problem here. What is this statement based on?

Back when the change was originally made (in 2015), the package builds I sampled all used absolute paths in order to avoid problems when, say, setting the PERLLIB in the project root and then using it in a subdirectory. Lots of things have changed since then, but all the packages I regularly build for Git for Windows (see https://github.com/git-for-windows/git-for-windows-automation/actions/workflows/build-and-deploy.yml for many of them) follow that same pattern.

At the moment, I don't think that the path conversion is privy of the information that it operates on a given command...

The exact point where this information is already available is when calling the command. Thus, as stated previously, I'd give MSYS2_ARG_CONV_EXCL a try. Did you look into that?

Oh, let me clarify a misunderstanding: I addressed the problem mentioned in the referenced commit not for me, but for all of Git for Windows' users. Judging by the download count (which I have to use in lieu of more reliable data that I however cannot collect because that would cause High Drama In The Open Source World), the number of those users is in the millions.

So: no, I cannot simply use MSYS2_ARG_CONV_EXCL (especially because it does not scale well at all).

Maybe we can work out another solution for that together? In my opinion the current situation is not acceptable as the change breaks more things than it fixes and thus should be reverted (if we cannot come up with a better solution rather soon).

Well, in the spirited words of The Dude:

😁

Don't get me wrong (and especially please understand that I say this in good humor, not to offend you or something): I understand where you are coming from, I also understand, though, that we're dealing with an ill-posed problem here. Basically only a human can tell with certainty whether a given command-line option or environment variable is supposed to contain a path list that needs to be translated from Unix-style to Windows-style, or whether it contains something different that distinctly does not refer to a Windows path. And to be truthful: I would expect most humans to fail on that task, too, if provided sufficiently hairy corner cases (think of a script that is called with an argument that eventually is appended to PERLLIB. In a heredoc that is passed to wsl sh. How many users do you know who can reliably reason about that?).

btw, even though this is IMO the wrong approach

Again, please appreciate that at this moment, this is just an opinion, and I cannot really tell how much experience you really have in the space. As I said, I am building MSYS2 packages on a regular basis since 2015, so I would contend that I have a little bit of experience here that should count for something.

(as is just catches a special case):

Hah! The entire path conversion is a special case. It is a gift that keeps on giving!

the code snippet from above has a bug in the expression and should maybe be: bool is_potential_path_list = *it == '/' || (*it == '.' && *(it+1) == '/'); (note the +1).

I concur. And I never claimed that it was correct. I said "something like", and was hoping that someone would be enticed to give it a whirl and open a PR so that we can do more than just talk. Obviously that plan failed rather miserably, joining so many of my past schemes 😉

But it would still be missing ../foo, or just let's assume /foo/HEAD:../baar for example.

It would be missing the former, although that is not even a path list and would therefore not run into the code introduced by the commit under discussion.

It would not be missing the latter, though, as we're literally looking at a slash at the beginning of the path list, not after any colon.

In the short run, I still think that we can probably figure out reliable patterns to catch most of the common scenarios.

We will never catch all of the scenarios, of course, which is the reason why we need "escape hatches". In scenarios where the automatic path conversion is overzealous, those escape hatches would be:

• MSYS_NO_PATHCONV as a big hammer
• MSYS2_ARG_CONV_EXCL and MSYS2_ENV_CONV_EXCL

In scenarios where the automatic path conversion fails to kick in, but would be desirable:

• Use "$(cygpath -wp "$arg")" instead of "$arg" for path lists
• Use "$(cygpath -w "$arg")" for paths.

I don't think that we will ever be able to completely prevent the need for those escape hatches.

Of course, I could imagine that eventually we'd introduce support for configuring regular expressions via environment variables to override the MSYS2 runtime's default classification. But then...: