Fixes issue #1344 - windows folder casing issue

[dangibson]

Description of Changes

Closes # 1344

---

New Version Info

Author's Instructions

• Derive a new MAJOR.MINOR.PATCH version number. Increment the:
  • MAJOR version when you make incompatible API changes
  • MINOR version when you add functionality in a backwards-compatible manner
  • PATCH version when you make backwards-compatible bug fixes
• Update CHANGELOG.md, following the established pattern.

Collaborator's Instructions

• Review CHANGELOG.md, suggesting a different version number if necessary.
• After merging, tag the commit using these (Mac-compatible) bash commands:

  git checkout master
  git pull
  sed -n "$(grep -n -m2 '####' CHANGELOG.md | cut -f1 -d: | sed 'N;s/\n/,/')p" CHANGELOG.md | sed '$d'
  git tag -a $(read -p "Tag Name: " tag;echo $tag) -m"$(git show --quiet --pretty=%s)";git push origin --tags

[rzvxa]

shouldn't we implement similar logic for renaming, s:TreeDirNode._glob and other parts comparing paths?
It's already been done to some extent but there are so many problems with path casing on windows.
We do something similar for s:Path.equals like this:

function! s:Path.equals(path)
    if nerdtree#runningWindows()
        return self.str() ==? a:path.str()
    else
        return self.str() ==# a:path.str()
    endif
endfunction

in vim ==? means ignoring the case so there is no need for lower function calls

[rzvxa]

To sum up I suggest changing the lower function call to ==? instead, and adding similar implementations for other sections that work with path

[alerque]

Review comments from @rzvxa seem like they do need addressing. Comparison operators should always use the specific case (in)sensitive ones, and all path comparisons should be consistent.

[rzvxa]

@dangibson Hello,

Do you want to work with me to make a new flag called g:NERDTreeInsensitiveFS?
I was trying to solve issue #1373 so I created #1375 I've noticed that we are working on the same problem in 2 different places, So I was thinking maybe we can work together in the new update to introduce a way for fixing this problem, Maybe we can even find similar problems in other places and fix them too.
The reason why I've added the flag instead of assuming it automatically was that I thought checking it for sure is a hard task and if we give this choice to the user they can make their own decision with it.
The only way to know for sure the path isn't case sensitive in Windows is to write 2 different files with the same name in each directory as it is a feature per directory which you can enable by setting the directory flagged as case in sensitive in NTFS.
For MacOS it's a little bit simpler as you can only mount case-sensitive drives/virtual drives and there is no way to mix and match 2 types in the same drive(thank god, I can't see any way mixing them can go gigantically wrong!)
Assuming the case sensitivity wrong in your PR is nothing but a minor inconvenience but if we want to do it in every place, especially in file system operations it can have huge consequences.

I'm also re-requesting @alerque's review as my point earlier about the use of case insensitive equality check over using the tolower operation was wrong in practice it proved me wrong when it didn't register "pAth/TO/foo.BAR" and "path/to/foo.bar" as being equals, So I'm sorry about blocking your PR with that wrong assumption without any evidence. I would've hoped that you'd told me that you tried that approach and it didn't work.
Other than that we are approaching solving this issue in other places as well, But It will take a few PRs to solve them all. So I can't see why we shouldn't go ahead with merging this one in.
We may even be able to get a Mac user (for example @kevinkowalew generously put in the time to confirm that my PR works on a Mac machine) to test these changes out.

If you don't have time for it or you are not interested anymore I understand, But if you have any objection to this way of doing it or if you want it to be done some other way let's talk about it.

Thank you for your time, I'm sorry for my long comment.

[dangibson]

I didn't know about ==? which is why I used tolower. I use neovim and write in lua - I don't know vimscript at all. I fumbled just enough to get it working for me.
Given my lack of knowledge of vimscript I think it is probably best that I don't go making larger changes to nerdtree.

[rzvxa]

@dangibson I totally understand.
What about this PR? After all, you've already found the problem and fixed it.
With a few minor tweaks here and there we can put it behind a flag so the end user can have control over it. The only objection I remember you've had about this flag was that it should come enabled by default on Windows and Mac machines(in other words it should be disabled only on UNIX) If we want to go down that route we should make this change of behavior clear for existing users with a warning if they don't set this flag in their own config, Since there might be some users who have made their workflow around this assumption.
I really don't want to be responsible for a random file overwrite on the user's machine.

[dangibson]

How do you make a directory case sensitive on ntfs?

[dangibson]

Instead of testing a flag, what about this:

path.vim:
" Returns 1 if this path is case sensitive
" Returns 0 if this path is not case sensitive
function! s:Path.isPathCaseSensitive()
" TODO: We could check if the specified path is case sensitive - perhaps
" on Windows someone has mapped a case sensitive drive.
if g:NERDTreeInsensitiveFS
return 0 " dir is NOT case sensitive
else
return 1 " dir IS case sensitive
endif
endfunction
function! s:Path.isPathCaseInsensitive()
return 1 - self.isPathCaseSensitive()
endfunction

Then my change would become:
function! s:Path.isUnder(parent)
...
for i in range(0, l:that_count-1)
if a:parent.isPathCaseInsensitive() " was checking nerdtree#runningWindows
if tolower(self.pathSegments[i]) !=# tolower(a:parent.pathSegments[i])
return 0
...

I'm not sure why "pAth/TO/foo.BAR" and "path/to/foo.bar" would not be equal when using ==?

[rzvxa]

How do you make a directory case sensitive on ntfs?

You can do it with something like this:
fsutil.exe file setCaseSensitiveInfo "C:\my folder" enable
https://learn.microsoft.com/en-us/windows/wsl/case-sensitivity
So it's a per-directory setting.

I wanted to create a utility function to pass the path to it and find out if it's case sensitive or not, When I got to implementing it and thinking about the ways I can achieve this I found that the easiest way to do so is to do something like this:

function! nerdtree#isPathCaseSensitive(path) abort
    if nerdtree#runningWindows()
        " This is pseudo-code, I'm sure & won't work in cmd(and almost everything else)
        return system('type nul > FOO.BAR && type nul > foo.bar')
    elseif executable('touch')
        return system('touch foo.bar && touch FOO.BAR')
    else
        call nerdtree#echoWarning('Not implemented fallback to case sensitive')
        return 1
    endif
endfunction

This utility can get a path and check if the directory containing this file is case-sensitive or not.
But even if we do this let's say for example we have a file located at path/to/foo.BAR and we check if it is case sensitive or not, for that purpose we either use fsutil on Windows or check it with creating and removing files(which I would like to mention won't work on read-only directories). Now let's assume we find out that this path isn't case-sensitive, But now if we want to compare it to another path we can't simply just do something like this.

if !nerdtree#isPathCaseSensitive(path1) && !nerdtree#isPathCaseSensitive(path2) && path1 ==? path2
     " same path
endif

The second path could very well be PATH/to/foo.BAR which is another file. Notice that the case-sensitive part always could be a level higher unless we check every directory against the other one with the check in place.

[rzvxa]

Oh, I forgot about the last part, I was indeed hallucinating when I was writing that code, I couldn't get it to work maybe I just had a typo in my variable name when I wrote the original piece of code. I've just written a simple test case for it and it did pass.
I'll make sure to change it back to ==? operator before we want to merge it in.

[dangibson]

I think if we use the s:Path.isPathCaseSensitive function then for now we could assume Windows is case-insensitive since I really don't think anyone would set a folder to being case sensitive. Even if they did, it would only be an issue if they have two folders with the same name but different case. I think that would be a very rare edge case, which they could still get around by manually setting g:NERDTreeInsensitiveFS to 0.
I don't think the code should test the NERDTreeInsensitiveFS flag - it should call the Path.isPathCaseSensitive function instead since that allows for easily adding other checks in the future.

[rzvxa]

I think if we use the s:Path.isPathCaseSensitive function then for now we could assume Windows is case-insensitive since I really don't think anyone would set a folder to being case sensitive. Even if they did, it would only be an issue if they have two folders with the same name but different case. I think that would be a very rare edge case, which they could still get around by manually setting g:NERDTreeInsensitiveFS to 0. I don't think the code should test the NERDTreeInsensitiveFS flag - it should call the Path.isPathCaseSensitive function instead since that allows for easily adding other checks in the future.

I agree about the fact that we should call the path.isCaseSensitive, This way we can extend upon it in the future and use it in many places. But other than having it as a method in the path class itself, We could have a global utility function that can be also behind the path.isCaseSensitive method so we can check paths for being case-sensitive without the need for creating a path object for them(for keeping the overhead in excess checks low).

But for the other part of your comment, I should say I'm not really a big fan of assuming things when we are not sure about it being a fact.
Nobody should get their files overwritten because of somebodies else assumption. I prefer if we go with the flag for this path.isCaseSensitive method until we find a reliable way of nailing it across platforms.
Some people might have a UNIX background and set up their system this way, Or with the rise of WSL maybe it becomes more of the norm or some unwanted artifact that could happen when users use vim over ssh or something that we didn't anticipate.
I think even if we don't find a way to detect it and use a flag we can always slowly ease people into turning it on by default on Windows/mac, We can set the flag to be disabled by default, And start giving warnings on the first launch after updating the plugin for a few revisions, Then we can change the warning into notice and turn on the flag by default.
After a while, we can even get rid of that notice message.

I would also like to add that we can always deprecate a flag, But I personally won't use a tool twice if it destroys my work.

[dangibson]

NTFS is insensitive by default. Therefore, NERDTree should also be insensitive on NTFS by default. If someone is going and changing NTFS to being sensitive then they can go and change their default in NERDTree also.

I never knew about the (m) command to move/rename in nerdtree. Perhaps for moving / renaming then it could be case sensitive by default while being case insensitive for other operations (like my original issue). If they move/rename then nerdtree could see if they've specified a value for the flag and if they haven't, give a error message then and not do the operation.

So there'd be 3 values for the flag - case-sensitive, case-insensitive, and not set. If the value is not set then non-destructive operations (like my original issue) would use whatever is default for the os, while destructive operations like move/rename would give an error with a helpful message about how to set the value.

[rzvxa]

You have a good point and I like this approach, Do you want to implement it? 3 value flag seems alright to me, Some actions can fall back to case sensitive for safety and warn users to set this flag for more clarity and enhanced experience.

[dangibson]

You seem to know more about vimscript and nerdtree than I do so I think it's probably better if you do it.

[rzvxa]

I'm no Vim script expert and I enjoy writing Lua like any other sane person out there. But I like to have the freedom to jump back to Vim if I find future Neovim path conflicts with my day-to-day goals.

I switched to Neovim only because of my day job, I work in the game industry and I have to use Windows machines pretty often. I've found that Vim has some rendering artifacts when used on Windows and in those days vim development was pretty orthodox and revolved around the sole maintainer so I didn't bother fixing it and switched to Neovim.
But right now both vim9 and vim9script seem promising to me. If I see more adoption of it in the popular plugins I would even like to propose a NERDTree rewrite in vim9script which can boost the plugin speed by miles. I'm certain that nobody will agree to such a change as it is not backward compatible but these proof of concept benchmarks by the Vim project show promising results.
https://github.com/vim/vim/blob/master/README_VIM9.md

I've never been able to use NERDTree in huge monolith projects and as the technical lead of many projects, I'm ashamed to confess I've aggregated for the adoption of microservices so it plays nicely with my Vim setup.
Rewriting the plugin in vim9script can make staggering changes to the overall speed of the project but on the other hand, I don't want to break anyone's workflow with a forced update to vim9.
Also, Neovim doesn't seem to want to implement it anytime soon.
neovim/neovim#13625

So sad to see this diversion between 2 branches. I would've liked to see more compatibility between them but it is totally understandable.
After all, Neovim is a hard fork of Vim somewhere between 7 and 8(I can't remember which one exactly, and even if it was a fork of vim7 they backported many newer changes from vim8)

TL;DR I'll work on this change in my own PR after you review my code and we decide to move on with merging it I would love to see you adopt it into this PR for unification. I prefer that you get full credit for this PR since only a minimal change to the current code you wrote is necessary.

[rzvxa]

@dangibson
I've updated my PR can you give it a go? now you can simply change

 if nerdtree#runningWindows()
    if tolower(self.pathSegments[i]) !=# tolower(a:parent.pathSegments[i])
        return 0
    endif
else
    if self.pathSegments[i] !=# a:parent.pathSegments[i]
        return 0
    endif
endif

into this

if nerdtree#pathEquals(self.pathSegments[i], a:parent.pathSegments[i])

or if you want to do some custom checks manually for example this:

if nerdtree#runningWindows()
    if stridx(tolower(a:path.str()), tolower(self.path.str()), 0) ==# -1
        return {}
    endif
else
    if stridx(a:path.str(), self.path.str(), 0) ==# -1
        return {}
    endif
endif

can be turned into this

if nerdtree#caseSensitiveFS()
    if stridx(a:path.str(), self.path.str(), 0) ==# -1
        return {}
    endif
else
    if stridx(tolower(a:path.str()), tolower(self.path.str()), 0) ==# -1
        return {}
    endif
endif

Let me know what you think about it, also if you find any problem both in code and/or user experience let me know.

[rzvxa]

It can potentially fix #1241

[rzvxa]

May or may not fix #1329 but if it doesn't now that we are tackling this problem we should go for this one too.

[alerque]

Unless I read through the discussion comments too quickly, this still needs addressing. Using tolower() should be unnecessary if explicitly using a case-insensitive matching operator.

[rzvxa]

yes, I'm sorry my example was wrong.
something like this should replace the old version.

if !nerdtree#caseSensitiveFS()
    if self.pathSegments[i] !=? a:parent.pathSegments[i]

[rzvxa]

@dangibson
Hello sir,

Do you have any interest in completing this PR? If not I can create a PR containing your commits so I can add the requested changes while keeping the original author of the commits. But to be honest I would love it if you do these changes yourself, Since you already familiarized yourself with the codebase it can be an awesome thing to have somebody else contributing to the project.

If I wanted to implement this by myself it would've been a lot worse, since we had some conversation around implementing it we found many weak points and came up with a more solid implementation as a result. I love working with other developers, All of the good ideas come from people who cooperate for the greater good despite their takes on the subject matter.

I'll delay merging this PR for another week so you have time to read this comment and respond if you want to, And if you don't I totally understand.

Thanks for helping to Improve NERDTree, I've used this plugin since I was a kid in high school so It is literally keeping my childhood alive.

Sincerely yours,
Ali Rezvani