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.

Sign up for GitHub

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

Jump to bottom

Avoid breaking current directory on neovim #95

Merged
merged 1 commit into from
Oct 26, 2016
Merged

Avoid breaking current directory on neovim #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions autoload/magit/git.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ endfunction
function! magit#git#is_work_tree(path)
let dir = getcwd()
try
call magit#utils#lcd(a:path)
call magit#utils#chdir(a:path)
let top_dir=magit#utils#strip(
\ system(g:magit_git_cmd . " rev-parse --show-toplevel")) . "/"
if ( v:shell_error != 0 )
return ''
endif
return top_dir
finally
call magit#utils#lcd(dir)
call magit#utils#chdir(dir)
endtry
endfunction

Expand All @@ -68,7 +68,7 @@ endfunction
function! magit#git#set_top_dir(path)
let dir = getcwd()
try
call magit#utils#lcd(a:path)
call magit#utils#chdir(a:path)
let top_dir=magit#utils#strip(
\ system(g:magit_git_cmd . " rev-parse --show-toplevel")) . "/"
if ( v:shell_error != 0 )
Expand All @@ -81,7 +81,7 @@ function! magit#git#set_top_dir(path)
let b:magit_top_dir=top_dir
let b:magit_git_dir=git_dir
finally
call magit#utils#lcd(dir)
call magit#utils#chdir(dir)
endtry
endfunction

Expand Down
4 changes: 2 additions & 2 deletions autoload/magit/state.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ function! magit#state#update() dict

let dir = getcwd()
try
call magit#utils#lcd(magit#git#top_dir())
call magit#utils#chdir(magit#git#top_dir())
call magit#utils#refresh_submodule_list()
for [mode, diff_dict_mode] in items(self.dict)
let status_list = magit#git#get_status()
Expand All @@ -289,7 +289,7 @@ function! magit#state#update() dict
endfor
endfor
finally
call magit#utils#lcd(dir)
call magit#utils#chdir(dir)
endtry

" remove files that have changed their mode or been committed/deleted/discarded...
Expand Down
25 changes: 15 additions & 10 deletions autoload/magit/utils.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,17 @@ function! magit#utils#is_submodule(dirname)
return ( index(s:submodule_list, a:dirname) != -1 )
endfunction

" s:magit_cd_cmd: plugin variable to choose lcd/cd command, 'lcd' if exists,
" 'cd' otherwise
let s:magit_cd_cmd = exists('*haslocaldir') && haslocaldir() ? 'lcd ' : 'cd '
" magit#utils#lcd: helper function to lcd. use cd if lcd doesn't exists
function! magit#utils#lcd(dir)
execute s:magit_cd_cmd . fnameescape(a:dir)
" magit#utils#chdir will change the directory respecting
" local/tab-local/global directory settings.
function! magit#utils#chdir(dir)
" This is a dirty hack to fix tcd breakages on neovim.
" Future work should be based on nvim API.
if exists(':tcd')
let chdir = haslocaldir() ? 'lcd' : haslocaldir(-1, 0) ? 'tcd' : 'cd'
Copy link
Owner

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)

Copy link
Contributor Author

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').

Copy link
Owner

Choose a reason for hiding this comment

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

I prefer exists() solution .

Copy link
Contributor Author

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... :/

else
let chdir = exists('*haslocaldir') && haslocaldir() ? 'lcd' : 'cd'
Copy link
Owner

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).

Copy link
Contributor Author

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

Copy link
Owner

@jreybert jreybert Oct 21, 2016 •
edited

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:

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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

endif
execute chdir fnameescape(a:dir)
endfunction

" magit#utils#clear_undo: this function clear local undo history.
Expand Down Expand Up @@ -55,7 +60,7 @@ endfunction
function! magit#utils#system(...)
let dir = getcwd()
try
call magit#utils#lcd(magit#git#top_dir())
call magit#utils#chdir(magit#git#top_dir())
" List as system() input is since v7.4.247, it is safe to check
" systemlist, which is sine v7.4.248
if exists('*systemlist')
Expand All @@ -75,7 +80,7 @@ function! magit#utils#system(...)
endif
endif
finally
call magit#utils#lcd(dir)
call magit#utils#chdir(dir)
endtry
endfunction

Expand All @@ -88,15 +93,15 @@ endfunction
function! magit#utils#systemlist(...)
let dir = getcwd()
try
call magit#utils#lcd(magit#git#top_dir())
call magit#utils#chdir(magit#git#top_dir())
" systemlist since v7.4.248
if exists('*systemlist')
return call('systemlist', a:000)
else
return split(call('magit#utils#system', a:000), '\n')
endif
finally
call magit#utils#lcd(dir)
call magit#utils#chdir(dir)
endtry
endfunction

Expand Down