Improve windows shell handling

[joelim-work]

Changes:

• Modify the shellCommand function in os_windows.go to handle quoted arguments with spaces. I got the idea from os/exec: Cannot execute command with space in the name on Windows, when there are parameters golang/go#17149 to set the command line directly in cmd.SysProcAttr.
• Add a quoteString function which quotes strings depending if the OS is Windows or not. This now means that displaying the documentation via <f-1> or :doc now works even if the lf path contains spaces (see Make the :doc command work even if lf is not in the PATH, add lf environment variable #1176).
• Refactor the exportFiles function so that all the main logic is done inside nav.go, and calls out to the above quoteString function to handle quoting.

---

The following commands now work, at least on my machine:

!dir "C:\Program Files" | %PAGER%

&lf -remote "send %id% toggle"

Executables with spaces in their path now also work (from #1276):

!"C:\Program Files\Go\bin\go.exe" version
!"C:\Program Files\Git\bin\git.exe" --version

mkdir with a quoted string now properly creates the directory in the current location instead of at C:\ (from #1309 (comment)):

$mkdir "foo bar"

---

I have also done some very brief testing with PowerShell, but I'm not so familiar with it so I would appreciate it if others helped to test it as well and make sure the changes are more stable. Hopefully we can avoid the situation where separate command-handling logic is required for CMD vs. PowerShell.

[ilyagr]

Thank you so much! I had a bit of experience with quoting on Windows with #1176, and it was maddening.

cc #1276 and @kietfsek: perhaps you could help test this out?

I haven't tried this out yet; I'd have to remember how to spin up a Windows VM to do it. Still, here are some thoughts from memory:

• this is likely fine, but please check that the :doc command still works. (it's probably OK since `%PAGER% still works). If not, you might need to revert the Windows part of Make the :doc command work even if lf is not in the PATH, add lf environment variable #1176 (unless we come up with something better).
• OTOH, perhaps this makes :doc better? Make the :doc command work even if lf is not in the PATH, add lf environment variable #1176 has a bug that if the path to lf contains a space, :doc won't work. It's possible something like this PR could fix that bug.

[veltza]

Sorry to intrude here. Does this fix a quoting issue that causes unexpected behavior on Windows?

For example, if you run a following command:

$mkdir "newdir"

It actually creates the new directory in root of the C drive and not the current working directory.

Whereas without quotes it properly creates a directory in the current working directory:

$mkdir newdir

This happens when I run lf with the default configuration in Powershell.

If the problem still persists, I can create a new issue about it.

[kietfsek]

@kietfsek: perhaps you could help test this out?

@ilyagr: I am willing to help. However, although I am OK on technical stuff (figuring out how to download a branch and set up the build environment -- I can work them out on my own), I am not familiar with GitHub interface and I have very limited experience of contributing in any form to a project on GitHub. I am willing to learn in order to help the testing, but I will need guidance.

[ilyagr]

I am willing to learn in order to help the testing, but I will need guidance.

@kietfsek Here's some guidance. Let me know if you still have questions; I'm not sure exactly what you need help with.

Checking out the PR

There are a few different ways.

A. You could use the Github Desktop tool, especially on Windows.

I haven't used it for a long time, but I remember it to be pretty straightforward.

B. You could clone Joe's repo from https://github.com/joelim-work/lf/ and check out the windows-shell-handling branch.

C. Or you could use my favorite way, from https://gist.github.com/gnarf/5406589:

1. Add the following to your .gitconfig:

[alias]
pr  = "!f() { git fetch -fu ${2:-$(git remote |grep ^upstream || echo origin)} refs/pull/$1/head:pr/$1 && git checkout pr/$1; }; f"

2. Clone this repo (https://github.com/gokcehan/lf/).
3. Run git pr 1309. I think that should work even on Windows using the shell from Git on Windows. It should create a local pr/1309 branch in your repo and switch to it.

Build lf

You'll need to install Go, see https://go.dev/doc/install. For compiling lf after that, there are instructions in the README.

[kietfsek]

@ilyagr

I think that's good and enough information. Thanks. I will start working on it later when I have time.

[joelim-work]

Sorry to intrude here. Does this fix a quoting issue that causes unexpected behavior on Windows?

For example, if you run a following command:

$mkdir "newdir"

It actually creates the new directory in root of the C drive and not the current working directory.

Whereas without quotes it properly creates a directory in the current working directory:

$mkdir newdir

This happens when I run lf with the default configuration in Powershell.

If the problem still persists, I can create a new issue about it.

@veltza I have tested $mkdir "foo bar" and it now creates the directory properly in the current location, and not in C:\. This is for the default CMD shell.

For PowerShell, I had to use single quotes (i.e. $mkdir 'foo bar' or New-Item -ItemType 'directory' -Name 'foo bar'). I don't have much experience with PowerShell, so I'm not sure how big of a problem it is - we could check whether the shell is CMD or PowerShell and handle the command differently, but I prefer not to do this if possible.

[joelim-work]

Thank you so much! I had a bit of experience with quoting on Windows with #1176, and it was maddening.

cc #1276 and @kietfsek: perhaps you could help test this out?

I haven't tried this out yet; I'd have to remember how to spin up a Windows VM to do it. Still, here are some thoughts from memory:

• this is likely fine, but please check that the :doc command still works. (it's probably OK since %PAGER% still works). If not, you might need to revert the Windows part of [Make the :doccommand work even iflfis not in the PATH, addlf` environment variable Make the :doc command work even if lf is not in the PATH, add lf environment variable #1176](Make the :doc command work even if lf is not in the PATH, add lf environment variable #1176) (unless we come up with something better).
• OTOH, perhaps this makes :doc better? Make the :doc command work even if lf is not in the PATH, add lf environment variable #1176 has a bug that if the path to lf contains a space, :doc won't work. It's possible something like this PR could fix that bug.

@ilyagr I got this to work by quoting the lf environment variable similar to how all the other file variables are exported, see 03f1c3b.

[kietfsek]

@ilyagr

I tried the PR (latest commit being 5ff3265), and the result:

• :doc works
• if lf.exe is not in PATH, :doc still doesn't work
• map X !"c:\Program Files\WinMerge\WinMergeU.exe" d:\test.txt works which didn't work before the PR

[joelim-work]

That's strange, I found :doc still worked even if lf.exe wasn't in PATH. Maybe try seeing what the value of the lf environment variable is using !echo %lf%, and try printing the documentation with !%lf% -doc.

[joelim-work]

So after a bit more testing with PowerShell, I do have some slight bad news. The quoting for cmd.SysProcAttr is treated differently depending on whether the shell is CMD or PowerShell, so we will have to add an if statement to handle this and use the original code for PowerShell. I don't really see any other good solution - I have sort of resigned myself to the fact that Windows is just a difficult beast to deal with.

EDIT: Fixed in 3e89fde

[kietfsek]

Update:

I realized what went wrong when testing the case that lf.exe is not in PATH, the way I tested it was incorrect -- I didn't know cmd.exe's path needs to be in PATH. Previously I set PATH to empty string to do the test, and the error I saw was caused by not able to launch cmd.exe. Sorry for the confusion.

After I ruled this out, I did see :doc worked for the case lf.exe not in PATH.

So:

• :doc works
• if lf.exe is not in PATH, :doc works
• map X !"c:\Program Files\WinMerge\WinMergeU.exe" d:\test.txt (cc Tip: make a Windows command that has space character in path and takes argument work #1276) works

(tested on latest commit, 130c63c)

[gokcehan]

Thank you very much @joelim-work for the patch and @ilyagr @veltza @kietfsek for the reviews and testings. This patch will certainly be much appreciated by our Windows users.