fix(#2467): remove newline in git paths #2478

mj2068 · 2023-10-17T15:13:00Z

Fixes #2467 (potentially also for #2466), regarding failed git integration on Windows.
Background: In file git/utils.lua, the two paths variables toplevel and git_dir are returned by vim.fn.system(...), which will add a trailing newline character, causing them to be invalid paths.
Fix: Added two lines of gsub function to remove any newline characters in these paths.

After preliminary tests on Ubuntu, I suggest caution for the general behavior of vim.fn.system() on all systems, where its return value seems to always contain a trailing newline, therefore its direct return value can't be used as paths.

alex-courtis

Looks good to me, I'm just not certain how this works for PowerShell

alex-courtis · 2023-10-21T03:23:37Z

lua/nvim-tree/git/utils.lua

@@ -40,10 +40,14 @@ function M.get_toplevel(cwd)
      if vim.v.shell_error ~= 0 then
        return nil, nil
      end
+      -- remove trailing newline(\n) character added by vim.fn.system
+      toplevel = toplevel:gsub("\n", "")


This is inside the cygpath block.

I was under the impression that you are using PowerShell. Does it see cygpath?

Apologize for didn't fully describe my environment before.
Yes, I use PowerShell, but I also have msys2 installed on my Windows 10.
Yes, PowerShell sees "cygpath", ~~because the POSIX compatible environment provided msys2 is based on Cygwin. So Cygwin tools like "cygpath"(exe files you can run directly) are in the Windows path.~~

Update:
I have to admit, in my case, the reason PowerShell sees "cygpath" is because it's inside X:/msys2/usr/bin, and this bin path is in the Windows path environment variable.
But I actually forgot whether it's added by the official msys2 installer or manually by me.

And, the situation of this issue has changed a bit due to the new commit, which made using "cygpath" a manual option with a default value: false. This bypassed the two vim.fn.system reassigning statements which were the causes of my original issue(#2467), leaving the paths conversion to these two line:

nvim-tree.lua/lua/nvim-tree/git/utils.lua

Lines 49 to 50 in db8145c

toplevel = toplevel:gsub("/", "\\")

git_dir = git_dir:gsub("/", "\\")

I thinking making "cygpath" a manual option is a good call, and will test it and report back.
Thx, alex. Peace.

PowerShell sees "cygpath" is because it's inside X:/msys2/usr/bin, and this bin path is in the Windows path environment variable.

That explains a lot - a valid use case that we never expected or handled.

I'm prepared for disappointment after your testing, however we will fix this one way or another.

Big favour, if possible: please run has.lua for neovim inside msys2. It would be great to fill out the entirety of OS Feature Flags

Had to do some reading about how msys2 works. Then I did the following inside the msys2 official shell, with nvim 0.9.4 installed from MSYS2 Packages using pacman, following their doc.

Flags: has_list_msys2_mingw.log

I then did quick git integration tests regarding the new cygwin_support manual option, false and true, using the latest commit.

With the "Clean Room" lua file, test folder "project1" git initialized, which contains one untracked file "README.md".
After "Ready!" message, :NvimTreeOpen, select "project1", hit Enter, the results are:

cygwin_support = false(default):
shown error message: [NvimTree] Could not start the fs_event watcher for path \tmp\project1\.git : ENOENT
log: nvim-tree_cygwin_false.log

cygwin_support = true:
no error, but no git tracking.
log: nvim-tree_cygwin_true.log

I think we need to normalize the paths.

Thx, alex.

Attachs the two screenshots, false and true respectively:

Thank you for all your diligent testing with this unfamiliar environment. This is quite the difficult issue...

It looks like the cygwin_support = true case is not matching any toplevel and returning nils. It's definitely hitting the use_cygpath branch as the win32 feature flag is set and we are seeing different results.

To confirm:

does this PR fix the above?

do we need to do more right now or can we merge this?

does this PR fix the above?

It did. Now, I'm not so sure, because the new "cypwin_support" option affected it.

do we need to do more right now or can we merge this?

I don't think we can merge this. But the issue is getting clearer to me. I'm working on a fix, can I have few days?

- Now there is a whitespace between value and unit. - Now values >= 1024 YiB are shown in YiB instead of B. - To reuse same code a new local function was added: round().

…ee#2489) * feat(nvim-tree#2312): fire `TextYankPost` event on path copy * stylua * Bug fix --------- Co-authored-by: Alexander Courtis <alex@courtis.org>

) * feat: add option to sort entries in help window * stylua * Add keymap to toggle sorting methods * Bug fix --------- Co-authored-by: Alexander Courtis <alex@courtis.org>

alex-courtis · 2023-10-22T22:02:20Z

Can I ask a big favour:

#2491 is a guess to attempt some path normalisation however may be incorrect or destructive.

Could you please attempt to incorporate the above into this PR?
It would be best to have all the pieces tested together in known environment(s). If #2491 is not useful please disregard it.

… bugfix

mj2068 · 2023-10-23T09:34:26Z

It's okay, alex, glad to help.

As to #2491, I don't think "\r\n" is the source of the issue.

vim.fn.system will always convert "\r\n" to "\n", according to:
:help system()

  Result is a String, filtered to avoid platform-specific quirks:
  - <CR><NL> is replaced with <NL>
  - NUL characters are replaced with SOH (0x01)

I confirmed this vim.fn.system behavior in my previous tests.

Anyway, will be back. Peace.

mj2068 · 2023-10-23T10:02:48Z

And @alex-courtis, I'm not very familiar with git PR workflow, I did some git things in my fork, like, fetch upstream, merge, rebase, etc., according to some tutorials. Now, this PR page shows me, it has changed a lot files. I hope I don't break anything, did I do something wrong?

alex-courtis · 2023-10-23T22:19:20Z

This sounds fantastic, don't worry about CI etc. We can get this right then we tidy.

I'll have a chance to look / review on the weekend.

To avoid shell compatibility issues in msys2 environment on Windows

mj2068 · 2023-10-25T14:13:38Z

Finally.
The compatibility issues are really complicated.

But I think this fixed it.
I tested git integration on Windows and Ubuntu.

Windows:
- PowerShell cygwin_support = false, OK✔.
- PowerShell cygwin_support = true, OK✔.
- MSYS2 MinGW shell cygwin_support = true, OK✔.
- MSYS2 MinGW shell cygwin_support = false, NOT❌. Without cygpath, paths are incompatible.
Ubuntu, OK✔. Not "win32", not affected.

Take a look.

alex-courtis · 2023-10-30T00:32:31Z

This PR is seeing up phantom changes; this branch is definitely up to date: master...mj2068:nvim-tree.lua:bugfix

alex-courtis

This is just fantastic, thank you for the great consideration and testing!

Closed #2491

fix(nvim-tree#2467): remove newline in git paths

4ed344a

alex-courtis approved these changes Oct 21, 2023

View reviewed changes

alex-courtis and others added 4 commits October 23, 2023 08:33

Merge branch 'master' into bugfix

671f194

fix: info size suffix and formatting (nvim-tree#2492)

a31dfad

- Now there is a whitespace between value and unit. - Now values >= 1024 YiB are shown in YiB instead of B. - To reuse same code a new local function was added: round().

feat(nvim-tree#2312): fire TextYankPost event on path copy (nvim-tr…

c2194e9

…ee#2489) * feat(nvim-tree#2312): fire `TextYankPost` event on path copy * stylua * Bug fix --------- Co-authored-by: Alexander Courtis <alex@courtis.org>

feat: mapping and options to sort entries in help window (nvim-tree#2482

78a9ca5

) * feat: add option to sort entries in help window * stylua * Add keymap to toggle sorting methods * Bug fix --------- Co-authored-by: Alexander Courtis <alex@courtis.org>

mj2068 added 2 commits October 23, 2023 15:10

fix(nvim-tree#2467): remove newline in git paths

49f5103

Merge branch 'bugfix' of https://github.com/mj2068/nvim-tree.lua into…

c8d0515

… bugfix

fix(nvim-tree#2467): change cygpath calls to array format

d9e03a5

To avoid shell compatibility issues in msys2 environment on Windows

stylua nit

fb7b4e0

alex-courtis approved these changes Oct 30, 2023

View reviewed changes

alex-courtis merged commit 7c5c074 into nvim-tree:master Oct 30, 2023

alex-courtis mentioned this pull request Oct 30, 2023

git integration not working in Windows. #2466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#2467): remove newline in git paths #2478

fix(#2467): remove newline in git paths #2478

mj2068 commented Oct 17, 2023 •

edited by alex-courtis

alex-courtis left a comment

alex-courtis Oct 21, 2023

mj2068 Oct 21, 2023 •

edited

alex-courtis Oct 21, 2023

mj2068 Oct 22, 2023

alex-courtis Oct 22, 2023

mj2068 Oct 23, 2023 •

edited

alex-courtis commented Oct 22, 2023

mj2068 commented Oct 23, 2023

mj2068 commented Oct 23, 2023

alex-courtis commented Oct 23, 2023

mj2068 commented Oct 25, 2023 •

edited

alex-courtis commented Oct 30, 2023

alex-courtis left a comment

	toplevel = toplevel:gsub("/", "\\")
	git_dir = git_dir:gsub("/", "\\")

fix(#2467): remove newline in git paths #2478

fix(#2467): remove newline in git paths #2478

Conversation

mj2068 commented Oct 17, 2023 • edited by alex-courtis

alex-courtis left a comment

Choose a reason for hiding this comment

alex-courtis Oct 21, 2023

Choose a reason for hiding this comment

mj2068 Oct 21, 2023 • edited

Choose a reason for hiding this comment

alex-courtis Oct 21, 2023

Choose a reason for hiding this comment

mj2068 Oct 22, 2023

Choose a reason for hiding this comment

alex-courtis Oct 22, 2023

Choose a reason for hiding this comment

mj2068 Oct 23, 2023 • edited

Choose a reason for hiding this comment

alex-courtis commented Oct 22, 2023

mj2068 commented Oct 23, 2023

mj2068 commented Oct 23, 2023

alex-courtis commented Oct 23, 2023

mj2068 commented Oct 25, 2023 • edited

alex-courtis commented Oct 30, 2023

alex-courtis left a comment

Choose a reason for hiding this comment

mj2068 commented Oct 17, 2023 •

edited by alex-courtis

mj2068 Oct 21, 2023 •

edited

mj2068 Oct 23, 2023 •

edited

mj2068 commented Oct 25, 2023 •

edited