From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 06B9FDEF2 for ; Tue, 6 Dec 2022 12:49:51 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D6B9E2F0E8 for ; Tue, 6 Dec 2022 12:49:50 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 6 Dec 2022 12:49:49 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 22AFB44DE5 for ; Tue, 6 Dec 2022 12:49:49 +0100 (CET) Date: Tue, 6 Dec 2022 12:49:47 +0100 From: Wolfgang Bumiller To: Lukas Wagner Cc: pbs-devel@lists.proxmox.com Message-ID: <20221206114947.og2qtby7mbrkbbch@casey.proxmox.com> References: <20221205145514.645111-1-l.wagner@proxmox.com> <20221205145514.645111-4-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221205145514.645111-4-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.225 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [diff.rs, lib.rs] Subject: Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] debug cli: add colored output to `diff archive` command X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Dec 2022 11:49:51 -0000 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 > --- > 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, 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 { > + 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>, > 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, > + } > + } > (...)