[pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output to `diff archive` command
Wolfgang Bumiller
w.bumiller at proxmox.com
Tue Dec 6 12:49:47 CET 2022
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 at 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,
> + }
> + }
>
(...)
More information about the pbs-devel
mailing list