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

Use fnamemodify in Cygwin for Windows paths #386

Closed
wants to merge 1 commit into from

Conversation

janlazo
Copy link
Contributor

@janlazo janlazo commented Jun 8, 2017

From initial testing, only Files and GFiles require fnamemodify.

I can't test Locate in cygwin because it requires updatedb and locate needs a regularly updated database. Windows has its own indexing for explorer.exe but I disabled it in my system for performance. I'll revist it in #372

@janlazo janlazo changed the title [Files, GFiles] use fnamemodify for Windows paths Use fnamemodify in Cygwin for Windows paths Jun 8, 2017
@junegunn
Copy link
Owner

junegunn commented Jun 9, 2017

Thanks, this makes me wonder, would it make sense to do :p expansion inside fzf#run?

@janlazo
Copy link
Contributor Author

janlazo commented Jun 9, 2017

Sure, since it simplifies FZF and s:get_git_root will not require fnamemodify if fugitive is unavailable.

I see dict.dir handled in fzf#run
I don't know why there's a has('nvim') condition.

@janlazo
Copy link
Contributor Author

janlazo commented Jun 9, 2017

Need fnamemodify only if dict.dir exists.
getcwd always returns a unix-style path in cygwin vim.

@junegunn
Copy link
Owner

junegunn commented Jun 9, 2017

It's only for the cases where dir is not given and it's not related to the current issue. You can always git blame on that :)

junegunn/fzf@cd59e5d

Let's just make sure that we're on the same page. :p expansion is only needed on Cygwin to handle network drives, and the following patch on the main repo should do, right?

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 99fda1f..64e8cd3 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -360,6 +360,8 @@ try
 
   if has('nvim') && !has_key(dict, 'dir')
     let dict.dir = s:fzf_getcwd()
+  elseif has('win32unix')
+    let dict.dir = fnamemodify(dir, ':p')
   endif
 
   if !has_key(dict, 'source') && !empty($FZF_DEFAULT_COMMAND)
@@ -765,8 +767,6 @@ function! s:cmd(bang, ...) abort
     let opts.dir = substitute(substitute(remove(args, -1), '\\\(["'']\)', '\1', 'g'), '[/\\]*$', '/', '')
     if s:is_win && !&shellslash
       let opts.dir = substitute(opts.dir, '/', '\\', 'g')
-    elseif has('win32unix')
-      let opts.dir = fnamemodify(opts.dir, ':p')
     endif
     let prompt = opts.dir
   else

@junegunn
Copy link
Owner

junegunn commented Jun 9, 2017

By the way, I wonder if neovim runs on cygwin.

@junegunn
Copy link
Owner

junegunn commented Jun 9, 2017

fixup.

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 99fda1f..bad7449 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -361,6 +361,9 @@ try
   if has('nvim') && !has_key(dict, 'dir')
     let dict.dir = s:fzf_getcwd()
   endif
+  if has('win32unix') && has_key(dict, 'dir')
+    let dict.dir = fnamemodify(dict.dir, ':p')
+  endif
 
   if !has_key(dict, 'source') && !empty($FZF_DEFAULT_COMMAND)
     let temps.source = s:fzf_tempname().(s:is_win ? '.bat' : '')
@@ -765,8 +768,6 @@ function! s:cmd(bang, ...) abort
     let opts.dir = substitute(substitute(remove(args, -1), '\\\(["'']\)', '\1', 'g'), '[/\\]*$', '/', '')
     if s:is_win && !&shellslash
       let opts.dir = substitute(opts.dir, '/', '\\', 'g')
-    elseif has('win32unix')
-      let opts.dir = fnamemodify(opts.dir, ':p')
     endif
     let prompt = opts.dir
   else

@janlazo
Copy link
Contributor Author

janlazo commented Jun 9, 2017

Thanks for the reminder. I use vim-rooter so thanks for the patch :)
I'm fine with the patch but need to test the prompt.

By the way, I wonder if neovim runs on cygwin.

It does if you don't use mintty.

@janlazo
Copy link
Contributor Author

janlazo commented Jun 9, 2017

The prompt works for me and is better for me because if I pass a Windows-style path to FZF, I expect the same style in the prompt.

@junegunn
Copy link
Owner

junegunn commented Jun 9, 2017

Okay, thanks for the feedback. Let me push the fix.

@janlazo
Copy link
Contributor Author

janlazo commented Jun 9, 2017

Thanks.

@janlazo janlazo deleted the cygwin branch June 9, 2017 03:33
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