MSYS2 "fix" broke lint-staged on my MSYS2 setup

[marcelltoth]

Description

I'm not sure what issue @ext was facing in #1121 but whatever configuration I tried, my MSYS does not reproduce that.

OTOH the fix implemented in #1178 completely breaks lint-staged for me (using MSYS2) due to the extra escaping, as my terminal / shell behave normally.

Steps to reproduce

Install MSYS2, use whatever environment, install lint-staged, add some files and try running npx/yarn lint-staged. I haven't prepared a repro for lint-staged just yet, but I played around with the test script @ext shared in his issue, slightly modifying it so that it now uses ESM (which is a requirement for current execa.

You can see the gist here: https://gist.github.com/marcelltoth/f987cf03e43783a91d3cb7a0594bb6e5

I tried running this in:

• My normal environment, that is MSYS2/MINGW64 using Windows Terminal & zsh as the login shell.
• Same as first but with MSYS as the environment.
• Same as first but with bash as the shell.
• Using a Git Bash window, that is fully separate from my standard install. (MSYS, bash)

In every single case, git was indeed executed with refs/stash@{0}, no lost curly braces. I don't even see where it would get lost, as execa executes the command without a shell (good choice IMO), so why does the whole thing even has anything to do with a shell environment?

Note

Another interesting point is this comment here #1121 (comment) that suggests the person needed a similar fix, even though he was not using MSYS (otherwise putting code under the fix wouldn't have done him any favors).

So I'm assuming we have misidentified the problem and it has to do with something else (git version? Node?), not MSYS.

While we're figuring that out, can we undo the "fix"? It breaks my env that behaves completely normally, to fix someone else's that's somehow messed up.

Debug Logs

expand to view

  lint-staged:GitWorkflow Done adding task modifications to index! +245ms
  lint-staged:execGit Running git command [ 'diff', '--name-only', '-z', '--diff-filter=ACMR', '--staged' ] +123ms
[SUCCESS] Applying modifications from tasks...
[STARTED] Cleaning up temporary files...
  lint-staged:GitWorkflow Dropping backup stash... +123ms
  lint-staged:execGit Running git command [ 'stash', 'list' ] +124ms
  lint-staged:execGit Running git command [ 'stash', 'drop', '--quiet', 'refs/stash@\\{0\\}' ] +165ms
[FAILED] error: refs/stash@\{0\} is not a valid reference

  ✖ lint-staged failed due to a git error.
  Any lost modifications can be restored from a git stash:

    > git stash list
    stash@{0}: automatic lint-staged backup
    > git stash apply --index stash@{0}

Environment

• OS: Windows 11
• Node.js: 16.11.1

[marcelltoth]

I spent some time, downgraded every piece of software @ext was using in #1121 and still couldn't reproduce the issue:

I'm assuming it was an MSYS bug that has been fixed since?

[ext]

Interesting, I've have been using the same setup (but updated to the latest version) since then and it still works for me.

I could try to install a new fresh installation of msys2 and give your updated example a go.

I agree with the conclusion that it might be something else that causes the need for needing the extra escaping.

[ext]

I just tried to comment the workaround in node_modules/lint-staged/lib/gitWorkflow.js and the issue came back for me.

I'm using the MSYS2 provided git (currently 2.39.2) of that could make a difference.

[marcelltoth]

I'm using the MSYS2 provided git (currently 2.39.2) of that could make a difference.

That could be a clue, I'm using everything from outside (I have the Windows PATH appended to my MSYS path). I figured those play better with my sources also in the "Windows side". Also better perf in general.

Before reinstalling, you could check what happens if you use the external git instead.

[ext]

I can confirm it is working when I run git for windows (by prepending it to PATH)

working (git for windows):

$ git --version
git version 2.19.0.windows.1

$ which git
/c/Program Files/Git/bin/git

$ npm exec lint-staged
√ Preparing lint-staged...
√ Hiding unstaged changes to partially staged files...
√ Running tasks for staged files...
√ Applying modifications from tasks...
√ Restoring unstaged changes to partially staged files...
√ Cleaning up temporary files...

non-working (git installed via msys2 pacman)

$ git --version
git version 2.39.2

$ which git
/usr/bin/git

$ npm exec lint-staged
√ Preparing lint-staged...
√ Hiding unstaged changes to partially staged files...
√ Running tasks for staged files...
√ Applying modifications from tasks...
√ Restoring unstaged changes to partially staged files...
× error: refs/stash@0 is not a valid reference

  × lint-staged failed due to a git error.
  Any lost modifications can be restored from a git stash:

    > git stash list
    stash@{0}: automatic lint-staged backup
    > git stash apply --index stash@{0}

Both of these tests are running with the workaround from #1121 commented (by editing node_modules/lint-staged/lib/gitWorkflow.js directly).

[marcelltoth]

Nice find, I could repro the same!

Investigation

I actually boiled it down to this:

const execaLib = require("execa");
const {spawnSync} = require('child_process');

spawnSync("git", ['--no-pager', 'show', 'refs/stash@{0}'], {stdio: 'inherit', stderr: 'inherit', windowsVerbatimArguments: true})
spawnSync("git", ['--no-pager', 'show', 'refs/stash@{0}'], {stdio: 'inherit', stderr: 'inherit', windowsVerbatimArguments: false})
spawnSync("git", ['--no-pager', 'show', '"refs/stash@{0}"'], {stdio: 'inherit', stderr: 'inherit', windowsVerbatimArguments: true})
spawnSync("git", ['--no-pager', 'show', '"refs/stash@{0}"'], {stdio: 'inherit', stderr: 'inherit', windowsVerbatimArguments: false})

With Windows git:

No quoting - verbatim - GOOD
No quoting - no verbatim - GOOD
Quoting - verbatim - GOOD
Quoting - no verbatim - FAIL

With UNIX git:

No quoting - verbatim - FAIL
No quoting - no verbatim - FAIL
Quoting - verbatim - GOOD
Quoting - no verbatim - FAIL

So with quoting and windowsVerbatimArguments: true both work fine, the rest fails in the unix case. We'd of course have to test on a real UNIX if we want to make this change, I hope it'll work there, too.

Idea

This execution / escaping is a mess. It'd be nice to figure this out for good, but I also had another idea, that is likely a much less worrying change: Why don't we simply use a number to refer to the stash, you can do that in git 2.11+ (mid-2016).

See this StackOverflow thread essentially asking about the same question we're running into: https://stackoverflow.com/q/17454235/10614791

A simple return String(index) should solve all the cases, without introducing and dangerous changes regarding command execution.

What do you think?

cc @ext @iiroj

[iiroj]

@marcelltoth I don't think we've previously established version requirements for git itself, but I think your suggestion would be acceptable. 👍

[marcelltoth]

Fixed in #1270