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.

Sign up for GitHub

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

xargs: do not merge extra args before replacing #363

Merged
merged 4 commits into from
Jun 23, 2024
Merged

xargs: do not merge extra args before replacing #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ea237c
xargs: do not merge extra args before replacing
YDX-2147483647 Apr 28, 2024
fdde66f
xargs: split input only at newlines if `replace`
YDX-2147483647 Apr 28, 2024
8beab49
xargs: support args with conflicts
YDX-2147483647 Apr 28, 2024
9bc80cd
Merge branch 'main' into xargs-replace
YDX-2147483647 Jun 23, 2024
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
170 changes: 115 additions & 55 deletions src/xargs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,14 +406,9 @@
let mut command = Command::new(entry_point);

if let Some(replace_str) = &self.options.replace {
// we replace the first instance of the replacement string with
// the extra args, and then replace all instances of the replacement
let replacement = self
.extra_args
.iter()
.map(|s| s.to_string_lossy())
.collect::<Vec<_>>()
.join(" ");
// Replace all occurrences in initial args with the extra arg,
// Thanks to `MaxArgsCommandSizeLimiter`, we only process a single extra arg here.
let replacement = self.extra_args[0].to_string_lossy();
let initial_args: Vec<OsString> = initial_args
.iter()
.map(|arg| {
Expand Down Expand Up @@ -687,10 +682,33 @@
}
}

struct InputProcessOptions {
exit_if_pass_char_limit: bool,
max_args: Option<usize>,
max_lines: Option<usize>,
no_run_if_empty: bool,
}

impl InputProcessOptions {
fn new(
exit_if_pass_char_limit: bool,
max_args: Option<usize>,
max_lines: Option<usize>,
no_run_if_empty: bool,
) -> Self {
InputProcessOptions {
exit_if_pass_char_limit,
max_args,
max_lines,
no_run_if_empty,
}
}
}

fn process_input(
builder_options: CommandBuilderOptions,
mut args: Box<dyn ArgumentReader>,
options: &Options,
options: &InputProcessOptions,
) -> Result<CommandResult, XargsError> {
let mut current_builder = CommandBuilder::new(&builder_options);
let mut have_pending_command = false;
Expand Down Expand Up @@ -757,6 +775,66 @@
}
}

fn normalize_options<'a>(
options: &'a Options,
matches: &'a clap::ArgMatches,
) -> (Option<usize>, Option<usize>, &'a Option<String>, Option<u8>) {
let (max_args, max_lines, replace) =
match (options.max_args, options.max_lines, &options.replace) {
// These 3 options are mutually exclusive.
// But `max_args=1` and `replace` do not actually conflict, so no warning.
(None | Some(1), None, Some(_)) => {
// If `replace`, all matches in initial args should be replaced with extra args read from stdin.
// It is possible to have multiple matches and multiple extra args, and the Cartesian product is desired.
// To be specific, we process extra args one by one, and replace all matches with the same extra arg in each time.
(Some(1), None, &options.replace)
}
(Some(_), None, None) | (None, Some(_), None) | (None, None, None) => {
(options.max_args, options.max_lines, &None)
}
_ => {
eprintln!(
"WARNING: -L, -n and -I/-i are mutually exclusive, but more than one were given; \
only the last option will be used"
);
let lines_index = matches
.indices_of(options::MAX_LINES)
.and_then(|v| v.last());
let args_index = matches.indices_of(options::MAX_ARGS).and_then(|v| v.last());
let replace_index = [options::REPLACE, options::REPLACE_I]
.iter()
.flat_map(|o| matches.indices_of(o).and_then(|v| v.last()))
.max();
if lines_index > args_index && lines_index > replace_index {
(None, options.max_lines, &None)
} else if args_index > lines_index && args_index > replace_index {
(options.max_args, None, &None)
} else {
(Some(1), None, &options.replace)
}
}
};

let delimiter = match (options.delimiter, options.null) {
(Some(delimiter), true) => {
if matches.indices_of(options::NULL).unwrap().last()
> matches.indices_of(options::DELIMITER).unwrap().last()
{
Some(b'\0')
} else {
Some(delimiter)

Check warning on line 825 in src/xargs/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/xargs/mod.rs#L825 

Added line #L825 was not covered by tests
}
}
(Some(delimiter), false) => Some(delimiter),
(None, true) => Some(b'\0'),
// If `replace` and no delimiter specified, each line of stdin turns into a line of stdout,
// so the input should be split at newlines only.
(None, false) => replace.as_ref().map(|_| b'\n'),
};

(max_args, max_lines, replace, delimiter)
}

fn do_xargs(args: &[&str]) -> Result<CommandResult, XargsError> {
let matches = clap::Command::new("xargs")
.version(crate_version!())
Expand Down Expand Up @@ -797,7 +875,7 @@
.long(options::MAX_ARGS)
.help(
"Set the max number of arguments read from stdin to be passed \
to each command invocation (mutually exclusive with -L)",
to each command invocation (mutually exclusive with -L and -I/-i)",
)
.value_parser(validate_positive_usize),
)
Expand All @@ -807,7 +885,7 @@
.long(options::MAX_LINES)
.help(
"Set the max number of lines from stdin to be passed to each \
command invocation (mutually exclusive with -n)",
command invocation (mutually exclusive with -n and -I/-i)",
)
.value_parser(validate_positive_usize),
)
Expand Down Expand Up @@ -857,17 +935,18 @@
.require_equals(true)
.value_parser(clap::value_parser!(String))
.value_name("R")
.help(
"Replace R in INITIAL-ARGS with names read from standard input; \
if R is unspecified, assume {}",
),
.help("If R is specified, the same as -I R; otherwise, the same as -I {}"),
)
.arg(
Arg::new(options::REPLACE_I)
.short('I')
.num_args(1)
.help("same as --replace=R")
.value_name("R")
.help(
"Replace R in initial arguments with names read from standard input; \
also, the input is split at newlines only
(mutually exclusive with -L and -n)",
)
.overrides_with(options::REPLACE)
.value_parser(clap::value_parser!(String)),
)
Expand Down Expand Up @@ -901,20 +980,7 @@
verbose: matches.get_flag(options::VERBOSE),
};

let delimiter = match (options.delimiter, options.null) {
(Some(delimiter), true) => {
if matches.indices_of(options::NULL).unwrap().last()
> matches.indices_of(options::DELIMITER).unwrap().last()
{
Some(b'\0')
} else {
Some(delimiter)
}
}
(Some(delimiter), false) => Some(delimiter),
(None, true) => Some(b'\0'),
(None, false) => None,
};
let (max_args, max_lines, replace, delimiter) = normalize_options(&options, &matches);

let action = match matches.get_many::<OsString>(options::COMMAND) {
Some(args) if args.len() > 0 => {
Expand All @@ -925,36 +991,21 @@
let env = std::env::vars_os().collect();

let mut limiters = LimiterCollection::new();

match (options.max_args, options.max_lines) {
(Some(max_args), Some(max_lines)) => {
eprintln!(
"WARNING: Both --{} and -L were given; last option will be used",
options::MAX_ARGS,
);
if matches.indices_of(options::MAX_LINES).unwrap().last()
> matches.indices_of(options::MAX_ARGS).unwrap().last()
{
limiters.add(MaxLinesCommandSizeLimiter::new(max_lines));
} else {
limiters.add(MaxArgsCommandSizeLimiter::new(max_args));
}
}
(Some(max_args), None) => limiters.add(MaxArgsCommandSizeLimiter::new(max_args)),
(None, Some(max_lines)) => limiters.add(MaxLinesCommandSizeLimiter::new(max_lines)),
(None, None) => (),
if let Some(max_args) = max_args {
limiters.add(MaxArgsCommandSizeLimiter::new(max_args));
}
if let Some(max_lines) = max_lines {
limiters.add(MaxLinesCommandSizeLimiter::new(max_lines));
}

if let Some(max_chars) = options.max_chars {
limiters.add(MaxCharsCommandSizeLimiter::new(max_chars));
}

limiters.add(MaxCharsCommandSizeLimiter::new_system(&env));

let mut builder_options =
CommandBuilderOptions::new(action, env, limiters, options.replace.clone()).map_err(
|_| "Base command and environment are too large to fit into one command execution",
)?;
let mut builder_options = CommandBuilderOptions::new(action, env, limiters, replace.clone())
.map_err(|_| {

Check warning on line 1006 in src/xargs/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/xargs/mod.rs#L1006 

Added line #L1006 was not covered by tests
"Base command and environment are too large to fit into one command execution"
})?;

Check warning on line 1008 in src/xargs/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/xargs/mod.rs#L1008 

Added line #L1008 was not covered by tests

builder_options.verbose = options.verbose;
builder_options.close_stdin = options.arg_file.is_none();
Expand All @@ -971,7 +1022,16 @@
Box::new(WhitespaceDelimitedArgumentReader::new(args_file))
};

let result = process_input(builder_options, args, &options)?;
let result = process_input(
builder_options,
args,
&InputProcessOptions::new(
options.exit_if_pass_char_limit,
max_args,
max_lines,
options.no_run_if_empty,
),
)?;
Ok(result)
}

Expand Down
62 changes: 62 additions & 0 deletions tests/xargs_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,16 @@ fn xargs_max_args_lines_conflict() {
.stderr(predicate::str::contains("WARNING"))
.stdout(predicate::str::diff("ab cd\nef gh\ni\n"));

Command::cargo_bin("xargs")
.expect("found binary")
// -n2 is last, so it should be given priority.
.args(["-I=_", "-n2", "echo", "_"])
.write_stdin("ab cd ef\ngh i\njkl")
.assert()
.success()
.stderr(predicate::str::contains("WARNING"))
.stdout(predicate::str::diff("_ ab cd\n_ ef gh\n_ i jkl\n"));

Command::cargo_bin("xargs")
.expect("found binary")
// -L2 is last, so it should be given priority.
Expand All @@ -148,6 +158,28 @@ fn xargs_max_args_lines_conflict() {
.success()
.stderr(predicate::str::contains("WARNING"))
.stdout(predicate::str::diff("ab cd ef\ngh i jkl\n"));

Command::cargo_bin("xargs")
.expect("found binary")
// -L2 is last, so it should be given priority.
.args(["-I=_", "-L2", "echo", "_"])
.write_stdin("ab cd\nef\ngh i\n\njkl\n")
.assert()
.success()
.stderr(predicate::str::contains("WARNING"))
.stdout(predicate::str::diff("_ ab cd ef\n_ gh i jkl\n"));

for redundant_arg in ["-L2", "-n2"] {
Command::cargo_bin("xargs")
.expect("found binary")
// -I={} is last, so it should be given priority.
.args([redundant_arg, "-I={}", "echo", "{} bar"])
.write_stdin("ab cd ef\ngh i\njkl")
.assert()
.success()
.stderr(predicate::str::contains("WARNING"))
.stdout(predicate::str::diff("ab cd ef bar\ngh i bar\njkl bar\n"));
}
}

#[test]
Expand Down Expand Up @@ -502,3 +534,33 @@ fn xargs_replace() {
.failure()
.stderr(predicate::str::contains("Error: Command not found"));
}

#[test]
fn xargs_replace_multiple_lines() {
Command::cargo_bin("xargs")
.expect("found binary")
.args(["-I", "_", "echo", "[_]"])
.write_stdin("ab c\nd ef\ng")
.assert()
.success()
.stderr(predicate::str::is_empty())
.stdout(predicate::str::diff("[ab c]\n[d ef]\n[g]\n"));

Command::cargo_bin("xargs")
.expect("found binary")
.args(["-I", "{}", "echo", "{} {} foo"])
.write_stdin("bar\nbaz")
.assert()
.success()
.stderr(predicate::str::is_empty())
.stdout(predicate::str::diff("bar bar foo\nbaz baz foo\n"));

Command::cargo_bin("xargs")
.expect("found binary")
.args(["-I", "non-exist", "echo"])
.write_stdin("abc\ndef\ng")
.assert()
.success()
.stderr(predicate::str::is_empty())
.stdout(predicate::str::diff("\n\n\n"));
}
Loading