From df78d869750d0e6506040630a473e7e646cf3452 Mon Sep 17 00:00:00 2001 From: Daniel Hofstetter Date: Fri, 8 Apr 2022 07:45:36 +0200 Subject: [PATCH] df: fix incorrect whitespace between columns Fixes #3194 --- src/uu/df/src/columns.rs | 27 +++ src/uu/df/src/df.rs | 27 +-- src/uu/df/src/table.rs | 370 +++++++++++++++++++++++++++------------ tests/by-util/test_df.rs | 71 +++++--- 4 files changed, 335 insertions(+), 160 deletions(-) diff --git a/src/uu/df/src/columns.rs b/src/uu/df/src/columns.rs index 91c52ed2e4f..d4495177cb4 100644 --- a/src/uu/df/src/columns.rs +++ b/src/uu/df/src/columns.rs @@ -183,4 +183,31 @@ impl Column { _ => Err(()), } } + + /// Return the alignment of the specified column. + pub(crate) fn align(column: &Self) -> Alignment { + match column { + Self::Source | Self::Target | Self::File | Self::Fstype => Alignment::Left, + _ => Alignment::Right, + } + } + + /// Return the minimum width of the specified column. + pub(crate) fn min_width(column: &Self) -> usize { + match column { + // 14 = length of "Filesystem" plus 4 spaces + Self::Source => 14, + // the shortest headers have a length of 4 chars so we use that as the minimum width + _ => 4, + } + } +} + +/// A column's alignment. +/// +/// We define our own `Alignment` enum instead of using `std::fmt::Alignment` because df doesn't +/// have centered columns and hence a `Center` variant is not needed. +pub(crate) enum Alignment { + Left, + Right, } diff --git a/src/uu/df/src/df.rs b/src/uu/df/src/df.rs index a7520117dcd..24739e2c306 100644 --- a/src/uu/df/src/df.rs +++ b/src/uu/df/src/df.rs @@ -25,7 +25,7 @@ use std::path::Path; use crate::blocks::{block_size_from_matches, BlockSize}; use crate::columns::{Column, ColumnError}; use crate::filesystem::Filesystem; -use crate::table::{DisplayRow, Header, Row}; +use crate::table::Table; static ABOUT: &str = "Show information about the file system on which each FILE resides,\n\ or all file systems by default."; @@ -383,27 +383,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } }; - // The running total of filesystem sizes and usage. - // - // This accumulator is computed in case we need to display the - // total counts in the last row of the table. - let mut total = Row::new("total"); - - println!("{}", Header::new(&opt)); - for filesystem in filesystems { - // If the filesystem is not empty, or if the options require - // showing all filesystems, then print the data as a row in - // the output table. - if opt.show_all_fs || opt.show_listed_fs || filesystem.usage.blocks > 0 { - let row = Row::from(filesystem); - println!("{}", DisplayRow::new(&row, &opt)); - total += row; - } - } - if opt.show_total { - println!("{}", DisplayRow::new(&total, &opt)); + // This can happen if paths are given as command-line arguments + // but none of the paths exist. + if filesystems.is_empty() { + return Ok(()); } + println!("{}", Table::new(&opt, filesystems)); + Ok(()) } diff --git a/src/uu/df/src/table.rs b/src/uu/df/src/table.rs index 3e0ae853f88..844880b9912 100644 --- a/src/uu/df/src/table.rs +++ b/src/uu/df/src/table.rs @@ -5,13 +5,11 @@ // spell-checker:ignore tmpfs Pcent Itotal Iused Iavail Ipcent //! The filesystem usage data table. //! -//! A table comprises a header row ([`Header`]) and a collection of -//! data rows ([`Row`]), one per filesystem. To display a [`Row`], -//! combine it with [`Options`] in the [`DisplayRow`] struct; the -//! [`DisplayRow`] implements [`std::fmt::Display`]. +//! A table ([`Table`]) comprises a header row ([`Header`]) and a +//! collection of data rows ([`Row`]), one per filesystem. use number_prefix::NumberPrefix; -use crate::columns::Column; +use crate::columns::{Alignment, Column}; use crate::filesystem::Filesystem; use crate::{BlockSize, Options}; use uucore::fsext::{FsUsage, MountInfo}; @@ -189,14 +187,14 @@ impl From for Row { } } -/// A displayable wrapper around a [`Row`]. +/// A formatter for [`Row`]. /// -/// The `options` control how the information in the row gets displayed. -pub(crate) struct DisplayRow<'a> { +/// The `options` control how the information in the row gets formatted. +pub(crate) struct RowFormatter<'a> { /// The data in this row. row: &'a Row, - /// Options that control how to display the data. + /// Options that control how to format the data. options: &'a Options, // TODO We don't need all of the command-line options here. Some // of the command-line options indicate which rows to include or @@ -207,7 +205,7 @@ pub(crate) struct DisplayRow<'a> { // `df.rs` module. } -impl<'a> DisplayRow<'a> { +impl<'a> RowFormatter<'a> { /// Instantiate this struct. pub(crate) fn new(row: &'a Row, options: &'a Options) -> Self { Self { row, options } @@ -261,84 +259,163 @@ impl<'a> DisplayRow<'a> { Some(x) => format!("{:.0}%", (100.0 * x).ceil()), } } -} -impl fmt::Display for DisplayRow<'_> { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + /// Returns formatted row data. + fn get_values(&self) -> Vec { + let mut strings = Vec::new(); + for column in &self.options.columns { - match column { - Column::Source => write!(f, "{0: <16} ", self.row.fs_device)?, - Column::Size => write!(f, "{0: >12} ", self.scaled_bytes(self.row.bytes))?, - Column::Used => write!(f, "{0: >12} ", self.scaled_bytes(self.row.bytes_used))?, - Column::Avail => write!(f, "{0: >12} ", self.scaled_bytes(self.row.bytes_avail))?, - Column::Pcent => { - write!(f, "{0: >5} ", DisplayRow::percentage(self.row.bytes_usage))?; - } - Column::Target => write!(f, "{0: <16}", self.row.fs_mount)?, - Column::Itotal => write!(f, "{0: >12} ", self.scaled_inodes(self.row.inodes))?, - Column::Iused => write!(f, "{0: >12} ", self.scaled_inodes(self.row.inodes_used))?, - Column::Iavail => { - write!(f, "{0: >12} ", self.scaled_inodes(self.row.inodes_free))?; - } - Column::Ipcent => { - write!(f, "{0: >5} ", DisplayRow::percentage(self.row.inodes_usage))?; - } - Column::File => { - write!(f, "{0: <16}", self.row.file.as_ref().unwrap_or(&"-".into()))?; - } - Column::Fstype => write!(f, "{0: <5} ", self.row.fs_type)?, + let string = match column { + Column::Source => self.row.fs_device.to_string(), + Column::Size => self.scaled_bytes(self.row.bytes), + Column::Used => self.scaled_bytes(self.row.bytes_used), + Column::Avail => self.scaled_bytes(self.row.bytes_avail), + Column::Pcent => Self::percentage(self.row.bytes_usage), + + Column::Target => self.row.fs_mount.to_string(), + Column::Itotal => self.scaled_inodes(self.row.inodes), + Column::Iused => self.scaled_inodes(self.row.inodes_used), + Column::Iavail => self.scaled_inodes(self.row.inodes_free), + Column::Ipcent => Self::percentage(self.row.inodes_usage), + Column::File => self.row.file.as_ref().unwrap_or(&"-".into()).to_string(), + + Column::Fstype => self.row.fs_type.to_string(), #[cfg(target_os = "macos")] - Column::Capacity => write!( - f, - "{0: >12} ", - DisplayRow::percentage(self.row.bytes_capacity) - )?, - } + Column::Capacity => Self::percentage(self.row.bytes_capacity), + }; + + strings.push(string); } - Ok(()) + + strings } } -/// The header row. -/// -/// The `options` control which columns are displayed. -pub(crate) struct Header<'a> { - /// Options that control which columns are displayed. - options: &'a Options, +/// The data of the header row. +struct Header {} + +impl Header { + /// Return the headers for the specified columns. + /// + /// The `options` control which column headers are returned. + fn get_headers(options: &Options) -> Vec { + let mut headers = Vec::new(); + + for column in &options.columns { + let header = match column { + Column::Source => String::from("Filesystem"), + Column::Size => options.block_size.to_string(), + Column::Used => String::from("Used"), + Column::Avail => String::from("Available"), + Column::Pcent => String::from("Use%"), + Column::Target => String::from("Mounted on"), + Column::Itotal => String::from("Inodes"), + Column::Iused => String::from("IUsed"), + Column::Iavail => String::from("IFree"), + Column::Ipcent => String::from("IUse%"), + Column::File => String::from("File"), + Column::Fstype => String::from("Type"), + #[cfg(target_os = "macos")] + Column::Capacity => String::from("Capacity"), + }; + + headers.push(header); + } + + headers + } } -impl<'a> Header<'a> { - /// Instantiate this struct. - pub(crate) fn new(options: &'a Options) -> Self { - Self { options } +/// The output table. +pub(crate) struct Table { + alignments: Vec, + rows: Vec>, + widths: Vec, +} + +impl Table { + pub(crate) fn new(options: &Options, filesystems: Vec) -> Self { + let headers = Header::get_headers(options); + let mut widths: Vec<_> = options + .columns + .iter() + .enumerate() + .map(|(i, col)| Column::min_width(col).max(headers[i].len())) + .collect(); + + let mut rows = vec![headers]; + + // The running total of filesystem sizes and usage. + // + // This accumulator is computed in case we need to display the + // total counts in the last row of the table. + let mut total = Row::new("total"); + + for filesystem in filesystems { + // If the filesystem is not empty, or if the options require + // showing all filesystems, then print the data as a row in + // the output table. + if options.show_all_fs || options.show_listed_fs || filesystem.usage.blocks > 0 { + let row = Row::from(filesystem); + let fmt = RowFormatter::new(&row, options); + let values = fmt.get_values(); + total += row; + + for (i, value) in values.iter().enumerate() { + if value.len() > widths[i] { + widths[i] = value.len(); + } + } + + rows.push(values); + } + } + + if options.show_total { + let total_row = RowFormatter::new(&total, options); + rows.push(total_row.get_values()); + } + + Self { + rows, + widths, + alignments: Self::get_alignments(&options.columns), + } + } + + fn get_alignments(columns: &Vec) -> Vec { + let mut alignments = Vec::new(); + + for column in columns { + alignments.push(Column::align(column)); + } + + alignments } } -impl fmt::Display for Header<'_> { +impl fmt::Display for Table { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - for column in &self.options.columns { - match column { - Column::Source => write!(f, "{0: <16} ", "Filesystem")?, - // `Display` is implemented for `BlockSize`, but - // `Display` only works when formatting an object into - // an empty format, `{}`. So we use `format!()` first - // to create the string, then use `write!()` to align - // the string and pad with spaces. - Column::Size => write!(f, "{0: >12} ", format!("{}", self.options.block_size))?, - Column::Used => write!(f, "{0: >12} ", "Used")?, - Column::Avail => write!(f, "{0: >12} ", "Available")?, - Column::Pcent => write!(f, "{0: >5} ", "Use%")?, - Column::Target => write!(f, "{0: <16} ", "Mounted on")?, - Column::Itotal => write!(f, "{0: >12} ", "Inodes")?, - Column::Iused => write!(f, "{0: >12} ", "IUsed")?, - Column::Iavail => write!(f, "{0: >12} ", "IFree")?, - Column::Ipcent => write!(f, "{0: >5} ", "IUse%")?, - Column::File => write!(f, "{0: <16}", "File")?, - Column::Fstype => write!(f, "{0: <5} ", "Type")?, - #[cfg(target_os = "macos")] - Column::Capacity => write!(f, "{0: >12} ", "Capacity")?, + let mut row_iter = self.rows.iter().peekable(); + while let Some(row) = row_iter.next() { + let mut col_iter = row.iter().enumerate().peekable(); + while let Some((i, elem)) = col_iter.next() { + match self.alignments[i] { + Alignment::Left => write!(f, "{: write!(f, "{:>width$}", elem, width = self.widths[i])?, + } + + if col_iter.peek().is_some() { + // column separator + write!(f, " ")?; + } + } + + if row_iter.peek().is_some() { + writeln!(f)?; } } + Ok(()) } } @@ -347,7 +424,7 @@ impl fmt::Display for Header<'_> { mod tests { use crate::columns::Column; - use crate::table::{DisplayRow, Header, Row}; + use crate::table::{Header, Row, RowFormatter}; use crate::{BlockSize, Options}; const COLUMNS_WITH_FS_TYPE: [Column; 7] = [ @@ -369,76 +446,119 @@ mod tests { ]; #[test] - fn test_header_display() { + fn test_default_header() { let options = Default::default(); assert_eq!( - Header::new(&options).to_string(), - "Filesystem 1K-blocks Used Available Use% Mounted on " + Header::get_headers(&options), + vec!( + "Filesystem", + "1K-blocks", + "Used", + "Available", + "Use%", + "Mounted on" + ) ); } #[test] - fn test_header_display_fs_type() { + fn test_header_with_fs_type() { let options = Options { columns: COLUMNS_WITH_FS_TYPE.to_vec(), ..Default::default() }; assert_eq!( - Header::new(&options).to_string(), - "Filesystem Type 1K-blocks Used Available Use% Mounted on " + Header::get_headers(&options), + vec!( + "Filesystem", + "Type", + "1K-blocks", + "Used", + "Available", + "Use%", + "Mounted on" + ) ); } #[test] - fn test_header_display_inode() { + fn test_header_with_inodes() { let options = Options { columns: COLUMNS_WITH_INODES.to_vec(), ..Default::default() }; assert_eq!( - Header::new(&options).to_string(), - "Filesystem Inodes IUsed IFree IUse% Mounted on " + Header::get_headers(&options), + vec!( + "Filesystem", + "Inodes", + "IUsed", + "IFree", + "IUse%", + "Mounted on" + ) ); } #[test] - fn test_header_display_block_size_1024() { + fn test_header_with_block_size_1024() { let options = Options { block_size: BlockSize::Bytes(3 * 1024), ..Default::default() }; assert_eq!( - Header::new(&options).to_string(), - "Filesystem 3K-blocks Used Available Use% Mounted on " + Header::get_headers(&options), + vec!( + "Filesystem", + "3K-blocks", + "Used", + "Available", + "Use%", + "Mounted on" + ) ); } #[test] - fn test_header_display_human_readable_binary() { + fn test_header_with_human_readable_binary() { let options = Options { block_size: BlockSize::HumanReadableBinary, ..Default::default() }; assert_eq!( - Header::new(&options).to_string(), - "Filesystem Size Used Available Use% Mounted on " + Header::get_headers(&options), + vec!( + "Filesystem", + "Size", + "Used", + "Available", + "Use%", + "Mounted on" + ) ); } #[test] - fn test_header_display_human_readable_si() { + fn test_header_with_human_readable_si() { let options = Options { block_size: BlockSize::HumanReadableDecimal, ..Default::default() }; assert_eq!( - Header::new(&options).to_string(), - "Filesystem Size Used Available Use% Mounted on " + Header::get_headers(&options), + vec!( + "Filesystem", + "Size", + "Used", + "Available", + "Use%", + "Mounted on" + ) ); } #[test] - fn test_row_display() { + fn test_row_formatter() { let options = Options { block_size: BlockSize::Bytes(1), ..Default::default() @@ -462,14 +582,15 @@ mod tests { inodes_free: 8, inodes_usage: Some(0.2), }; + let fmt = RowFormatter::new(&row, &options); assert_eq!( - DisplayRow::new(&row, &options).to_string(), - "my_device 100 25 75 25% my_mount " + fmt.get_values(), + vec!("my_device", "100", "25", "75", "25%", "my_mount") ); } #[test] - fn test_row_display_fs_type() { + fn test_row_formatter_with_fs_type() { let options = Options { columns: COLUMNS_WITH_FS_TYPE.to_vec(), block_size: BlockSize::Bytes(1), @@ -494,14 +615,15 @@ mod tests { inodes_free: 8, inodes_usage: Some(0.2), }; + let fmt = RowFormatter::new(&row, &options); assert_eq!( - DisplayRow::new(&row, &options).to_string(), - "my_device my_type 100 25 75 25% my_mount " + fmt.get_values(), + vec!("my_device", "my_type", "100", "25", "75", "25%", "my_mount") ); } #[test] - fn test_row_display_inodes() { + fn test_row_formatter_with_inodes() { let options = Options { columns: COLUMNS_WITH_INODES.to_vec(), block_size: BlockSize::Bytes(1), @@ -526,14 +648,15 @@ mod tests { inodes_free: 8, inodes_usage: Some(0.2), }; + let fmt = RowFormatter::new(&row, &options); assert_eq!( - DisplayRow::new(&row, &options).to_string(), - "my_device 10 2 8 20% my_mount " + fmt.get_values(), + vec!("my_device", "10", "2", "8", "20%", "my_mount") ); } #[test] - fn test_row_display_bytes_and_inodes() { + fn test_row_formatter_with_bytes_and_inodes() { let options = Options { columns: vec![Column::Size, Column::Itotal], block_size: BlockSize::Bytes(100), @@ -558,14 +681,12 @@ mod tests { inodes_free: 8, inodes_usage: Some(0.2), }; - assert_eq!( - DisplayRow::new(&row, &options).to_string(), - " 1 10 " - ); + let fmt = RowFormatter::new(&row, &options); + assert_eq!(fmt.get_values(), vec!("1", "10")); } #[test] - fn test_row_display_human_readable_si() { + fn test_row_formatter_with_human_readable_si() { let options = Options { block_size: BlockSize::HumanReadableDecimal, columns: COLUMNS_WITH_FS_TYPE.to_vec(), @@ -590,14 +711,23 @@ mod tests { inodes_free: 8, inodes_usage: Some(0.2), }; + let fmt = RowFormatter::new(&row, &options); assert_eq!( - DisplayRow::new(&row, &options).to_string(), - "my_device my_type 4.0k 1.0k 3.0k 25% my_mount " + fmt.get_values(), + vec!( + "my_device", + "my_type", + "4.0k", + "1.0k", + "3.0k", + "25%", + "my_mount" + ) ); } #[test] - fn test_row_display_human_readable_binary() { + fn test_row_formatter_with_human_readable_binary() { let options = Options { block_size: BlockSize::HumanReadableBinary, columns: COLUMNS_WITH_FS_TYPE.to_vec(), @@ -622,14 +752,23 @@ mod tests { inodes_free: 8, inodes_usage: Some(0.2), }; + let fmt = RowFormatter::new(&row, &options); assert_eq!( - DisplayRow::new(&row, &options).to_string(), - "my_device my_type 4.0Ki 1.0Ki 3.0Ki 25% my_mount " + fmt.get_values(), + vec!( + "my_device", + "my_type", + "4.0Ki", + "1.0Ki", + "3.0Ki", + "25%", + "my_mount" + ) ); } #[test] - fn test_row_display_round_up_usage() { + fn test_row_formatter_with_round_up_usage() { let options = Options { block_size: BlockSize::Bytes(1), ..Default::default() @@ -653,9 +792,10 @@ mod tests { inodes_free: 8, inodes_usage: Some(0.2), }; + let fmt = RowFormatter::new(&row, &options); assert_eq!( - DisplayRow::new(&row, &options).to_string(), - "my_device 100 25 75 26% my_mount " + fmt.get_values(), + vec!("my_device", "100", "25", "75", "26%", "my_mount") ); } } diff --git a/tests/by-util/test_df.rs b/tests/by-util/test_df.rs index d91f1ac36dd..e8d1290906a 100644 --- a/tests/by-util/test_df.rs +++ b/tests/by-util/test_df.rs @@ -29,9 +29,26 @@ fn test_df_compatible_si() { #[test] fn test_df_output() { let expected = if cfg!(target_os = "macos") { - "Filesystem Size Used Available Capacity Use% Mounted on " + vec![ + "Filesystem", + "Size", + "Used", + "Available", + "Capacity", + "Use%", + "Mounted", + "on", + ] } else { - "Filesystem Size Used Available Use% Mounted on " + vec![ + "Filesystem", + "Size", + "Used", + "Available", + "Use%", + "Mounted", + "on", + ] }; let output = new_ucmd!() .arg("-H") @@ -39,6 +56,7 @@ fn test_df_output() { .succeeds() .stdout_move_str(); let actual = output.lines().take(1).collect::>()[0]; + let actual = actual.split_whitespace().collect::>(); assert_eq!(actual, expected); } @@ -253,23 +271,26 @@ fn test_block_size_1024() { assert_eq!(get_header(34 * 1024 * 1024 * 1024), "34G-blocks"); } -// TODO The spacing does not match GNU df. Also we need to remove -// trailing spaces from the heading row. #[test] fn test_output_selects_columns() { let output = new_ucmd!() .args(&["--output=source"]) .succeeds() .stdout_move_str(); - assert_eq!(output.lines().next().unwrap(), "Filesystem "); + assert_eq!(output.lines().next().unwrap().trim_end(), "Filesystem"); let output = new_ucmd!() .args(&["--output=source,target"]) .succeeds() .stdout_move_str(); assert_eq!( - output.lines().next().unwrap(), - "Filesystem Mounted on " + output + .lines() + .next() + .unwrap() + .split_whitespace() + .collect::>(), + vec!["Filesystem", "Mounted", "on"] ); let output = new_ucmd!() @@ -277,8 +298,13 @@ fn test_output_selects_columns() { .succeeds() .stdout_move_str(); assert_eq!( - output.lines().next().unwrap(), - "Filesystem Mounted on Used " + output + .lines() + .next() + .unwrap() + .split_whitespace() + .collect::>(), + vec!["Filesystem", "Mounted", "on", "Used"] ); } @@ -289,12 +315,16 @@ fn test_output_multiple_occurrences() { .succeeds() .stdout_move_str(); assert_eq!( - output.lines().next().unwrap(), - "Filesystem Mounted on " + output + .lines() + .next() + .unwrap() + .split_whitespace() + .collect::>(), + vec!["Filesystem", "Mounted", "on"] ); } -// TODO Fix the spacing. #[test] fn test_output_file_all_filesystems() { // When run with no positional arguments, `df` lets "-" represent @@ -304,13 +334,12 @@ fn test_output_file_all_filesystems() { .succeeds() .stdout_move_str(); let mut lines = output.lines(); - assert_eq!(lines.next().unwrap(), "File "); + assert_eq!(lines.next().unwrap(), "File"); for line in lines { - assert_eq!(line, "- "); + assert_eq!(line, "- "); } } -// TODO Fix the spacing. #[test] fn test_output_file_specific_files() { // Create three files. @@ -326,15 +355,7 @@ fn test_output_file_specific_files() { .succeeds() .stdout_move_str(); let actual: Vec<&str> = output.lines().collect(); - assert_eq!( - actual, - vec![ - "File ", - "a ", - "b ", - "c " - ] - ); + assert_eq!(actual, vec!["File", "a ", "b ", "c "]); } #[test] @@ -355,5 +376,5 @@ fn test_nonexistent_file() { .args(&["--output=file", "does-not-exist", "."]) .fails() .stderr_is("df: does-not-exist: No such file or directory\n") - .stdout_is("File \n. \n"); + .stdout_is("File\n. \n"); }