Skip to content

Fix Vim filename escaping for StageDiff + more #254

Closed
wants to merge 1 commit into from

2 participants

@guns
guns commented Sep 21, 2012
  • Filename arguments to Gedit should not be escaped, as s:Edit already
    s:fnameescape()s when passing filenames to the next command.

    Fixes:

    A filename with a Vim metacharacter in the Gstatus window cannot be
    diffed via StageDiff because it becomes double escaped:

      #   modified:   etc/%mutt/muttrc
    

    Pressing 'D' on this line opens two new buffers:

      t/muttrc            (filename shifted by length of repo().dir())
      etc/\%mutt/muttrc   (one actual backslash)
    
  • s:shellesc should pass an optional {special} argument to
    shellescape() when it is used to sanitize filenames for Vim functions
    and not direct system calls. Vim commands will do one more pass over
    the filename and erroneously expand metacharacters like '%'

    Fixes:

      #   modified:   etc/%mutt/muttrc
    
      This same file cannot be processed correctly by s:StagePatch or
      s:StageDiffEdit. Pressing 'p' on this line results in "No
      changes." from git, and pressing 'dp' results in:
    
      fatal: ambiguous argument 'etc/README.markdownmutt/muttrc' ...
    
      Where README.markdown is the current active buffer, replacing
      the '%' in the filename.
    

In addition to these bugs (now fixed), Fugitive still can't handle
filenames that show up as quoted in git status:

a"b.txt  a\b.txt  a❤b.txt

git status shows these files as:

#   "a\"b.txt"
#   "a\\b.txt"
#   "a\342\235\244b.txt"

and fugitive breaks because it doesn't strip the surrounding quotes.
I don't have time to fix this today, but I'll come back to it if it
annoys me in the future.

@tpope
Owner
tpope commented Mar 6, 2013

Is this fixed by 1da788a by any chance? Happy to revisit if not.

@guns
guns commented Mar 6, 2013

No, I'm afraid it does not, although the error is slightly different:
when pressing D on the etc/%mutt/muttrc example from above, a new
buffer is opened as before: etc/\%mutt/muttrc (with the literal
backslash), but there is now a new error message:

"/opt/haus/" Illegal file name

The full path to this mutt config file is /opt/haus/etc/%mutt/muttrc.

FWIW, the changes I posted above still work for me.

Thanks for looking into it!

@tpope
Owner
tpope commented Mar 7, 2013

Actually I didn't even realize the patch would even apply.

I think the change I linked about means we do need to escape spaces at least. Another obvious inclusion is | (and ", although those won't work yet for reasons you pointed out). Can you wrap the appropriate spots in a escape(..., ' |') and squash? I think that's more than good enough for a first pass.

@guns
guns commented Mar 8, 2013

Looking at the problem again, the metacharacter expansion is occuring
because these calls are being executed via:

execute 'Gedit '.s:fnameescape(…)

Calling the underlying functions directly avoids this expansion. I have
an initial commit here:

guns@0be205d

but it's just a quick adaptation (but better than the mess in this pulll
request). I'll polish it and look for more edge cases tomorrow.

@guns
guns commented Mar 8, 2013

Ah, that doesn't work either. I haven't been able to spend time on this,
but the posted patch is broken. Sorry for the noise.

I think what might be better than solving this particular case is to
jump to properly parsing and passing git status --porcelain output,
even when it outputs C-style string literals.

So I'll close this issue for now and take some time to think up a better
suggestion.

@guns guns closed this Mar 8, 2013
@guns guns deleted the guns:fix-vim-filename-escaping branch Mar 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.