fix(autoload/tar.vim): Not preserving pwd #17339
Conversation
runtime/autoload/tar.vim
Outdated
| @@ -432,11 +433,14 @@ if v:shell_error != 0 | |||
| set nomod | |||
|
|
|||
| let &report= repkeep | |||
| exe "cd ".pwdkeep | |||
There was a problem hiding this comment.
| exe "cd ".pwdkeep | |
| cd - |
and then get rid of pwdkeep var
There was a problem hiding this comment.
Shouldn't it be lcd everywhere then?
Like:
" attempt to change to the indicated directory
try
exe "lcd ".fnameescape(tmpdir)
...
There was a problem hiding this comment.
@chrisbra cd - might not do what is expected if v:shell_error:
if v:shell_error != 0
cd ..
call s:Rmdir("_ZIPVIM_")
exe "cd ".fnameescape(curdir)
echohl Error | echo "***error*** (tar#Read) sorry, unable to open or extract ".tarfile." with ".fname | echohl None
endif
It would change it back to the original path and then cd - would change it back to the tmpdir.
So I would use exe "cd " .. fnameescape(curdir) instead of cd -.
And use lcd everywhere too.
There was a problem hiding this comment.
That is unfortunate we can't easily use defer with old lambdas:
" attempt to change to the indicated directory
try
exe "lcd ".fnameescape(tmpdir)
defer {-> lcd -}
There was a problem hiding this comment.
yes makes sense. We could potentially also use a try/catch/finally block.
runtime/autoload/tar.vim
Outdated
| @@ -569,6 +573,7 @@ fun! tar#Write(fname) | |||
| setlocal nomod | |||
|
|
|||
| let &report= repkeep | |||
| exe "cd ".pwdkeep | |||
runtime/autoload/tar.vim
Outdated
| @@ -280,6 +280,7 @@ endfun | |||
| " --------------------------------------------------------------------- | |||
| " tar#Read: {{{2 | |||
| fun! tar#Read(fname,mode) | |||
| let pwdkeep = getcwd() | |||
There was a problem hiding this comment.
there is already curdir saving the same info down 15 lines:
let curdir= getcwd()
runtime/autoload/tar.vim
Outdated
| endfun | ||
|
|
||
| " --------------------------------------------------------------------- | ||
| " tar#Write: {{{2 | ||
| fun! tar#Write(fname) | ||
| let pwdkeep = getcwd() | ||
| let repkeep= &report | ||
| set report=10 | ||
| " temporary buffer variable workaround because too fucking tired. but it works now |
There was a problem hiding this comment.
And while we are here, it probably would be good to remove this strong expression.
| endfun | ||
|
|
||
| " --------------------------------------------------------------------- | ||
| " tar#Write: {{{2 | ||
| fun! tar#Write(fname) | ||
| let pwdkeep = getcwd() |
There was a problem hiding this comment.
@habamax while in tar#Read we can use curdir because is set to the original working directory, in tar#Write it seems to be set to a temporary directory instead, e.g. /tmp/vemgadL/1 (tried this in Linux Alpine), so we probably still need a separate variable to restore the original pwd?
There was a problem hiding this comment.
So while it might not be DRY, having a dedicated variable, like pwdkeep, at least in tar#Write, seems the easiest way to restore the original pwd, and at that stage might as well use it in tar#Read for consistency?
There was a problem hiding this comment.
For tar#Write for sure we need another var to keep&restore current directory.
It is not clear what problem OG author had, but apparently there was something.
There was a problem hiding this comment.
I wonder if there are test cases, simple grep for tar#Write shows nothing :(
There was a problem hiding this comment.
What the original author meant to do goes beyond my understanding. I'll keep the pwdkeep variable for tar#Write while implementing the other suggested changes.
|
Thanks for the feedback. I've implemented the suggested changes. |
|
thanks. Should be fine to include now. Perhaps we can create a tar test in the future. Should be relatively straight forward by using test_plugin_zip.vim as an example |
…g tar files While at it, use `:lcd` to temporarily set the window local directory instead of `:cd` for the global working directory. fixes: vim/vim#17334 closes: vim/vim#17339 vim/vim@719ec0f Co-authored-by: Michele Sorcinelli <michelesr@autistici.org>
…g tar files While at it, use `:lcd` to temporarily set the window local directory instead of `:cd` for the global working directory. fixes: vim/vim#17334 closes: vim/vim#17339 vim/vim@719ec0f Co-authored-by: Michele Sorcinelli <michelesr@autistici.org>
…g tar files While at it, use `:lcd` to temporarily set the window local directory instead of `:cd` for the global working directory. fixes: vim/vim#17334 closes: vim/vim#17339 vim/vim@719ec0f Co-authored-by: Michele Sorcinelli <michelesr@autistici.org>
Fix #17334