Skip to content

Commit

Permalink
fix: assorted completion issues with ~ and vars (#199)
Browse files Browse the repository at this point in the history
* Propagate exit results from failed command substitution through to assignments that depend on it.
* Targeted compatibility fix for limited set of printf %q cases
* Return 1 from compgen when no completions were generated
* Only filtering completions based on the token-being-completed for action-generated completions, not for those coming from an invoked function
* Stop unconditionally expanding token-being-completed in non-compgen scenarios.
  • Loading branch information
reubeno authored Oct 12, 2024
1 parent 78b1d6f commit d6fa500
Show file tree
Hide file tree
Showing 21 changed files with 607 additions and 255 deletions.
1 change: 1 addition & 0 deletions .config/nextest.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
retries = 3
status-level = "slow"
slow-timeout = { period = "5s", terminate-after = 3, grace-period = "2s" }
fail-fast = false

[profile.default.junit]
path = "test-results.xml"
7 changes: 6 additions & 1 deletion brush-core/src/builtins/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,6 @@ impl builtins::Command for CompGenCommand {
context: commands::ExecutionContext<'_>,
) -> Result<crate::builtins::ExitCode, crate::error::Error> {
let spec = self.common_args.create_spec();

let token_to_complete = self.word.as_deref().unwrap_or_default();

// N.B. We basic-expand the token-to-be-completed first.
Expand Down Expand Up @@ -471,6 +470,12 @@ impl builtins::Command for CompGenCommand {

match result {
completion::Answer::Candidates(candidates, _options) => {
// We are expected to return 1 if there are no candidates, even if no errors
// occurred along the way.
if candidates.is_empty() {
return Ok(builtins::ExitCode::Custom(1));
}

for candidate in candidates {
writeln!(context.stdout(), "{candidate}")?;
}
Expand Down
2 changes: 1 addition & 1 deletion brush-core/src/builtins/echo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl builtins::Command for EchoCommand {

let (expanded_arg, keep_going) = escape::expand_backslash_escapes(
arg.as_str(),
escape::EscapeMode::EchoBuiltin,
escape::EscapeExpansionMode::EchoBuiltin,
)?;
s.push_str(&String::from_utf8_lossy(expanded_arg.as_slice()));

Expand Down
21 changes: 19 additions & 2 deletions brush-core/src/builtins/printf.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use clap::Parser;
use std::io::Write;

use crate::{builtins, commands, expansion};
use crate::{builtins, commands, escape, expansion};

/// Format a string.
#[derive(Parser)]
Expand Down Expand Up @@ -40,13 +40,30 @@ impl PrintfCommand {
&self,
context: &commands::ExecutionContext<'_>,
) -> Result<String, crate::error::Error> {
// Special-case common format string: "%s".
match self.format_and_args.as_slice() {
// Special-case common format string: "%s".
[fmt, arg] if fmt == "%s" => Ok(arg.clone()),
// Special-case invocation of printf with %q-based format string from bash-completion.
// It has hard-coded expectation of backslash-style escaping instead of quoting.
[fmt, arg] if fmt == "%q" || fmt == "~%q" => {
Ok(Self::evaluate_format_with_percent_q(None, arg))
}
[fmt, arg] if fmt == "~%q" => Ok(Self::evaluate_format_with_percent_q(Some("~"), arg)),
// Fallback to external command.
_ => self.evaluate_via_external_command(context),
}
}

fn evaluate_format_with_percent_q(prefix: Option<&str>, arg: &str) -> String {
let mut result = escape::quote_if_needed(arg, escape::QuoteMode::BackslashEscape);

if let Some(prefix) = prefix {
result.insert_str(0, prefix);
}

result
}

#[allow(clippy::unwrap_in_result)]
fn evaluate_via_external_command(
&self,
Expand Down
20 changes: 19 additions & 1 deletion brush-core/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use command_fds::{CommandFdExt, FdMapping};
use itertools::Itertools;

use crate::{
builtins, error,
builtins, error, escape,
interp::{self, Execute, ProcessGroupPolicy},
openfiles::{self, OpenFile, OpenFiles},
processes, sys, trace_categories, ExecutionParameters, ExecutionResult, Shell,
Expand Down Expand Up @@ -177,6 +177,24 @@ impl From<&String> for CommandArg {
}
}

impl CommandArg {
pub fn quote_for_tracing(&self) -> String {
match self {
CommandArg::String(s) => escape::quote_if_needed(s, escape::QuoteMode::Quote),
CommandArg::Assignment(a) => {
let mut s = a.name.to_string();
let op = if a.append { "+=" } else { "=" };
s.push_str(op);
s.push_str(&escape::quote_if_needed(
a.value.to_string().as_str(),
escape::QuoteMode::Quote,
));
s
}
}
}
}

#[allow(unused_variables)]
pub(crate) fn compose_std_command<S: AsRef<OsStr>>(
shell: &mut Shell,
Expand Down
Loading

0 comments on commit d6fa500

Please sign in to comment.