-
Notifications
You must be signed in to change notification settings - Fork 49
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
Avoid breaking current directory on neovim #95
Conversation
Your patch is not compatible with vim, because of haslocaldir(-1, 0) (Travis arguing) |
Can we merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have multiple comments in this PR:
- please split the tcd fix and the pushd feature in two different PRs
- make the PR for branch next
- for the tcd fix, could you provide me a way to reproduce your error?
- for the pushd feature, please see the comments, and:
- use it everywhere
- don't bother of tcd fix in the pushd PR, it will be merged when the two PR will be merged
|
||
" magit#utils#pushd_root goes to git root directory, saving current directory | ||
" prior to changing the cwd. | ||
function! magit#utils#pushd_root() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushd
and and popd
are not reentrant. Please add a counter in pushd
, such as:
function pushd()
b:pushd_cnt++
if ( b:pushd_cnt <= 0 )
raise error
else if ( b:pushd_cnt == 1)
b:_vimagit_cwd = cwd
chdir root
function popd()
b:pushd_cnt--
if ( b:pushd_cnt < 0 )
raise error
else if ( b:pushd_cnt == 0 )
chdir b:_vimagit_cwd
if has('nvim') | ||
let chdir = haslocaldir() ? 'lcd' : haslocaldir(-1, 0) ? 'tcd' : 'cd' | ||
else | ||
let chdir = exists('*haslocaldir') && haslocaldir() ? 'lcd' : 'cd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has(nvim)
and exists('*haslocaldir')
should be evaluated at plugin initialization (outside of a function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can externalize both checks, but that would only make a difference if we define a function for each case. Otherwise we're swapping a strong check (i.e. has('nvim')
) that won't change during runtime to something that can be overriden (i.e. g:magit_running_on_neovim=1
).
By conditioning the implementation like that, we end up with an unmaintainable version of it:
if has('nvim')
function! magit#utils#chdir(dir)
let chdir = haslocaldir() ? 'lcd' : haslocaldir(-1, 0) ? 'tcd' : 'cd'
exec chdir a:dir
endfunction
elseif exists('*haslocaldir')
function! magit#utils#chdir(dir)
let chdir = haslocaldir() ? 'lcd' : 'cd'
exec chdir a:dir
endfunction
else
function! magit#utils#chdir(dir)
cd a:dir
endfunction
endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think it is unmaintainable? I think this is a good solution, even better than my previous solution of storing the chdir command in a variable.
I do prefer this solution. Maybe this is a micro optimization, but if vimagit wins two if statements during runtime, I prefer that.
Several points here:
has('nvim')
is not enough check, you should also check nvim version, because not all nvim version has tcd.- your integration has failed because of Error when vim files are in a path with spaces #89 : now,
a:dir
must be escaped withfnameescape(a:dir)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this solution, we end up with 3 functions to maintain, so we have three places to add fnameescape
to.
Also,this is a premature optimization; we're trading readability and maintainability for theoretical performance gains, which never proved to be actually necessary.
Plus, if we scale the amount of time spent on operations, changing the directory setting of nvim/vim is way faster than making a system call to interact with git, for example, so even if vimagit was extremely slow, this optimization would not perceivable results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I won't fight against this, in the end this is only a micro optim.
One day, I hope changing directory won't be necessary... I already tried to play around with GIT_WORK_TREE and GIT_DIR environment variables, but it was a dead end, I can't remember why.
I should give another try: in case it works, vimagit won't need to change dir anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not mix two solutions based on third party presence.
At least, a mix of 2 solutions based on git version would be OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realised my comment was stupid. Still, you can use --git-dir
and --work-dir
, so you don't need to mess with env vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already tried these. I try to remember why I did not went to the end... Maybe because these options appeared lately in git. I'll check that, but it could worth a try to support both:
- Old git version with magit#chdir
- Not so old git version with --git-dir
" magit#utils#pushd_root goes to git root directory, saving current directory | ||
" prior to changing the cwd. | ||
function! magit#utils#pushd_root() | ||
if has('nvim') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you use magit#utils#chdir
here?
|
||
if root !=? cwd | ||
call magit#utils#chdir(root) | ||
let g:_vimagit_cwd = cwd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_vimait_cwd
variable must be local to the current buffer (user may have several magit session opened in one vim session).
7e5cef4
to
b1a1485
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is close to be merged, thanks for your hard work and your patience!
@@ -55,7 +60,7 @@ endfunction | |||
function! magit#utils#system(...) | |||
let dir = getcwd() | |||
try | |||
call magit#utils#lcd(magit#git#top_dir()) | |||
execute magit#utils#chdir(magit#git#top_dir()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why you changed call
to execute
? And why don't you use execute
everywhere in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not supposed to happen. Will revert to call
.
I'll probably need your help to write a test here. " Open neovim
tcd /tmp/git1
tabnew
tcd /tmp/git2
tabprevious
echo getcwd() "== /tmp/git1
Magit
tabnext
echo getcwd() "== /tmp/git2
Magit
tabprevious
echo getcwd() "== /tmp/git2 |
if has('nvim') | ||
let chdir = haslocaldir() ? 'lcd' : haslocaldir(-1, 0) ? 'tcd' : 'cd' | ||
else | ||
let chdir = exists('*haslocaldir') && haslocaldir() ? 'lcd' : 'cd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not mix two solutions based on third party presence.
At least, a mix of 2 solutions based on git version would be OK.
" This is a dirty hack to fix tcd breakages on neovim. | ||
" Future work should be based on nvim API and has a `if has('nvim')` check. | ||
if has('nvim') | ||
let chdir = haslocaldir() ? 'lcd' : haslocaldir(-1, 0) ? 'tcd' : 'cd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:tcd
has been introduced right after nvim 0.1.3 (I did not check, it should be officially present since nvim 0.1.4).
Before :tcd
introduction, haslocaldir(-1, 0)
will return an error:
E118: Too many arguments for function: haslocaldir
E15: Invalid expression: haslocaldir(-1, 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. I think nvim allows checking for version with has('nvim-0.1.4')
. If not I'll probably be able to exists('*tcd')
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer exists() solution .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I already implemented using exists
. Github doesn't update diffs properly now... :/
Neovim has support for tab-local settings, additionally to global and window-local. When a tab is set with `tcd`, if the directory is changed with `cd`, the tab-local setting is discarded. This patch fixes the current bug by setting checking the granularity of current setting prior to changing directory.
Good job 👍 |
Neovim allows a
tab-local
directory. The previous directory change function was breaking tab local directory configuration, changing it withcd
.This PR fixes the problem.