170 - Windows Slash Tests

[yhakbar]

This should confirm that windows builds handle slashes appropriately.

[yhakbar]

Blocked by #206

[yhakbar]

Closes #170

[CosmicHorrorDev]

I think this problem is only reproducible when using things like git-bash on Windows. There's some more info on it here BurntSushi/ripgrep#1973 (comment)

I'm not sure how, but other programs like sed and grep don't have the same issue

[yhakbar]

I don't have a Windows machine, so I can't reproduce locally. Is the objective to discover whatever Windows git bash is doing to forward slashes in arguments and/or stdin and introduce a flag to undo that behavior?

I'm curious as to what git bash is doing, and I have no idea why sed or grep wouldn't have their arguments and/or stdin mangled the same way.

[CosmicHorrorDev]

I've got access to a Windows machine, so I'll start digging in deeper to try and get things sorted out more

[yhakbar]

Hey @CosmicHorrorDev , did you get a chance to reproduce in Windows? IMO if this isn't easily reproducible, it might not be worth holding up the release, as it would be addressing an issue before stdin even makes it to the binary.

I selfishly want the release to happen soon so I can stop using my fork to distribute ARM builds, but understand if you want to take your time resolving these issues.

[CosmicHorrorDev]

Haven't had time to setup a Windows env for reproducing

I agree this issue shouldn't hold up the next release. I'll be working on the final touches for preparing the release over this upcoming weekend

[nc7s]

Got to use my Windows machine to test this, both gnu and msvc targets are tested, on both powershell and git-bash. Strangely, this specific test didn't go wrong in either target, but cli::in_place_following_symlink failed.

Update: that test failed to create a soft link because of "Error: A required privilege is not held by the client. (os error 1314)", even though the tempdir it created is perfectly under my user's own place.

[nc7s]

I wrote a simple test program:

#include <stdio.h>

int main(int argc, char** argv) {
        for(int i = 0; i < argc; i++) {
                printf("%d\t%s\n", i, argv[i]);
        }
        return 0;
}

Compiled it with cl.exe and linked with link.exe, from Visual Studio.

Ran it under powershell and git-bash. Voilà:

PS C:\Users\wo\Works\sd> .\test.exe /
0       C:\Users\wo\Works\sd\test.exe
1       /
PS C:\Users\wo\Works\sd> .\test.exe \
0       C:\Users\wo\Works\sd\test.exe
1       \
PS C:\Users\wo\Works\sd> bash

wo@ MINGW64 ~/Works/sd (170/windows-slash-tests)
$ ./test.exe /
0       C:\Users\wo\Works\sd\test.exe
1       C:/Users/wo/scoop/apps/git/2.42.0.2/

wo@ MINGW64 ~/Works/sd (170/windows-slash-tests)
$ ./test.exe //
0       C:\Users\wo\Works\sd\test.exe
1       /

wo@ MINGW64 ~/Works/sd (170/windows-slash-tests)
$ ./test.exe ///
0       C:\Users\wo\Works\sd\test.exe
1       //

wo@ MINGW64 ~/Works/sd (170/windows-slash-tests)
$ ./test.exe \\
0       C:\Users\wo\Works\sd\test.exe
1       \

wo@ MINGW64 ~/Works/sd (170/windows-slash-tests)
$ ./test.exe \\\\
0       C:\Users\wo\Works\sd\test.exe
1       \\

IDK if it's git-bash or mingw64, but it apparently lies in the translation layer of one of them.

[nc7s]

It's mingw64 - or msys2, since this is documented by the latter, not the former - that does this "automagic path translation":

When calling native executables from the context of Cygwin then all the arguments that look like Unix paths will get auto converted to Windows.

Our case is, of course, covered.

[yhakbar]

It's mingw64 - or msys2, since this is documented by the latter, not the former - that does this "automagic path translation":

When calling native executables from the context of Cygwin then all the arguments that look like Unix paths will get auto converted to Windows.

Our case is, of course, covered.

@CosmicHorrorDev would adding a recommendation that the environment variable MSYS2_ENV_CONV_EXCL be populated with * to prevent the "automagic path translation" be sufficient to close #170 ?

[nc7s]

It's mingw64 - or msys2, since this is documented by the latter, not the former - that does this "automagic path translation":

When calling native executables from the context of Cygwin then all the arguments that look like Unix paths will get auto converted to Windows.

@CosmicHorrorDev would adding a recommendation that the environment variable MSYS2_ENV_CONV_EXCL be populated with * to prevent the "automagic path translation" be sufficient to close #170 ?

(It's MSYS2_ARG_CONV_EXCL, for ARGuments ;)

This works, but doesn't look nice. You certainly don't want to prefix with it every time running sd or something else. Do we advice the user to alias it? Globally export? Or we ship a wrapper that adds it?

[CosmicHorrorDev]

(I'll be able to get to this next week)

[CosmicHorrorDev]

Finally got a dev environment setup on windows

Strangely, this specific test didn't go wrong in either target, but cli::in_place_following_symlink failed.

Update: that test failed to create a soft link because of "Error: A required privilege is not held by the client. (os error 1314)", even though the tempdir it created is perfectly under my user's own place.

I can also reproduce this issue. Not sure what the underlying issue is, but I didn't do any digging

This works, but doesn't look nice. You certainly don't want to prefix with it every time running sd or something else. Do we advice the user to alias it? Globally export? Or we ship a wrapper that adds it?

My current plan is to allow for specifying custom path separators through a --path-separator <SEPARATOR> flag, and then automatically setting the path separator to / in MSYS akin to what's done in sharkdp/fd#730

I'm currently working on adding support for custom path separators by looking through how ripgrep and fd handle things

[nc7s]

My current plan is to allow for specifying custom path separators through a --path-separator <SEPARATOR> flag, and then automatically setting the path separator to / in MSYS akin to what's done in sharkdp/fd#730

I'm currently working on adding support for custom path separators by looking through how ripgrep and fd handle things

ripgrep and fd have file paths in their output, so that option makes sense. AFAICT sd doesn't, even with --preview (it just prints the replaced content, with no file marking to distinguish multiple files). If it prints diffs, file names can be naturally shown (this could be its own PR), then it's more reasonable to add it.

Other than that, I also don't think it's a good idea to use an output modifier to change "input" behavior, but it's just a gut feeling.

Update: --preview does print file names, which I overlooked. Meanwhile I'm getting somewhere with diffs - PR for later.

[yhakbar]

@CosmicHorrorDev , would providing the ability to adjust the path-separator really be the best solution here?

The issue in the example referenced in #170 isn't that they were using a Windows path separator of \ and sd was mangling the stdin, using / instead, but that msys2 was "automagically" swapping out \ for / before it made its way to the stdin for sd, right? I think the example was an HTML tag being mangled by msys2, as it erroneously swapped out the slashes in the tag, thinking they were path separators.

I think the Path type is supposed to handle discrepancies in path separators natively if it gets the appropriate stdin for the operating system being used. Recommending that the user configure msys2 to not mangle the input to sd through the appropriate environment variable seems to me to be the most direct way to solve the issue.

[CosmicHorrorDev]

Recommending that the user configure msys2 to not mangle the input to sd through the appropriate environment variable seems to me to be the most direct way to solve the issue.

I was mistaken about the utility behind a custom path separator. I agree with you 👍

[chmln]

Appreciate the discussion here and definitely agree as well with recommending the env var approach.

I have always believed in adding config options as an absolute last resort, which might seem a bit unusual but leads to better experience for both the users and the developers in the long term