Tab complete file paths containing spaces #121

Closed
wants to merge 16 commits into
from

Conversation

Projects
None yet
4 participants
Owner

georgebrock commented Mar 4, 2014

  • Adds support for tab completing file names with spaces.
  • Handles quoted arguments correctly (i.e. spaces are not escaped within quotes)

Fixes #83

@zamith zamith and 1 other commented on an outdated diff Mar 6, 2014

lib/gitsh/completer.rb
@@ -117,6 +126,18 @@ def matchable_input
File.expand_path(input)
end
end
+
+ def escape(path)
+ if quoted?
+ path
+ else
+ path.gsub(' ', '\ ')
+ end
+ end
+
+ def quoted?
+ @_quoted ||= line_buffer[0...-input.length].end_with?('"', "'")
@zamith

zamith Mar 6, 2014

We could have done this with

line_buffer.gsub(/#{input}$/, '').end_with?('"', "'")

which I think reads better.

@georgebrock

georgebrock Mar 8, 2014

Owner

I'm not sure if this is an improvement: the input would need escaping before interpolating into the regular expression.

@zamith

zamith Mar 8, 2014

Something like this?

line_buffer.gsub(/#{Regexp.quote(input)}$/, '').end_with?('"', "'")

Probably it is not a big gain, you're right. And it is a private method, so the implications are not big and it can always be changed in the future.

georgebrock added the bug label Mar 8, 2014

georgebrock added this to the 0.6 milestone Mar 8, 2014

@houndci-bot houndci-bot commented on the diff Mar 19, 2014

spec/units/parser_spec.rb
@@ -66,6 +66,24 @@
)
end
+ it 'parses a command with unquoted arguments containing escaped characters' do
+ expect(parser).to parse(%q(add some\ file.txt hello\"world \\)).as(
@houndci-bot

houndci-bot Mar 19, 2014

Line is too long. [82/80]

@georgebrock

georgebrock Mar 19, 2014

Owner

I can't think of a more succinct way of describing this example, so I'm happy to leave this at 82 characters.

@mike-burns

mike-burns Mar 31, 2014

Owner

If you rename some file.txt to a file.txt, it will be shorter!

@mike-burns

mike-burns Mar 31, 2014

Owner

Aha, it looks like @houndci commented on the wrong line. You're right, this is a great description.

@houndci-bot houndci-bot and 2 others commented on an outdated diff Mar 21, 2014

lib/gitsh/completer.rb
@@ -21,7 +21,7 @@ def initialize(input, readline, env, internal_command)
end
def complete
- available_completers.map(&:completions).flatten
+ available_completers.map(&:completions).flatten.map { |arg| escape(arg) }
@houndci-bot

houndci-bot Mar 21, 2014

Line is too long. [81/80]

@mike-burns

mike-burns Mar 31, 2014

Owner

How about Enumerable#flat_map?

There's also the slightly less clear Enumerable#inject:

available_completers.inject do |acc, completers|
  acc + completers.completions.map { |arg| escape(arg) }
end
@georgebrock

georgebrock Apr 2, 2014

Owner

Good call, thanks. I always forget about #flat_map.

@houndci-bot houndci-bot and 1 other commented on an outdated diff Mar 21, 2014

spec/units/completer_spec.rb
+ it 'completes quoted heads' do
+ completer = build_completer(
+ input: 'checkout "mas',
+ repo_heads: %w( master )
+ )
+
+ expect(completer.call('mas')).to include 'master'
+ end
+
+ it 'completes paths containing spaces' do
+ in_a_temporary_directory do
+ write_file('some text file.txt', "Some text\n")
+ completer = build_completer(input: 'add ')
+
+ expect(completer.call('som')).to include 'some\ text\ file.txt '
+ end
@houndci-bot

houndci-bot Mar 21, 2014

Line is too long. [82/80]

@georgebrock

georgebrock Apr 2, 2014

Owner

Go home, @houndci. You're drunk.

I think this actually referred to line 65, which I've wrapped to shorten it.

@mike-burns mike-burns commented on the diff Mar 31, 2014

lib/gitsh/completer.rb
tokens = full_input.split
tokens.any? && full_input.end_with?(' ') || tokens.size > 1
end
+ def completing_quoted_argument?
@mike-burns

mike-burns Mar 31, 2014

Owner

This method definition should be below #escape.

@georgebrock

georgebrock Apr 2, 2014

Owner

Indeed it should. Moved.

@georgebrock

georgebrock Jul 22, 2016

Owner

This could probably be more accurately implemented using rl_completion_found_quote, rl_completion_quote_character, or some combination of the two.

@mike-burns mike-burns and 1 other commented on an outdated diff Mar 31, 2014

lib/gitsh/completer.rb
+ @_quoted ||= input_before_current_argument.end_with?('"', "'")
+ end
+
+ def full_input
+ readline.line_buffer
+ end
+
+ def input_before_current_argument
+ full_input[0...-input.length]
+ end
+
+ def escape(arg)
+ if completing_quoted_argument?
+ arg.strip
+ else
+ arg.gsub(/ (.)/, '\ \1')
@mike-burns

mike-burns Mar 31, 2014

Owner
2.0.0p353 :001 > 'foo    bar'.gsub(/ (.)/, '\ \1')
 => "foo\\  \\  bar" 

Intended?

@georgebrock

georgebrock Apr 2, 2014

Owner

Definitely not intended, good catch. I believe the intention was to avoid escaping the space at the end of the completion, which is there to separate it from the next argument rather than being part of the current argument.

I've fixed this using a look ahead, which I don't like doing, but in this case it seems like the simplest approach:

arg.gsub(/ (?!$)/, '\ ')
Owner

georgebrock commented Apr 2, 2014

Thanks for the review, @mike-burns. I've made some changes, and would appreciate another review (especially of the slightly gnarly regular expression in Completer#escape)

@mike-burns mike-burns commented on the diff Apr 2, 2014

lib/gitsh/completer.rb
@@ -36,12 +36,31 @@ def available_completers
end
end
+ def escape(arg)
+ if completing_quoted_argument?
+ arg.strip
+ else
+ arg.gsub(/ (?!$)/, '\ ')
@mike-burns

mike-burns Apr 2, 2014

Owner

How do you want the system to respond to the user typing \ manually, such as add a\ textTAB?

This will also come up for partial matches, right? If the files names a text file and a text document exist, completing a will give the user a\ text\; typing dTAB from there will cause further completion off text with a \ in it.

I believe the above code will double-escape the \ in that case.

@georgebrock

georgebrock Apr 25, 2014

Owner

Good catch.

Unfortunately trying to fix it got really complicated really fast. Ruby's Readline module doesn't understand that escaped spaces ('\ ') aren't a break between arguments, so when it's given add a\ text to complete it assumes we are completing the word text and not the phrase a\ text. We have access to enough information to figure out that this is happening, and return ['a\ text\ file', 'a\ text\ document'] or ['\ file', '\ document'], but in either case there are circumstances where that's the wrong thing to do, and the Ruby bindings don't give us enough information to figure out which would be correct for the current situation.

What we really need to do is tell Readline how to determine if a character is escaped. Conveniently, we can do this using rl_char_is_quoted_p. Inconveniently, the Ruby bindings don't expose this method.

@mike-burns

mike-burns Apr 28, 2014

Owner

Does libedit have that function?

Should we write some C to expose this function to gitsh?

Alternatively we could write a small parser that's used only during tab completion.

@georgebrock

georgebrock May 1, 2014

Owner

I spent some time last Friday writing some C to expose that function, and in the process discovered that libedit either doesn't have the function or at least has a sufficiently different implementation that my code wouldn't build against it.

Writing a parser wouldn't solve the problem of Readline/libedit only replacing the portion it has determined is being completed. We could stop using the built in tab completion entirely, but we'd lose some support for various inputrc settings.

@mike-burns

mike-burns May 2, 2014

Owner

Is it possible to force a dependency on GNU Readline? During ./configure, check whether Ruby was built using Readline instead of libedit?

(Alternatively: does re-writing in another language solve this problem?)

@georgebrock

georgebrock May 2, 2014

Owner

I'm going to have another look at Ruby Readline implementations (#49) first.

The simplest other language approach would probably be a C extension to extend the built-in Ruby Readline module, or possibly to replace it entirely. I'm reticent to do a full rewrite, although with this, the process name thing (#41), and the sh-bang problems (#7), we're racking up quite a few things that we can't do in Ruby.

@georgebrock

georgebrock Jul 4, 2014

Owner

I've started integrating the custom Readline code on the gb-custom-readline-extension branch.

So far I've got a working C extension which creates an empty Gitsh::LineEditor module, which is properly configured and built by the ./configure && make dance. Running make install will install it globally for the selected Ruby version without doing any extra work, which means that other programs could theoretically require 'gitsh/line_editor, but that's probably fine.

All that remains now is to make it, y'know, do things, and to update the configure.ac or extconf.rb scripts to ensure we have access to all of the relevant line editing features.

@georgebrock georgebrock modified the milestone: 0.6, 0.7 Apr 5, 2014

georgebrock added the needs-c label May 16, 2014

@georgebrock georgebrock modified the milestone: 0.8, 0.7 Jun 13, 2014

georgebrock and others added some commits Feb 12, 2014

@georgebrock georgebrock Improve tab completion for files with spaces.
This involved some parser changes to accept escaped characters in non-quoted
arguments.

The tab completion isn't perfect: When the argument is prefixed with a
single quote, the completed version will still get escaped, but shouldn't be
because of the hard quotes.
45fcebc
@georgebrock georgebrock Integration spec covering quoted completion. 90cfc44
@georgebrock George Brocklehurst and Zamith For single quoted paths, don't escape spaces. 81ae074
@georgebrock George Brocklehurst and Zamith Don't escape spaces in double-quoted arguments. 1df51fb
@georgebrock George Brocklehurst and Zamith Memoize quoted method.
This will be called N+1 times for N paths.
d00bcfb
@georgebrock George Brocklehurst and Zamith Move integration spec to unit level. 990656b
@georgebrock George Brocklehurst and Zamith Shorten long lines (a little)
Hound is probably still going to hate this. Sorry, hound.
8451743
@georgebrock georgebrock Improve naming. 389fd06
@georgebrock georgebrock Extract method for complex string manipulation. 3fee17b
@georgebrock georgebrock Improve spec descriptions. 6a1f804
@georgebrock georgebrock No suffix on quoted head completions.
When completing a quoted head (e.g. `checkout "mas\t`) the resulting
completion shouldn't have a space suffix (`checkout "master"` not `checkout
"master "`)

This has the nice side effect of removing some repeated conditionals from
PathCompleter class.
ef0315c
@georgebrock georgebrock Use #flat_map to shorten a long line. 4346371
@georgebrock georgebrock Re-order methods to preserve dependency order. d4d7ea1
@georgebrock georgebrock Wrap long line. 5b20993
@georgebrock georgebrock Fix escaping on multiple spaces.
This uses a regular expression look ahead to avoid escaping a space at the
end of the completion. The final space is there to separate this argument
from the next argument, it's not part of the argument itself.
4ca8f3b
@georgebrock georgebrock More method order tweaking. 76ab8b1

@georgebrock georgebrock modified the milestone: 0.9, 0.8 Sep 22, 2014

Owner

mike-burns commented Oct 10, 2014

What's the status of this? Is there a part of it to review or discuss, or does it just need some hacking done?

Owner

georgebrock commented Oct 13, 2014

@mike-burns The code in this PR is an improvement, but a fairly incomplete and probably confusing one without the gb-custom-readline-extension branch. I'll close it in favour of #189.

georgebrock referenced this pull request Jun 15, 2016

Closed

Use custom Readline integration #255

13 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment