Merge pull request #3322 from jfinkels/df-multiple-columns-error

df: error on duplicate columns in --output arg

src/uu/df/src/columns.rs

@@ -55,19 +55,31 @@ pub(crate) enum Column {
     Capacity,
 }
 
+/// An error while defining which columns to display in the output table.
+#[derive(Debug)]
+pub(crate) enum ColumnError {
+    /// If a column appears more than once in the `--output` argument.
+    MultipleColumns(String),
+}
+
 impl Column {
     /// Convert from command-line arguments to sequence of columns.
     ///
     /// The set of columns that will appear in the output table can be
     /// specified by command-line arguments. This function converts
     /// those arguments to a [`Vec`] of [`Column`] variants.
-    pub(crate) fn from_matches(matches: &ArgMatches) -> Vec<Self> {
+    ///
+    /// # Errors
+    ///
+    /// This function returns an error if a column is specified more
+    /// than once in the command-line argument.
+    pub(crate) fn from_matches(matches: &ArgMatches) -> Result<Vec<Self>, ColumnError> {
         match (
             matches.is_present(OPT_PRINT_TYPE),
             matches.is_present(OPT_INODES),
             matches.occurrences_of(OPT_OUTPUT) > 0,
         ) {
-            (false, false, false) => vec![
+            (false, false, false) => Ok(vec![
                 Self::Source,
                 Self::Size,
                 Self::Used,
@@ -76,29 +88,37 @@ impl Column {
                 Self::Capacity,
                 Self::Pcent,
                 Self::Target,
-            ],
+            ]),
             (false, false, true) => {
-                matches
+                // Unwrapping should not panic because in this arm of
-                    .values_of(OPT_OUTPUT)
+                // the `match` statement, we know that `OPT_OUTPUT`
-                    .unwrap()
+                // is non-empty.
-                    .map(|s| {
+                let names = matches.values_of(OPT_OUTPUT).unwrap();
-                        // Unwrapping here should not panic because the
+                let mut seen: Vec<&str> = vec![];
-                        // command-line argument parsing library should be
+                let mut columns = vec![];
-                        // responsible for ensuring each comma-separated
+                for name in names {
-                        // string is a valid column label.
+                    if seen.contains(&name) {
-                        Self::parse(s).unwrap()
+                        return Err(ColumnError::MultipleColumns(name.to_string()));
-                    })
+                    }
-                    .collect()
+                    seen.push(name);
+                    // Unwrapping here should not panic because the
+                    // command-line argument parsing library should be
+                    // responsible for ensuring each comma-separated
+                    // string is a valid column label.
+                    let column = Self::parse(name).unwrap();
+                    columns.push(column);
+                }
+                Ok(columns)
             }
-            (false, true, false) => vec![
+            (false, true, false) => Ok(vec![
                 Self::Source,
                 Self::Itotal,
                 Self::Iused,
                 Self::Iavail,
                 Self::Ipcent,
                 Self::Target,
-            ],
+            ]),
-            (true, false, false) => vec![
+            (true, false, false) => Ok(vec![
                 Self::Source,
                 Self::Fstype,
                 Self::Size,
@@ -108,8 +128,8 @@ impl Column {
                 Self::Capacity,
                 Self::Pcent,
                 Self::Target,
-            ],
+            ]),
-            (true, true, false) => vec![
+            (true, true, false) => Ok(vec![
                 Self::Source,
                 Self::Fstype,
                 Self::Itotal,
@@ -117,7 +137,7 @@ impl Column {
                 Self::Iavail,
                 Self::Ipcent,
                 Self::Target,
-            ],
+            ]),
             // The command-line arguments -T and -i are each mutually
             // exclusive with --output, so the command-line argument
             // parser should reject those combinations before we get

src/uu/df/src/df.rs

@@ -11,17 +11,19 @@ mod columns;
 mod filesystem;
 mod table;
 
-use uucore::error::{UResult, USimpleError};
+use uucore::display::Quotable;
+use uucore::error::{UError, UResult};
 use uucore::format_usage;
 use uucore::fsext::{read_fs_list, MountInfo};
 
 use clap::{crate_version, Arg, ArgMatches, Command};
 
+use std::error::Error;
 use std::fmt;
 use std::path::Path;
 
 use crate::blocks::{block_size_from_matches, BlockSize};
-use crate::columns::Column;
+use crate::columns::{Column, ColumnError};
 use crate::filesystem::Filesystem;
 use crate::table::{DisplayRow, Header, Row};
 
@@ -103,8 +105,12 @@ impl Default for Options {
     }
 }
 
+#[derive(Debug)]
 enum OptionsError {
     InvalidBlockSize,
+
+    /// An error getting the columns to display in the output table.
+    ColumnError(ColumnError),
 }
 
 impl fmt::Display for OptionsError {
@@ -115,6 +121,11 @@ impl fmt::Display for OptionsError {
             // TODO This needs to vary based on whether `--block-size`
             // or `-B` were provided.
             Self::InvalidBlockSize => write!(f, "invalid --block-size argument"),
+            Self::ColumnError(ColumnError::MultipleColumns(s)) => write!(
+                f,
+                "option --output: field {} used more than once",
+                s.quote()
+            ),
         }
     }
 }
@@ -131,7 +142,7 @@ impl Options {
             include: matches.values_of_lossy(OPT_TYPE),
             exclude: matches.values_of_lossy(OPT_EXCLUDE_TYPE),
             show_total: matches.is_present(OPT_TOTAL),
-            columns: Column::from_matches(matches),
+            columns: Column::from_matches(matches).map_err(OptionsError::ColumnError)?,
         })
     }
 }
@@ -273,6 +284,28 @@ where
         .collect()
 }
 
+#[derive(Debug)]
+enum DfError {
+    /// A problem while parsing command-line options.
+    OptionsError(OptionsError),
+}
+
+impl Error for DfError {}
+
+impl UError for DfError {
+    fn usage(&self) -> bool {
+        matches!(self, Self::OptionsError(OptionsError::ColumnError(_)))
+    }
+}
+
+impl fmt::Display for DfError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Self::OptionsError(e) => e.fmt(f),
+        }
+    }
+}
+
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let matches = uu_app().get_matches_from(args);
@@ -284,7 +317,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         }
     }
 
-    let opt = Options::from(&matches).map_err(|e| USimpleError::new(1, format!("{}", e)))?;
+    let opt = Options::from(&matches).map_err(DfError::OptionsError)?;
 
     // Get the list of filesystems to display in the output table.
     let filesystems: Vec<Filesystem> = match matches.values_of(OPT_PATHS) {

tests/by-util/test_df.rs

@@ -272,3 +272,11 @@ fn test_output_file_specific_files() {
         ]
     );
 }
+
+#[test]
+fn test_output_field_no_more_than_once() {
+    new_ucmd!()
+        .arg("--output=target,source,target")
+        .fails()
+        .usage_error("option --output: field 'target' used more than once");
+}