Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output to `diff archive` command

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Dec 05, 2022 at 03:55:14PM +0100, Lukas Wagner wrote:
> This commit adds the `--color` flag to the `diff archive` tool.
> Valid values are `always`, `auto` and `never`. `always` and
> `never` should be self-explanatory, whereas `auto` will enable
> colors unless one of the following is true:
>   - STDOUT is not a tty
>   - TERM=dumb is set
>   - NO_COLOR is set
> 
> The tool will highlight changed file attributes in yellow.
> Furthermore, (A)dded files are highlighted in green,
> (M)odified in yellow and (D)eleted in red.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  Cargo.toml                           |   2 +
>  debian/control                       |   2 +
>  src/bin/proxmox_backup_debug/diff.rs | 372 ++++++++++++++++++++++-----
>  3 files changed, 310 insertions(+), 66 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 38e9c1f2..516dab37 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -42,6 +42,7 @@ path = "src/lib.rs"
>  
>  [dependencies]
>  apt-pkg-native = "0.3.2"
> +atty = "0.2.14"

Please drop this, it's kind of a weird crate. It should just take an
`&impl AsRawFd`, the enum thing doesn't provide any convenience and it's
just... `libc::isatty(1) == 1` for us...

>  base64 = "0.13"
>  bitflags = "1.2.1"
>  bytes = "1.0"
> @@ -73,6 +74,7 @@ serde = { version = "1.0", features = ["derive"] }
>  serde_json = "1.0"
>  siphasher = "0.3"
>  syslog = "4.0"
> +termcolor = "1.1.2"
>  tokio = { version = "1.6", features = [ "fs", "io-util", "io-std", "macros", "net", "parking_lot", "process", "rt", "rt-multi-thread", "signal", "time" ] }
>  tokio-openssl = "0.6.1"
>  tokio-stream = "0.1.0"
> diff --git a/debian/control b/debian/control
> index 6207d85e..3a27cb62 100644
> --- a/debian/control
> +++ b/debian/control

There's generally no need to bump d/control in patch series like this.
I'm auto-generating those ;-)

> @@ -7,6 +7,7 @@ Build-Depends: debhelper (>= 12),
>   rustc:native,
>   libstd-rust-dev,
>   librust-anyhow-1+default-dev,
> + librust-atty-0.2.14-dev ,
>   librust-apt-pkg-native-0.3+default-dev (>= 0.3.2-~~),
>   librust-base64-0.13+default-dev,
>   librust-bitflags-1+default-dev (>= 1.2.1-~~),
> @@ -94,6 +95,7 @@ Build-Depends: debhelper (>= 12),
>   librust-siphasher-0.3+default-dev,
>   librust-syslog-4+default-dev,
>   librust-tar-0.4+default-dev,
> + librust-termcolor-1.1.2-dev,
>   librust-thiserror-1+default-dev,
>   librust-tokio-1+default-dev (>= 1.6-~~),
>   librust-tokio-1+fs-dev (>= 1.6-~~),
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index b5ecd721..d2406ef5 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -1,5 +1,7 @@
>  use std::collections::{HashMap, HashSet};
> +use std::convert::{TryFrom, TryInto};
>  use std::ffi::{OsStr, OsString};
> +use std::io::Write;
>  use std::iter::FromIterator;
>  use std::path::{Path, PathBuf};
>  use std::sync::Arc;
> @@ -28,6 +30,8 @@ use pbs_tools::json::required_string_param;
>  use pxar::accessor::ReadAt;
>  use pxar::EntryKind;
>  use serde_json::Value;
> +
> +use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
>  use tokio::io::AsyncReadExt;
>  
>  type ChunkDigest = [u8; 32];
> @@ -87,6 +91,11 @@ pub fn diff_commands() -> CommandLineInterface {
>                  type: bool,
>                  description: "Compare file content rather than solely relying on mtime for detecting modified files.",
>              },
> +            "color": {
> +                optional: true,
> +                type: String,
> +                description: "Set mode for colored output. Can be `always`, `auto` or `never`. `auto` will display colors only if stdout is a tty. Defaults to `auto`."
> +            }

So for this I'd recommend just using

    type: ColorMode,

Make the 'fn take a

    color: Option<ColorMode>,

parameter...

>          }
>      }
>  )]
> @@ -106,6 +115,12 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
>          None => false,
>      };
>  
> +    let color = match param.get("color") {

... and let this just be: let color = color.unwrap_or_default();

> +        Some(Value::String(color)) => color.as_str().try_into()?,
> +        Some(_) => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
> +        None => ColorMode::Auto,
> +    };
> +
>      let namespace = match param.get("ns") {
>          Some(Value::String(ns)) => ns.parse()?,
>          Some(_) => bail!("invalid namespace parameter"),
> @@ -133,6 +148,8 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
>          namespace,
>      };
>  
> +    let output_params = OutputParams { color };
> +
>      if archive_name.ends_with(".pxar") {
>          let file_name = format!("{}.didx", archive_name);
>          diff_archive(
> @@ -141,6 +158,7 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
>              &file_name,
>              &repo_params,
>              compare_contents,
> +            &output_params,
>          )
>          .await?;
>      } else {
> @@ -156,6 +174,7 @@ async fn diff_archive(
>      file_name: &str,
>      repo_params: &RepoParams,
>      compare_contents: bool,
> +    output_params: &OutputParams,
>  ) -> Result<(), Error> {
>      let (index_a, accessor_a) = open_dynamic_index(snapshot_a, file_name, repo_params).await?;
>      let (index_b, accessor_b) = open_dynamic_index(snapshot_b, file_name, repo_params).await?;
> @@ -209,17 +228,40 @@ async fn diff_archive(
>      // which where *really* modified.
>      let modified_files = compare_files(potentially_modified, compare_contents).await?;
>  
> -    show_file_list(&added_files, &deleted_files, &modified_files);
> +    show_file_list(&added_files, &deleted_files, &modified_files, output_params)?;
>  
>      Ok(())
>  }
>  

You can turn this into an API type:

Add:

    #[api(default: "auto")]

(Yes, you still need `default` here even when using #[default], but the
api macro definitely should learn about the #[default] macrker.)

Put your `description` from above here as doc comment.

Add:
    #[derive(Default, Deserialize)]
    #[serde(rename_all = "lowercase")]

> +enum ColorMode {

Add doc comments to the variants

> +    Always,

Add `#[default]` here.

> +    Auto,
> +    Never,
> +}
> +
> +impl TryFrom<&str> for ColorMode {

^ This can be dropped completely then.
Also, for the future, I recommend implementing `FromStr` over `TryFrom`.

> +    type Error = Error;
> +
> +    fn try_from(value: &str) -> Result<Self, Self::Error> {
> +        match value {
> +            "auto" => Ok(Self::Auto),
> +            "always" => Ok(Self::Always),
> +            "never" => Ok(Self::Never),
> +            _ => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
> +        }
> +    }
> +}
> +
>  struct RepoParams {
>      repo: BackupRepository,
>      crypt_config: Option<Arc<CryptConfig>>,
>      namespace: BackupNamespace,
>  }
>  
> +struct OutputParams {
> +    color: ColorMode,
> +}
> +
>  async fn open_dynamic_index(
>      snapshot: &str,
>      archive_name: &str,
> @@ -530,70 +572,271 @@ impl ChangedProperties {
>      }
>  }
>  
> -fn change_indicator(changed: bool) -> &'static str {
> -    if changed {
> -        "*"
> -    } else {
> -        " "
> -    }
> +enum FileOperation {
> +    Added,
> +    Modified,
> +    Deleted,
>  }
>  
> -fn format_filesize(entry: &FileEntry, changed: bool) -> String {
> -    if let Some(size) = entry.file_size() {
> -        format!(
> -            "{}{:.1}",
> -            change_indicator(changed),
> -            HumanByte::new_decimal(size as f64)
> -        )
> -    } else {
> -        String::new()
> +struct ColumnWidths {
> +    operation: usize,
> +    entry_type: usize,
> +    uid: usize,
> +    gid: usize,
> +    mode: usize,
> +    filesize: usize,
> +    mtime: usize,
> +}
> +
> +impl Default for ColumnWidths {
> +    fn default() -> Self {
> +        Self {
> +            operation: 1,
> +            entry_type: 2,
> +            uid: 6,
> +            gid: 6,
> +            mode: 6,
> +            filesize: 10,
> +            mtime: 11,
> +        }
>      }
>  }
>  
> -fn format_mtime(entry: &FileEntry, changed: bool) -> String {
> -    let mtime = &entry.metadata().stat.mtime;
> +struct FileEntryPrinter {
> +    stream: StandardStream,
> +    column_widths: ColumnWidths,
> +    changed_color: Color,
> +}
>  
> -    let mut format = change_indicator(changed).to_owned();
> -    format.push_str("%F %T");
> +impl FileEntryPrinter {
> +    pub fn new(output_params: &OutputParams) -> Self {
> +        let color_choice = match output_params.color {
> +            ColorMode::Always => ColorChoice::Always,
> +            ColorMode::Auto => {
> +                if atty::is(atty::Stream::Stdout) {

^
Just use unsafe { libc::isatty(1) == 1 }

I know, "unsafe", booh, but that's exactly what it does ;-)

> +                    // Show colors unless `TERM=dumb` or `NO_COLOR` is set.
> +                    ColorChoice::Auto
> +                } else {
> +                    ColorChoice::Never
> +                }
> +            }
> +            ColorMode::Never => ColorChoice::Never,
> +        };
>  
> -    proxmox_time::strftime_local(&format, mtime.secs).unwrap_or_default()
> -}
> +        let stdout = StandardStream::stdout(color_choice);
>  
> -fn format_mode(entry: &FileEntry, changed: bool) -> String {
> -    let mode = entry.metadata().stat.mode & 0o7777;
> -    format!("{}{:o}", change_indicator(changed), mode)
> -}
> +        Self {
> +            stream: stdout,
> +            column_widths: ColumnWidths::default(),
> +            changed_color: Color::Yellow,
> +        }
> +    }
>  
(...)