Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

* fix duplicated slash in s:Path.isUnder() (on windows OS) #1122

Merged
merged 7 commits into from
May 14, 2020
Merged

* fix duplicated slash in s:Path.isUnder() (on windows OS) #1122

merged 7 commits into from
May 14, 2020

Conversation

Eugenij-W
Copy link
Contributor

@Eugenij-W Eugenij-W commented May 12, 2020

Fix exception in NERDTreeFind.
On windows OS and if the file is located in the root directory of the disk, TreeDirNode.reveal() trow exception because Path.isUnder() duplicate slash on root drive paths (look like "c://").

Description of Changes

using substitute() to add a slash.
Rewrite NERDTreePath.isUnder() and NERDTreePath.isAncestor() for direct comparison of paths without transformations.


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 merge, tag the merge commit, e.g. git tag -a 3.1.4 -m "v3.1.4" && git push origin --tags

Eugenij-W and others added 4 commits May 13, 2020 03:11
…irectory on drive Path.str() return path with [back]slash)
…rect comparison of paths without transformations
The str() function returns "C:\" on the root folder and "C:\temp" on
non-root folders, one with and one without a trailing backslash. This
inconsistency needs to be handled so the stridx() function will work
correctly.
This commit handles an edge case that can be triggered with these
commands:
    :cd /home/me
    :e /foobar.txt  (an existing file)
    :NERDTreeFind
What happened was the root directory name '/' was being Resolved(), and
the trailing (and only) slash was being removed. The NERDTree was then
created in the current working directory, instead of the root directory.
:NERDTreeFind then wasn't able to find foobar.txt, and printed an error.
@PhilRunninger
Copy link
Member

@Eugenij-W , why did you change your mind on the first commit, the one using substitute()?

@Eugenij-W
Copy link
Contributor Author

Eugenij-W commented May 14, 2020

the first version is quite suitable, but not suitable because it will give an incorrect result for 'C:\' isUnder 'C:\', and i think it is slow on large paths, the second version only checks the data that is required, without concatenations and other things.

@PhilRunninger PhilRunninger mentioned this pull request May 14, 2020
Copy link
Member

@PhilRunninger PhilRunninger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the succinctness of the substitute() function, but I'm OK with either approach. Do you have any metrics to back up your claim?

I tested this PR on my MacOS, and discovered this edge case. It happens only when NERDTree hasn't already been created.

:cd /home/me
:e /foobar.txt
:NERDTreeFind

I tracked it down to the lib/nerdtree/creator.vim file, and uploaded the fix in #1124. Copy that change into your PR, so it's all in one.

Thanks for tracking this down.

let this = self.str()
let that = a:path.str()
return stridx(that, this) ==# 0
return a:path.isUnder(self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the variable names lead to some ambiguity. Change path to child. This function isn't used within NERDTree, but we'll leave it, in case any other plugins use it.

let this = self.str()
let that = a:path.str()
return stridx(this, that . s:Path.Slash()) ==# 0
if nerdtree#runningWindows() && a:path.drive !=# self.drive
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same ambiguity issue here. Rename a:path to a:parent, and that_count to parent_count.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, glad to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants