From d6607a4b9f8522a0a09f3017615a78c2e4f31354 Mon Sep 17 00:00:00 2001 From: Yunus Emre Ozen <94426948+ozenyunus35@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:00:57 +0300 Subject: [PATCH 1/3] join: take field lists given as separate operands after -o GNU join accepts the fields of -o as separate operands, as in `join -o 1.1 1.2 2.2 file1 file2`: when an operand comes on top of two pending ones right after -o, the oldest pending one is taken as more of the field list if it is one. uutils reported an extra operand instead. Do the same, taking only as many operands as needed to leave two files, so `join -o 1.1 1.2 2.2 f1` still tries to open 2.2. This fixes the o-sep-unquoted-spaces, o-sep-unquoted-tabs and o-sep-space-enoent cases of the GNU test tests/join/join.pl. --- src/uu/join/src/join.rs | 138 +++++++++++++++++++++++++++++-------- tests/by-util/test_join.rs | 27 ++++++++ 2 files changed, 136 insertions(+), 29 deletions(-) diff --git a/src/uu/join/src/join.rs b/src/uu/join/src/join.rs index 4c2e6d52ab7..dcc5608cb5d 100644 --- a/src/uu/join/src/join.rs +++ b/src/uu/join/src/join.rs @@ -6,6 +6,7 @@ // spell-checker:ignore (ToDO) autoformat FILENUM whitespaces nocheck memmem use clap::builder::ValueParser; +use clap::parser::ValueSource; use clap::{Arg, ArgAction, Command}; use memchr::{Memchr3, memchr_iter, memmem::Finder}; use std::cmp::Ordering; @@ -771,7 +772,11 @@ fn get_and_parse_field_number(matches: &clap::ArgMatches, key: &str) -> UResult< /// This function takes the matches from the command-line arguments, processes them, /// and returns a `Settings` struct that encapsulates the configuration for the program. #[allow(clippy::field_reassign_with_default)] -fn parse_settings(matches: &clap::ArgMatches, diag_args: Option<&[OsString]>) -> UResult { +fn parse_settings( + matches: &clap::ArgMatches, + operand_formats: Vec<(usize, Vec)>, + diag_args: Option<&[OsString]>, +) -> UResult { let keys = get_and_parse_field_number(matches, "j")?; let key1 = get_and_parse_field_number(matches, "1")?; let key2 = get_and_parse_field_number(matches, "2")?; @@ -794,11 +799,17 @@ fn parse_settings(matches: &clap::ArgMatches, diag_args: Option<&[OsString]>) -> let formats: Vec<&String> = formats.collect(); // GNU lets `-o` repeat and accumulates the fields. Auto mode applies only // when every value is exactly `auto`; otherwise each `auto` is ignored. - if formats.iter().all(|f| *f == "auto") { + if formats.iter().all(|f| *f == "auto") && operand_formats.is_empty() { settings.autoformat = true; } else { let mut specs = vec![]; - for format in formats { + let mut operand_formats = operand_formats.into_iter().peekable(); + let indices = matches.indices_of("o").into_iter().flatten(); + for (format, index) in formats.into_iter().zip(indices) { + // Field lists taken from the operands keep their place among the `-o` values. + while let Some((_, taken)) = operand_formats.next_if(|(i, _)| *i < index) { + specs.extend(taken); + } if format == "auto" { continue; } @@ -821,6 +832,7 @@ fn parse_settings(matches: &clap::ArgMatches, diag_args: Option<&[OsString]>) -> })?); } } + specs.extend(operand_formats.flat_map(|(_, taken)| taken)); settings.format = specs; } } @@ -846,6 +858,92 @@ fn parse_settings(matches: &clap::ArgMatches, diag_args: Option<&[OsString]>) -> Ok(settings) } +/// The two file operands, and the field lists taken from the operands as +/// part of `-o`, each with the place at which it was taken. +struct Operands<'a> { + file1: &'a OsString, + file2: &'a OsString, + formats: Vec<(usize, Vec)>, +} + +/// Like GNU join, when an operand comes on top of two pending ones right after +/// `-o`, the oldest pending operand is taken as more of the `-o` field list if +/// it is one, so that `join -o 1.1 2.2 file1 file2` works. +fn split_operands(matches: &clap::ArgMatches) -> UResult> { + const OPERANDS: [&str; 3] = ["file1", "file2", "extra"]; + + let mut operands: Vec<(usize, &OsString)> = OPERANDS + .iter() + .flat_map(|id| { + let indices = matches.indices_of(id).into_iter().flatten(); + indices.zip(matches.get_many::(id).into_iter().flatten()) + }) + .collect(); + operands.sort_by_key(|(index, _)| *index); + + let options: Vec<(usize, &str)> = matches + .ids() + .map(clap::Id::as_str) + .filter(|id| { + !OPERANDS.contains(id) && matches.value_source(id) == Some(ValueSource::CommandLine) + }) + .flat_map(|id| { + let indices = matches.indices_of(id).into_iter().flatten(); + indices.map(move |index| (index, id)) + }) + .collect(); + let follows_o = |index: usize| { + options + .iter() + .filter(|(option_index, _)| *option_index < index) + .max_by_key(|(option_index, _)| *option_index) + .is_some_and(|(_, id)| *id == "o") + }; + + let mut names: Vec<&OsString> = Vec::with_capacity(2); + let mut formats = vec![]; + for (index, name) in operands { + if names.len() == 2 { + match names[0].to_str().and_then(parse_field_list) { + Some(specs) if follows_o(index) => { + formats.push((index, specs)); + names.remove(0); + } + _ => { + return Err(UUsageError::new( + 1, + translate!("join-error-extra-operand", "operand" => name.quote()), + )); + } + } + } + names.push(name); + } + + match names[..] { + [file1, file2] => Ok(Operands { + file1, + file2, + formats, + }), + [file1] => Err(UUsageError::new( + 1, + translate!("join-error-missing-operand-after", "file" => file1.quote()), + )), + _ => Err(UUsageError::new( + 1, + translate!("join-error-missing-operand"), + )), + } +} + +/// Parses a `-o` field list, or returns `None` if `list` is not one. +fn parse_field_list(list: &str) -> Option> { + uucore::diagnostics::list_items(list, &[' ', ',', '\t']) + .map(|(item, _)| Spec::parse(item).ok()) + .collect() +} + #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { // The command line is kept for the caret in `-o` diagnostics, which needs @@ -860,32 +958,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { opts.alternate_handling = Some(AlternateHandling::Shifted); let _ = try_init_collator(opts); - let settings = parse_settings(&matches, diag_args.as_deref())?; - - let (file1, file2) = match ( - matches.get_one::("file1"), - matches.get_one::("file2"), - ) { - (Some(file1), Some(file2)) => (file1, file2), - (Some(file1), None) => { - return Err(UUsageError::new( - 1, - translate!("join-error-missing-operand-after", "file" => file1.quote()), - )); - } - _ => { - return Err(UUsageError::new( - 1, - translate!("join-error-missing-operand"), - )); - } - }; - if let Some(extra) = matches.get_one::("extra") { - return Err(UUsageError::new( - 1, - translate!("join-error-extra-operand", "operand" => extra.quote()), - )); - } + // Operand errors are reported after option errors, as before. + let mut operands = split_operands(&matches); + let operand_formats = operands + .as_mut() + .map(|operands| std::mem::take(&mut operands.formats)) + .unwrap_or_default(); + let settings = parse_settings(&matches, operand_formats, diag_args.as_deref())?; + let Operands { file1, file2, .. } = operands?; if file1 == "-" && file2 == "-" { return Err(USimpleError::new( diff --git a/tests/by-util/test_join.rs b/tests/by-util/test_join.rs index bb7c9d95aab..1bb50590f5c 100644 --- a/tests/by-util/test_join.rs +++ b/tests/by-util/test_join.rs @@ -861,3 +861,30 @@ fn test_operand_count() { .stderr_contains("join: extra operand 'fields_3.txt'\n") .stderr_contains("--help"); } + +#[test] +fn format_list_in_separate_operands() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + at.write("a", "1 a x\n2 b y\n"); + at.write("b", "1 d u\n2 e v\n"); + + // The fields of -o may be given as separate operands before the files. + ts.ucmd() + .args(&["-o", "1.1", "1.2", "1.3", "2.1", "2.2", "2.3", "a", "b"]) + .succeeds() + .stdout_only("1 a x 1 d u\n2 b y 2 e v\n"); + + // Only as many operands as needed to leave two files are taken as fields. + ts.ucmd() + .args(&["-o", "1.1", "1.2", "2.2", "a"]) + .fails_with_code(1) + .stderr_only("join: 2.2: No such file or directory\n"); + + // An operand that is not a field list is still an extra operand. + ts.ucmd() + .args(&["-o", "1.1", "a", "b", "c"]) + .fails_with_code(1) + .stderr_contains("join: extra operand 'c'\n"); +} From dbcb2ba2e82d1763406ba4de44fa0f349b7485df Mon Sep 17 00:00:00 2001 From: Yunus Emre Ozen <94426948+ozenyunus35@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:18:37 +0300 Subject: [PATCH 2/3] tests/join: don't depend on how the missing file name is quoted On Windows the name in the error is quoted ('2.2'), so check the parts of the message instead of the exact line. --- tests/by-util/test_join.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/by-util/test_join.rs b/tests/by-util/test_join.rs index 1bb50590f5c..d3d109bdb58 100644 --- a/tests/by-util/test_join.rs +++ b/tests/by-util/test_join.rs @@ -880,7 +880,9 @@ fn format_list_in_separate_operands() { ts.ucmd() .args(&["-o", "1.1", "1.2", "2.2", "a"]) .fails_with_code(1) - .stderr_only("join: 2.2: No such file or directory\n"); + .no_stdout() + .stderr_contains("2.2") + .stderr_contains("No such file or directory"); // An operand that is not a field list is still an extra operand. ts.ucmd() From 645b35e7c699a72ffece9e185687176c169f21ba Mon Sep 17 00:00:00 2001 From: Yunus Emre Ozen <94426948+ozenyunus35@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:09:17 +0300 Subject: [PATCH 3/3] join: take -o field operands the way GNU does Follow GNU's add_file_name: an operand that follows a -o field list (or another such operand) might be more of it, and the older pending one is taken when a third operand comes. This also accepts other options in between and keeps the fields in the order GNU adds them. --- src/uu/join/src/join.rs | 187 ++++++++++++++++++++----------------- tests/by-util/test_join.rs | 62 ++++++++++-- 2 files changed, 155 insertions(+), 94 deletions(-) diff --git a/src/uu/join/src/join.rs b/src/uu/join/src/join.rs index dcc5608cb5d..aa4b95120b5 100644 --- a/src/uu/join/src/join.rs +++ b/src/uu/join/src/join.rs @@ -774,7 +774,7 @@ fn get_and_parse_field_number(matches: &clap::ArgMatches, key: &str) -> UResult< #[allow(clippy::field_reassign_with_default)] fn parse_settings( matches: &clap::ArgMatches, - operand_formats: Vec<(usize, Vec)>, + formats: &[FieldList<'_>], diag_args: Option<&[OsString]>, ) -> UResult { let keys = get_and_parse_field_number(matches, "j")?; @@ -795,22 +795,28 @@ fn parse_settings( if let Some(value_os) = matches.get_one::("t") { settings.separator = parse_separator(value_os)?; } - if let Some(formats) = matches.get_many::("o") { - let formats: Vec<&String> = formats.collect(); + if !formats.is_empty() { // GNU lets `-o` repeat and accumulates the fields. Auto mode applies only // when every value is exactly `auto`; otherwise each `auto` is ignored. - if formats.iter().all(|f| *f == "auto") && operand_formats.is_empty() { + if formats + .iter() + .all(|format| matches!(format, FieldList::Option(format) if *format == "auto")) + { settings.autoformat = true; } else { let mut specs = vec![]; - let mut operand_formats = operand_formats.into_iter().peekable(); - let indices = matches.indices_of("o").into_iter().flatten(); - for (format, index) in formats.into_iter().zip(indices) { - // Field lists taken from the operands keep their place among the `-o` values. - while let Some((_, taken)) = operand_formats.next_if(|(i, _)| *i < index) { - specs.extend(taken); - } - if format == "auto" { + for format in formats { + let format = match format { + FieldList::Option(format) => *format, + FieldList::Operand(list) => { + let list = list.to_string_lossy(); + for (part, _) in uucore::diagnostics::list_items(&list, &[' ', ',', '\t']) { + specs.push(Spec::parse(part)?); + } + continue; + } + }; + if *format == "auto" { continue; } // `-o` has no long form. @@ -832,7 +838,6 @@ fn parse_settings( })?); } } - specs.extend(operand_formats.flat_map(|(_, taken)| taken)); settings.format = specs; } } @@ -858,90 +863,101 @@ fn parse_settings( Ok(settings) } -/// The two file operands, and the field lists taken from the operands as -/// part of `-o`, each with the place at which it was taken. +/// A `-o` field list, given as the option's value or as an operand. +enum FieldList<'a> { + Option(&'a String), + Operand(&'a OsString), +} + +/// The two file operands, and the `-o` field lists in the order GNU join adds them. struct Operands<'a> { - file1: &'a OsString, - file2: &'a OsString, - formats: Vec<(usize, Vec)>, + files: UResult<(&'a OsString, &'a OsString)>, + formats: Vec>, } -/// Like GNU join, when an operand comes on top of two pending ones right after -/// `-o`, the oldest pending operand is taken as more of the `-o` field list if -/// it is one, so that `join -o 1.1 2.2 file1 file2` works. -fn split_operands(matches: &clap::ArgMatches) -> UResult> { - const OPERANDS: [&str; 3] = ["file1", "file2", "extra"]; - - let mut operands: Vec<(usize, &OsString)> = OPERANDS - .iter() - .flat_map(|id| { - let indices = matches.indices_of(id).into_iter().flatten(); - indices.zip(matches.get_many::(id).into_iter().flatten()) - }) - .collect(); - operands.sort_by_key(|(index, _)| *index); - - let options: Vec<(usize, &str)> = matches - .ids() - .map(clap::Id::as_str) - .filter(|id| { - !OPERANDS.contains(id) && matches.value_source(id) == Some(ValueSource::CommandLine) - }) - .flat_map(|id| { - let indices = matches.indices_of(id).into_iter().flatten(); - indices.map(move |index| (index, id)) - }) - .collect(); - let follows_o = |index: usize| { - options - .iter() - .filter(|(option_index, _)| *option_index < index) - .max_by_key(|(option_index, _)| *option_index) - .is_some_and(|(_, id)| *id == "o") - }; +#[derive(Clone, Copy, PartialEq, Eq)] +enum OperandStatus { + MustBeOperand, + MightBeFieldList, +} + +/// Splits the operands like GNU join: an operand that directly follows a `-o` +/// field list, or another such operand, might be more of that list. When a +/// third operand comes, the older pending operand that might be a field list is +/// taken as one, so that `join -o 1.1 2.2 file1 file2` works. +fn split_operands(matches: &clap::ArgMatches) -> Operands<'_> { + enum Token<'a> { + Option(Option<&'a String>), + Operand(&'a OsString), + } - let mut names: Vec<&OsString> = Vec::with_capacity(2); + let mut tokens: Vec<(usize, Token<'_>)> = vec![]; + for id in matches.ids().map(clap::Id::as_str) { + if matches.value_source(id) != Some(ValueSource::CommandLine) { + continue; + } + let indices = matches.indices_of(id).into_iter().flatten(); + match id { + "file1" | "file2" | "extra" => { + let names = matches.get_many::(id).into_iter().flatten(); + tokens.extend(indices.zip(names.map(Token::Operand))); + } + "o" => { + let formats = matches.get_many::(id).into_iter().flatten(); + tokens.extend(indices.zip(formats.map(|format| Token::Option(Some(format))))); + } + _ => tokens.extend(indices.map(|index| (index, Token::Option(None)))), + } + } + tokens.sort_by_key(|(index, _)| *index); + + let mut status = OperandStatus::MustBeOperand; + let mut pending: Vec<(&OsString, OperandStatus)> = Vec::with_capacity(2); let mut formats = vec![]; - for (index, name) in operands { - if names.len() == 2 { - match names[0].to_str().and_then(parse_field_list) { - Some(specs) if follows_o(index) => { - formats.push((index, specs)); - names.remove(0); + let mut error = None; + for (_, token) in tokens { + match token { + Token::Option(format) => { + status = OperandStatus::MustBeOperand; + if let Some(format) = format { + formats.push(FieldList::Option(format)); + if format != "auto" { + status = OperandStatus::MightBeFieldList; + } } - _ => { - return Err(UUsageError::new( - 1, - translate!("join-error-extra-operand", "operand" => name.quote()), - )); + } + Token::Operand(name) => { + if pending.len() == 2 { + let taken = usize::from(pending[0].1 == OperandStatus::MustBeOperand); + if pending[taken].1 == OperandStatus::MustBeOperand { + error.get_or_insert_with(|| { + UUsageError::new( + 1, + translate!("join-error-extra-operand", "operand" => name.quote()), + ) + }); + continue; + } + formats.push(FieldList::Operand(pending.remove(taken).0)); } + pending.push((name, status)); } } - names.push(name); } - match names[..] { - [file1, file2] => Ok(Operands { - file1, - file2, - formats, - }), - [file1] => Err(UUsageError::new( + let files = match (error, &pending[..]) { + (Some(error), _) => Err(error), + (None, [(file1, _), (file2, _)]) => Ok((*file1, *file2)), + (None, [(file1, _)]) => Err(UUsageError::new( 1, translate!("join-error-missing-operand-after", "file" => file1.quote()), )), - _ => Err(UUsageError::new( + (None, _) => Err(UUsageError::new( 1, translate!("join-error-missing-operand"), )), - } -} - -/// Parses a `-o` field list, or returns `None` if `list` is not one. -fn parse_field_list(list: &str) -> Option> { - uucore::diagnostics::list_items(list, &[' ', ',', '\t']) - .map(|(item, _)| Spec::parse(item).ok()) - .collect() + }; + Operands { files, formats } } #[uucore::main] @@ -958,14 +974,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { opts.alternate_handling = Some(AlternateHandling::Shifted); let _ = try_init_collator(opts); - // Operand errors are reported after option errors, as before. - let mut operands = split_operands(&matches); - let operand_formats = operands - .as_mut() - .map(|operands| std::mem::take(&mut operands.formats)) - .unwrap_or_default(); - let settings = parse_settings(&matches, operand_formats, diag_args.as_deref())?; - let Operands { file1, file2, .. } = operands?; + let operands = split_operands(&matches); + let settings = parse_settings(&matches, &operands.formats, diag_args.as_deref())?; + let (file1, file2) = operands.files?; if file1 == "-" && file2 == "-" { return Err(USimpleError::new( diff --git a/tests/by-util/test_join.rs b/tests/by-util/test_join.rs index d3d109bdb58..8859589fd6f 100644 --- a/tests/by-util/test_join.rs +++ b/tests/by-util/test_join.rs @@ -863,7 +863,7 @@ fn test_operand_count() { } #[test] -fn format_list_in_separate_operands() { +fn test_format_list_in_separate_operands() { let ts = TestScenario::new(util_name!()); let at = &ts.fixtures; @@ -876,17 +876,67 @@ fn format_list_in_separate_operands() { .succeeds() .stdout_only("1 a x 1 d u\n2 b y 2 e v\n"); + // An operand that follows the -o fields is one too, even after a file. + ts.ucmd() + .args(&["a", "-o", "1.1", "1.2", "b"]) + .succeeds() + .stdout_only("1 a\n2 b\n"); + // Only as many operands as needed to leave two files are taken as fields. + #[cfg(not(windows))] + let missing = "join: 2.2: No such file or directory\n"; + #[cfg(windows)] + let missing = "join: '2.2': No such file or directory\n"; ts.ucmd() .args(&["-o", "1.1", "1.2", "2.2", "a"]) .fails_with_code(1) - .no_stdout() - .stderr_contains("2.2") - .stderr_contains("No such file or directory"); + .stderr_only(missing); - // An operand that is not a field list is still an extra operand. + // The operand taken as fields must be a field list, as with GNU join. ts.ucmd() .args(&["-o", "1.1", "a", "b", "c"]) .fails_with_code(1) - .stderr_contains("join: extra operand 'c'\n"); + .stderr_only("join: invalid file number in field spec: 'a'\n"); + + // An operand that doesn't follow -o is still an extra operand. + ts.ucmd() + .args(&["-o", "auto", "1.1", "a", "b"]) + .fails_with_code(1) + .stderr_contains("join: extra operand 'b'\n"); +} + +#[test] +fn test_format_list_operands_around_other_options() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + at.write("a", "1,a\n2,b\n"); + at.write("b", "1,d\n2,e\n"); + + // The field operand only has to follow -o, not to come right before the files. + ts.ucmd() + .args(&["-o", "1.1", "1.2", "-t", ",", "a", "b"]) + .succeeds() + .stdout_only("1,a\n2,b\n"); + + // `1.2` comes before -o, so the operand taken as fields is `a`. + ts.ucmd() + .args(&["-t", ",", "1.2", "-o", "1.1", "a", "b"]) + .fails_with_code(1) + .stderr_only("join: invalid file number in field spec: 'a'\n"); +} + +#[test] +fn test_format_list_operand_order() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + at.write("a", "1 a\n2 b\n"); + at.write("b", "1 d\n2 e\n"); + + // `2.2` is taken as fields when `b` comes, so before the later `-o 1.2`. + ts.ucmd() + .args(&["-o", "1.1", "2.2", "a", "b", "-o", "1.2"]) + .succeeds() + .stdout_only("1 d a\n2 e b\n"); }