From ccd61e49345f686b73a13c70981759b4cd967a5c Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Wed, 21 Feb 2024 16:04:34 -0800 Subject: [PATCH] fix: Fix double-print in tree command. (#120) The prior logic unnecessarily recursed, in fact the underlying walker handled recursion correctly already, and the code only needed to handle skipping directories. This commit also cleans up and simplifies the run_* functions' code by pulling some things out into their own functions which report nice error messages. Signed-off-by: Andrew Lilley Brinker --- omnibor/Cargo.toml | 2 - omnibor/src/bin/omnibor.rs | 180 +++++++++++++++++++++---------------- 2 files changed, 103 insertions(+), 79 deletions(-) diff --git a/omnibor/Cargo.toml b/omnibor/Cargo.toml index 1db391d..8ed417e 100644 --- a/omnibor/Cargo.toml +++ b/omnibor/Cargo.toml @@ -21,7 +21,6 @@ url = "2.5.0" # Binary-only dependencies anyhow = { version = "1.0.80", optional = true } -async-recursion = { version = "1.0.5", optional = true } async-walkdir = { version = "1.0.0", optional = true } clap = { version = "4.5.1", features = ["derive"], optional = true } futures-lite = { version = "2.2.0", optional = true } @@ -34,7 +33,6 @@ tokio-test = "0.4.3" [features] build-binary = [ "dep:anyhow", - "dep:async-recursion", "dep:async-walkdir", "dep:clap", "dep:futures-lite", diff --git a/omnibor/src/bin/omnibor.rs b/omnibor/src/bin/omnibor.rs index fea49c9..8b7929d 100644 --- a/omnibor/src/bin/omnibor.rs +++ b/omnibor/src/bin/omnibor.rs @@ -2,12 +2,13 @@ use anyhow::anyhow; use anyhow::Context as _; use anyhow::Error; use anyhow::Result; -use async_recursion::async_recursion; +use async_walkdir::DirEntry as AsyncDirEntry; use async_walkdir::WalkDir; use clap::Args; use clap::Parser; use clap::Subcommand; use futures_lite::stream::StreamExt as _; +use omnibor::ArtifactId; use omnibor::Sha256; use serde_json::json; use std::default::Default; @@ -166,29 +167,20 @@ impl FromStr for SelectedHash { * Command Implementations *-------------------------------------------------------------------------*/ -/// Type alias for the specific ID we're using. -type ArtifactId = omnibor::ArtifactId; - /// Run the `id` subcommand. /// /// This command just produces the `gitoid` URL for the given file. fn run_id(args: &IdArgs) -> Result<()> { - let path = &args.path; - let file = File::open(path).with_context(|| format!("failed to open '{}'", path.display()))?; - - match args.hash { - SelectedHash::Sha256 => { - let id = ArtifactId::id_reader(&file).context("failed to produce Artifact ID")?; + let file = open_file(&args.path)?; + let url = match args.hash { + SelectedHash::Sha256 => sha256_id_file(&file, &args.path)?.url(), + }; - match args.format { - Format::Plain => { - println!("{}", id.url()); - } - Format::Json => { - let output = json!({ "id": id.url().to_string() }); - println!("{}", output); - } - } + match args.format { + Format::Plain => println!("{}", url), + Format::Json => { + let output = json!({ "id": url.to_string() }); + println!("{}", output); } } @@ -199,80 +191,114 @@ fn run_id(args: &IdArgs) -> Result<()> { /// /// This command produces the `gitoid` URL for all files in a directory tree. fn run_tree(args: &TreeArgs) -> Result<()> { - #[async_recursion] - async fn process_dir(path: &Path, format: Format, hash: SelectedHash) -> Result<()> { - let mut entries = WalkDir::new(path); - - loop { - match entries.next().await { - Some(Ok(entry)) => { - let path = &entry.path(); - - let file_type = entry.file_type().await.with_context(|| { - format!("unable to identify file type for '{}'", path.display()) - })?; - - if file_type.is_dir() { - process_dir(path, format, hash).await?; - continue; - } + let TreeArgs { path, format, hash } = args; + + Runtime::new() + .context("failed to initialize the async runtime")? + .block_on(async move { + let mut entries = WalkDir::new(path); + + loop { + match entries.next().await { + None => break, + Some(Err(e)) => print_error(e, *format), + Some(Ok(entry)) => { + let path = &entry.path(); + + if entry_is_dir(&entry).await? { + continue; + } - let mut file = AsyncFile::open(path) - .await - .with_context(|| format!("failed to open file '{}'", path.display()))?; - - match hash { - SelectedHash::Sha256 => { - let id = ArtifactId::id_async_reader(&mut file).await.with_context( - || { - format!( - "failed to produce Artifact ID for '{}'", - path.display() - ) - }, - )?; - - match format { - Format::Plain => println!("{} => {}", path.display(), id.url()), - Format::Json => { - let output = json!({ - "path": path.display().to_string(), - "id": id.url().to_string() - }); + let mut file = open_async_file(path).await?; - println!("{}", output); - } + // This 'match' is included to ensure this gets updated + // if we ever add a new hash algorithm. + let url = match *hash { + SelectedHash::Sha256 => { + sha256_id_async_file(&mut file, path).await?.url() } + }; + + match *format { + Format::Plain => println!("{} => {}", path.display(), url), + Format::Json => println!( + "{}", + json!({ + "path": path.display().to_string(), + "id": url.to_string() + }) + ), } } } - Some(Err(e)) => print_error(Error::from(e), format), - None => break, } - } - - Ok(()) - } - let runtime = Runtime::new().context("failed to initialize the async runtime")?; - runtime.block_on(process_dir(&args.path, args.format, args.hash)) + Ok(()) + }) } -/// Print an error, respecting formatting. -fn print_error(error: Error, format: Format) { - match format { - Format::Plain => print_plain_error(error), - Format::Json => { - let output = json!({ - "error": error.to_string(), - }); +/*=========================================================================== + * Helper Functions + *-------------------------------------------------------------------------*/ - eprintln!("{}", output); +/// Print an error, respecting formatting. +fn print_error>(error: E, format: Format) { + fn _print_error(error: Error, format: Format) { + match format { + Format::Plain => print_plain_error(error), + Format::Json => { + let output = json!({ + "error": error.to_string(), + }); + + eprintln!("{}", output); + } } } + + _print_error(error.into(), format) } /// Print an error in plain formatting. fn print_plain_error(error: Error) { eprintln!("error: {}", error); } + +/// Check if the entry is for a directory. +async fn entry_is_dir(entry: &AsyncDirEntry) -> Result { + entry + .file_type() + .await + .with_context(|| { + format!( + "unable to identify file type for '{}'", + entry.path().display() + ) + }) + .map(|file_type| file_type.is_dir()) +} + +/// Open a file. +fn open_file(path: &Path) -> Result { + File::open(path).with_context(|| format!("failed to open '{}'", path.display())) +} + +/// Open an asynchronous file. +async fn open_async_file(path: &Path) -> Result { + AsyncFile::open(path) + .await + .with_context(|| format!("failed to open file '{}'", path.display())) +} + +/// Identify a file using a SHA-256 hash. +fn sha256_id_file(file: &File, path: &Path) -> Result> { + ArtifactId::id_reader(file) + .with_context(|| format!("failed to produce Artifact ID for '{}'", path.display())) +} + +/// Identify a file using a SHA-256 hash. +async fn sha256_id_async_file(file: &mut AsyncFile, path: &Path) -> Result> { + ArtifactId::id_async_reader(file) + .await + .with_context(|| format!("failed to produce Artifact ID for '{}'", path.display())) +}