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

Fix whitespace handling in command-mode completion #4587

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 81 additions & 17 deletions helix-core/src/shellwords.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use std::borrow::Cow;

/// Auto escape for shellwords usage.
pub fn escape(input: &str) -> Cow<'_, str> {
pub fn escape(input: Cow<str>) -> Cow<str> {
if !input.chars().any(|x| x.is_ascii_whitespace()) {
Cow::Borrowed(input)
input
} else if cfg!(unix) {
Cow::Owned(input.chars().fold(String::new(), |mut buf, c| {
if c.is_ascii_whitespace() {
Expand All @@ -17,18 +17,18 @@ pub fn escape(input: &str) -> Cow<'_, str> {
}
}

enum State {
OnWhitespace,
Unquoted,
UnquotedEscaped,
Quoted,
QuoteEscaped,
Dquoted,
DquoteEscaped,
}

/// Get the vec of escaped / quoted / doublequoted filenames from the input str
pub fn shellwords(input: &str) -> Vec<Cow<'_, str>> {
enum State {
OnWhitespace,
Unquoted,
UnquotedEscaped,
Quoted,
QuoteEscaped,
Dquoted,
DquoteEscaped,
}

use State::*;

let mut state = Unquoted;
Expand Down Expand Up @@ -140,6 +140,70 @@ pub fn shellwords(input: &str) -> Vec<Cow<'_, str>> {
args
}

/// Checks that the input ends with an ascii whitespace character which is
/// not escaped.
///
/// # Examples
///
/// ```rust
/// use helix_core::shellwords::ends_with_whitespace;
/// assert_eq!(ends_with_whitespace(" "), true);
/// assert_eq!(ends_with_whitespace(":open "), true);
/// assert_eq!(ends_with_whitespace(":open foo.txt "), true);
/// assert_eq!(ends_with_whitespace(":open"), false);
/// #[cfg(unix)]
/// assert_eq!(ends_with_whitespace(":open a\\ "), false);
/// #[cfg(unix)]
/// assert_eq!(ends_with_whitespace(":open a\\ b.txt"), false);
/// ```
pub fn ends_with_whitespace(input: &str) -> bool {
use State::*;

// Fast-lane: the input must end with a whitespace character
// regardless of quoting.
if !input.ends_with(|c: char| c.is_ascii_whitespace()) {
return false;
}

let mut state = Unquoted;

for c in input.chars() {
state = match state {
OnWhitespace => match c {
'"' => Dquoted,
'\'' => Quoted,
'\\' if cfg!(unix) => UnquotedEscaped,
'\\' => OnWhitespace,
c if c.is_ascii_whitespace() => OnWhitespace,
_ => Unquoted,
},
Unquoted => match c {
'\\' if cfg!(unix) => UnquotedEscaped,
'\\' => Unquoted,
c if c.is_ascii_whitespace() => OnWhitespace,
_ => Unquoted,
},
UnquotedEscaped => Unquoted,
Quoted => match c {
'\\' if cfg!(unix) => QuoteEscaped,
'\\' => Quoted,
'\'' => OnWhitespace,
_ => Quoted,
},
QuoteEscaped => Quoted,
Dquoted => match c {
'\\' if cfg!(unix) => DquoteEscaped,
'\\' => Dquoted,
'"' => OnWhitespace,
_ => Dquoted,
},
DquoteEscaped => Dquoted,
}
}

matches!(state, OnWhitespace)
}

#[cfg(test)]
mod test {
use super::*;
Expand Down Expand Up @@ -247,15 +311,15 @@ mod test {
#[test]
#[cfg(unix)]
fn test_escaping_unix() {
assert_eq!(escape("foobar"), Cow::Borrowed("foobar"));
assert_eq!(escape("foo bar"), Cow::Borrowed("foo\\ bar"));
assert_eq!(escape("foo\tbar"), Cow::Borrowed("foo\\\tbar"));
assert_eq!(escape("foobar".into()), Cow::Borrowed("foobar"));
assert_eq!(escape("foo bar".into()), Cow::Borrowed("foo\\ bar"));
assert_eq!(escape("foo\tbar".into()), Cow::Borrowed("foo\\\tbar"));
}

#[test]
#[cfg(windows)]
fn test_escaping_windows() {
assert_eq!(escape("foobar"), Cow::Borrowed("foobar"));
assert_eq!(escape("foo bar"), Cow::Borrowed("\"foo bar\""));
assert_eq!(escape("foobar".into()), Cow::Borrowed("foobar"));
assert_eq!(escape("foo bar".into()), Cow::Borrowed("\"foo bar\""));
}
}
24 changes: 17 additions & 7 deletions helix-term/src/commands/typed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2183,10 +2183,11 @@ pub(super) fn command_mode(cx: &mut Context) {
static FUZZY_MATCHER: Lazy<fuzzy_matcher::skim::SkimMatcherV2> =
Lazy::new(fuzzy_matcher::skim::SkimMatcherV2::default);

// simple heuristic: if there's no just one part, complete command name.
// if there's a space, per command completion kicks in.
// we use .this over split_whitespace() because we care about empty segments
if input.split(' ').count() <= 1 {
let parts = shellwords::shellwords(input);
let ends_with_whitespace = shellwords::ends_with_whitespace(input);

if parts.is_empty() || (parts.len() == 1 && !ends_with_whitespace) {
// If the command has not been finished yet, complete commands.
let mut matches: Vec<_> = typed::TYPABLE_COMMAND_LIST
.iter()
.filter_map(|command| {
Expand All @@ -2202,19 +2203,28 @@ pub(super) fn command_mode(cx: &mut Context) {
.map(|(name, _)| (0.., name.into()))
.collect()
} else {
let parts = shellwords::shellwords(input);
let part = parts.last().unwrap();
// Otherwise, use the command's completer and the last shellword
// as completion input.
let part = if parts.len() == 1 {
&Cow::Borrowed("")
} else {
parts.last().unwrap()
};

if let Some(typed::TypableCommand {
completer: Some(completer),
..
}) = typed::TYPABLE_COMMAND_MAP.get(&parts[0] as &str)
{
let part_len = shellwords::escape(part.clone()).len();

completer(editor, part)
.into_iter()
.map(|(range, file)| {
let file = shellwords::escape(file);

// offset ranges to input
let offset = input.len() - part.len();
let offset = input.len() - part_len;
let range = (range.start + offset)..;
(range, file)
})
Expand Down
6 changes: 1 addition & 5 deletions helix-term/src/ui/prompt.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crate::compositor::{Component, Compositor, Context, Event, EventResult};
use crate::{alt, ctrl, key, shift, ui};
use helix_core::shellwords;
use helix_view::input::KeyEvent;
use helix_view::keyboard::KeyCode;
use std::{borrow::Cow, ops::RangeFrom};
Expand Down Expand Up @@ -336,10 +335,7 @@ impl Prompt {

let (range, item) = &self.completion[index];

// since we are using shellwords to parse arguments, make sure
// that whitespace in files is properly escaped.
let item = shellwords::escape(item);
self.line.replace_range(range.clone(), &item);
Comment on lines -340 to -342
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this undo the previous PR? The idea was that it would complete paths with spaces in quotes

Copy link
Member Author

@the-mikedavis the-mikedavis Nov 5, 2022

Choose a reason for hiding this comment

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

This part moved into the map in command_mode: https://github.com/helix-editor/helix/pull/4587/files#diff-8f2b243886fc16c4a33efa5bd4ef54824301ed882ef62878ad9d4f6188d3d6a1R2227

The nice part of moving it there is that the completion results in the prompt show as escaped which matches how you have to input them (for example a file "a b.txt" would show as a\ b.txt in the prompt's completion items).

self.line.replace_range(range.clone(), item);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we pass the entire item instead of just the reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

item is already a &Cow<str> here. The old code was taking a reference because shellwords::escape returns a Cow<str>

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. LGTM


self.move_end();
}
Expand Down