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

Add g:NERDTreeFindResolveSymlinks for NERDTreeFind #1412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EvgeniyBlinov
Copy link

Description of Changes

Add g:NERDTreeFindResolveSymlinks for NERDTreeFind

" DEFAULT value | open NERDTreeFind in symlink resolved dir
let g:NERDTreeFindResolveSymlinks = 1

" open NERDTreeFind in file current dir
let g:NERDTreeFindResolveSymlinks = 0


New Version Info

@rzvxa
Copy link
Member

rzvxa commented Mar 16, 2024

Hello,

First off thank you for your contribution. It seems like a good idea but I'm not sure if using a flag for it is the best implementation, I personally think having 2 commands like NERDTreeFind and NERDTreeFindSymlink is a better way of doing this but we can discuss if you have a good use case for this approach.

There are also #1404 and issues mentioned there, which are somewhat related to this, Take a look at them if have the time, it would be great if we could work toward having better control over how NerdTree is treating the symlinks in general. For example, having the Path.Resolve method handle how all symlinks are treated seems like a good solution to me however we should make sure there are no weird assumptions throughout the code that could break it.

@EvgeniyBlinov
Copy link
Author

EvgeniyBlinov commented Mar 17, 2024

Please, add any solution. Because current NERDTreeFind blocking my workflow.

@rzvxa
Copy link
Member

rzvxa commented Mar 17, 2024

It is definitely on the roadmap, I usually don't go overboard with symlinks but I know a lot of people and projects that do. So keep an eye out for the next few updates.

Until then feel free to use the patch you've submitted or get on board to help with adding an option for symlink resolution in general, Either way, I'm going to make sure that you get credited for this PR no matter what(even if the proposed option doesn't get in).

Have a great day!

@EvgeniyBlinov
Copy link
Author

Please, add any solution. Because current NERDTreeFind blocking my workflow.

@rzvxa up
Please, add any solution.

@rzvxa
Copy link
Member

rzvxa commented Apr 24, 2024

@EvgeniyBlinov Sorry for it getting a bit delayed, I just want to let you know I haven't forgotten this issue; I'm currently just a bit swamped with a deadline.

I'll try to get it into upstream by the next week end, I might need a few more days I promise it won't be much longer than that.

Please excuse me for this inconvenience.

@rzvxa
Copy link
Member

rzvxa commented Apr 24, 2024

Just to be clear, I didn't merge it yet, since we have to make sure it wouldn't break anything. I'm not too keen on breaking anyone's workflow since NERDTree has been in a stable state for a long long time and people tend to forget about it.

I want you to know that I truly appreciate what you've done here, I hope you understand the reasoning behind my decision.

@rzvxa
Copy link
Member

rzvxa commented May 13, 2024

@EvgeniyBlinov I'm extremely sorry for the delay, I'll start working on it today and ping you when there is a working PR so you can test and review the changes.

@rzvxa
Copy link
Member

rzvxa commented May 13, 2024

@EvgeniyBlinov This doesn't work, Vim always resolves the path on expansion, Are you sure your changes are creating the desired effect? I can't reproduce what you've described here.

@EvgeniyBlinov
Copy link
Author

EvgeniyBlinov commented May 15, 2024

@EvgeniyBlinov This doesn't work, Vim always resolves the path on expansion, Are you sure your changes are creating the desired effect? I can't reproduce what you've described here.
Test case:

mkdir -p ~/TMP/nerdtreesltestdir/
cd ~/TMP/nerdtreesltestdir/
ln -s ~/.bashrc ./
vim ./.bashrc

NerdTree open in HOME directory. But the expected behavior is opening in the current directory ~/TMP/nerdtreesltestdir/ .
On commit f3a4d8e

@rzvxa
Copy link
Member

rzvxa commented May 15, 2024

@EvgeniyBlinov This doesn't work, Vim always resolves the path on expansion, Are you sure your changes are creating the desired effect? I can't reproduce what you've described here.
Test case:

mkdir -p ~/TMP/nerdtreesltestdir/
cd ~/TMP/nerdtreesltestdir/
ln -s ~/.bashrc ./
vim ./.bashrc

NerdTree open in HOME directory. But the expected behavior is opening in the current directory ~/TMP/nerdtreesltestdir/ . On commit f3a4d8e

Have you had any chance of fixing it by not resolving the path? I've tried your proposed change in this PR with your exact example, But it doesn't solve the issue.

The problem is that Vim's behavior for symlinks is to resolve the path, So in many places when you pass in a symlink it tries to expand it to the real path internally, and one way or another it would get resolved somewhere(it can happen in many different operations even a simple thing like 1 ctrl-g would give the resolved path).

To truly solve this issue we have to stop using internal functions like expand which does the resolution for us, There might be some other areas that need to be addressed. Overall it is more complicated than what I originally thought, Unless there is something that I'm missing.

Does the proposed change in this PR solve your issue? because I couldn't get the desired effect you've mentioned. If it does, can you share a short video of it with me? What is your Vim version? There might be something different between our setups.

@EvgeniyBlinov
Copy link
Author

@EvgeniyBlinov This doesn't work, Vim always resolves the path on expansion, Are you sure your changes are creating the desired effect? I can't reproduce what you've described here.
Test case:

mkdir -p ~/TMP/nerdtreesltestdir/
cd ~/TMP/nerdtreesltestdir/
ln -s ~/.bashrc ./
vim ./.bashrc

NerdTree open in HOME directory. But the expected behavior is opening in the current directory ~/TMP/nerdtreesltestdir/ . On commit f3a4d8e

Have you had any chance of fixing it by not resolving the path? I've tried your proposed change in this PR with your exact example, But it doesn't solve the issue.

The problem is that Vim's behavior for symlinks is to resolve the path, So in many places when you pass in a symlink it tries to expand it to the real path internally, and one way or another it would get resolved somewhere(it can happen in many different operations even a simple thing like 1 ctrl-g would give the resolved path).

To truly solve this issue we have to stop using internal functions like expand which does the resolution for us, There might be some other areas that need to be addressed. Overall it is more complicated than what I originally thought, Unless there is something that I'm missing.

Does the proposed change in this PR solve your issue? because I couldn't get the desired effect you've mentioned. If it does, can you share a short video of it with me? What is your Vim version? There might be something different between our setups.

Dockerfile

FROM ubuntu:24.04

SHELL ["/bin/bash", "-c"]

ARG DEBIAN_FRONTEND=noninteractive
ENV DEBIAN_FRONTEND=noninteractive

RUN apt-get update && apt-get install -y \
    vim \
    git \
    curl

USER ubuntu

WORKDIR /home/ubuntu

ENV HOME=/home/ubuntu

RUN mkdir -p /home/ubuntu/.vim/pack/vendor/start && \
    mkdir -p /home/ubuntu/.vim/pack/vendor/opt


RUN git clone https://github.com/preservim/nerdtree /home/ubuntu/.vim/pack/vendor/opt/nerdtree-bug  && \
    cd $_ && \
    git checkout f3a4d8e

RUN git clone https://github.com/EvgeniyBlinov/nerdtree /home/ubuntu/.vim/pack/vendor/opt/nerdtree-patched && \
    cd $_ && \
    git checkout findAndRevealPath_symlink_resolve

RUN mkdir -p ~/TMP/nerdtreesltestdir/ && \
    cd ~/TMP/nerdtreesltestdir/ && \
    ln -s ~/.bashrc ./

vim version

vim --version
VIM - Vi IMproved 9.1 (2024 Jan 02, compiled Mar 31 2024 00:15:53)
Included patches: 1-16
Modified by team+vim@tracker.debian.org
Compiled by team+vim@tracker.debian.org
Huge version without GUI.  Features included (+) or not (-):
+acl               +file_in_path      +mouse_urxvt       -tag_any_white
+arabic            +find_in_path      +mouse_xterm       -tcl
+autocmd           +float             +multi_byte        +termguicolors
+autochdir         +folding           +multi_lang        +terminal
-autoservername    -footer            -mzscheme          +terminfo
-balloon_eval      +fork()            +netbeans_intg     +termresponse
+balloon_eval_term +gettext           +num64             +textobjects
-browse            -hangul_input      +packages          +textprop
++builtin_terms    +iconv             +path_extra        +timers
+byte_offset       +insert_expand     -perl              +title
+channel           +ipv6              +persistent_undo   -toolbar
+cindent           +job               +popupwin          +user_commands
-clientserver      +jumplist          +postscript        +vartabs
-clipboard         +keymap            +printer           +vertsplit
+cmdline_compl     +lambda            +profile           +vim9script
+cmdline_hist      +langmap           -python            +viminfo
+cmdline_info      +libcall           +python3           +virtualedit
+comments          +linebreak         +quickfix          +visual
+conceal           +lispindent        +reltime           +visualextra
+cryptv            +listcmds          +rightleft         +vreplace
+cscope            +localmap          -ruby              +wildignore
+cursorbind        -lua               +scrollbind        +wildmenu
+cursorshape       +menu              +signs             +windows
+dialog_con        +mksession         +smartindent       +writebackup
+diff              +modify_fname      +sodium            -X11
+digraphs          +mouse             -sound             +xattr
-dnd               -mouseshape        +spell             -xfontset
-ebcdic            +mouse_dec         +startuptime       -xim
+emacs_tags        +mouse_gpm         +statusline        -xpm
+eval              -mouse_jsbterm     -sun_workshop      -xsmp
+ex_extra          +mouse_netterm     +syntax            -xterm_clipboard
+extra_search      +mouse_sgr         +tag_binary        -xterm_save
-farsi             -mouse_sysmouse    -tag_old_static
   system vimrc file: "/etc/vim/vimrc"
     user vimrc file: "$HOME/.vimrc"
 2nd user vimrc file: "~/.vim/vimrc"
      user exrc file: "$HOME/.exrc"
       defaults file: "$VIMRUNTIME/defaults.vim"
  fall-back for $VIM: "/usr/share/vim"
Compilation: gcc -c -I. -Iproto -DHAVE_CONFIG_H -Wdate-time -g -O2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -ffile-prefix-map=/build/vim-g8cgSd/vim-9.1.0016=. -flto=auto -ffat-lto-objects -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection -fdebug-prefix-map=/build/vim-g8cgSd/vim-9.1.0016=/usr/src/vim-2:9.1.0016-1ubuntu7 -DSYS_VIMRC_FILE=\"/etc/vim/vimrc\" -DSYS_GVIMRC_FILE=\"/etc/vim/gvimrc\" -D_REENTRANT -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
Linking: gcc -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -Wl,-z,relro -Wl,-z,now -Wl,--as-needed -o vim -lm -ltinfo -lselinux -lsodium -lacl -lattr -lgpm -L/usr/lib/python3.12/config-3.12-x86_64-linux-gnu -lpython3.12 -ldl -lm

Commands

## build
docker build -t vim-test .

## run
docker run --rm --name vim-test vim-test tail -f /dev/null

## exec into container
docker exec -it vim-test bash

Commands in container

## bug reproducing
( cd /home/ubuntu/.vim/pack/vendor/start && ln -s ./../opt/nerdtree-bug ./nerdtree )
vim ~/TMP/nerdtreesltestdir/.bashrc +:NERDTreeFind

## clear package
rm /home/ubuntu/.vim/pack/vendor/start/nerdtree

## reproducing patched version
( cd /home/ubuntu/.vim/pack/vendor/start && ln -s ./../opt/nerdtree-patched ./nerdtree )
echo 'let g:NERDTreeFindResolveSymlinks = 0' > /home/ubuntu/.vim/vimrc
vim ~/TMP/nerdtreesltestdir/.bashrc +:NERDTreeFind

@rzvxa
Copy link
Member

rzvxa commented May 22, 2024

Awesome, It is one of the more elaborate reproduction steps I've ever seen here.
Let me study it and I'll reach out to you.

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.

None yet

2 participants