From 96328ad9cadea3bc9d4a46be9fe57ba0a0602e6c Mon Sep 17 00:00:00 2001 From: DeveloperC Date: Wed, 12 Nov 2025 12:49:32 +0000 Subject: [PATCH] feat: add output CLI argument with enum for format control --- Cargo.lock | 32 +++++++++++ Cargo.toml | 3 + end-to-end-tests/features/steps/then.py | 5 +- src/commits/commit/mod.rs | 51 +++++++++++++---- src/commits/commit/tests/mod.rs | 52 ----------------- src/commits/mod.rs | 74 ++++++++++++++++++++----- src/linting_results/mod.rs | 58 +++++++++++++++++++ src/linting_results/pretty.rs | 66 ++++++++++++++++++++++ src/main.rs | 49 ++++++++++------ src/output.rs | 8 +++ 10 files changed, 301 insertions(+), 97 deletions(-) delete mode 100644 src/commits/commit/tests/mod.rs create mode 100644 src/linting_results/mod.rs create mode 100644 src/linting_results/pretty.rs create mode 100644 src/output.rs diff --git a/Cargo.lock b/Cargo.lock index 6cf7a89e..67e3d026 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11,6 +11,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "ansi_term" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d52a9bb7ec0cf484c551830a7ce27bd20d67eac647e1befb56b0be4ee39a55d2" +dependencies = [ + "winapi", +] + [[package]] name = "anstream" version = "0.6.15" @@ -127,6 +136,7 @@ checksum = "f46ad14479a25103f283c0f10005961cf086d8dc42205bb44c46ac563475dca6" name = "clean_git_history" version = "1.1.1" dependencies = [ + "ansi_term", "anyhow", "clap", "git2", @@ -612,6 +622,22 @@ version = "0.2.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" +[[package]] +name = "winapi" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" +dependencies = [ + "winapi-i686-pc-windows-gnu", + "winapi-x86_64-pc-windows-gnu", +] + +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" + [[package]] name = "winapi-util" version = "0.1.9" @@ -621,6 +647,12 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" + [[package]] name = "windows-sys" version = "0.52.0" diff --git a/Cargo.toml b/Cargo.toml index f236d923..56561c3d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,9 @@ clap = { version = "4.4.6", features = ["derive"] } log = "0.4.20" pretty_env_logger = "0.5.0" +# For fancy output. +ansi_term = "0.12.1" + # For error handling. anyhow = "1.0.89" diff --git a/end-to-end-tests/features/steps/then.py b/end-to-end-tests/features/steps/then.py index 6caaecf0..f193e55b 100644 --- a/end-to-end-tests/features/steps/then.py +++ b/end-to-end-tests/features/steps/then.py @@ -22,7 +22,10 @@ def assert_git_history_is_not_clean(context): result = execute_clean_git_history(context) # Then - assert_no_output(result) + # Output now goes to stdout instead of stderr + assert result.stdout != "", "Expected standard output to contain error information.\n" + \ + f"Standard output = {result.stdout.encode()}.\n" + assert_no_errors(result) assert_command_unsuccessful(result) return result diff --git a/src/commits/commit/mod.rs b/src/commits/commit/mod.rs index 318ac4d9..69895845 100644 --- a/src/commits/commit/mod.rs +++ b/src/commits/commit/mod.rs @@ -1,36 +1,63 @@ -use git2::{Oid, Repository}; +use crate::linting_results::CommitError; -pub(super) struct Commit { - commit_hash: git2::Oid, +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct Commit { + pub hash: String, + pub message: String, number_of_parents: usize, } impl Commit { - pub(super) fn from_git(repository: &Repository, oid: Oid) -> Result { - let commit = repository.find_commit(oid)?; + pub(super) fn from_git(commit: &git2::Commit) -> Commit { let number_of_parents = commit.parents().len(); + let message = match commit.message().map(|m| m.to_string()) { + Some(message) => { + trace!( + "Found the commit message {message:?} for the commit with the hash '{}'.", + commit.id() + ); + message + } + None => { + warn!( + "Can not find commit message for the commit with the hash '{}'.", + commit.id() + ); + String::new() + } + }; + debug!( "The commit with the hash '{}' has {:?} parents.", commit.id(), number_of_parents, ); - Ok(Commit { - commit_hash: commit.id(), + Commit { + hash: commit.id().to_string(), + message, number_of_parents, - }) + } } pub(super) fn is_merge_commit(&self) -> bool { let is_merge_commit = self.number_of_parents > 1; if is_merge_commit { - warn!("Commit {:?} is a merge commit.", self.commit_hash); + warn!("Commit {:?} is a merge commit.", self.hash); } is_merge_commit } -} -#[cfg(test)] -mod tests; + /// Lint this commit and return any linting errors found. + pub(crate) fn lint(&self) -> Vec { + let mut errors = Vec::new(); + + if self.is_merge_commit() { + errors.push(CommitError::MergeCommit); + } + + errors + } +} diff --git a/src/commits/commit/tests/mod.rs b/src/commits/commit/tests/mod.rs deleted file mode 100644 index cc935f8c..00000000 --- a/src/commits/commit/tests/mod.rs +++ /dev/null @@ -1,52 +0,0 @@ -use std::collections::VecDeque; - -use crate::commits::commit::Commit; -use crate::Commits; - -#[test] -fn test_commits_contain_merge_commits() { - // Given - let commits = Commits { - commits: VecDeque::from(vec![ - Commit { - commit_hash: git2::Oid::from_str("1").unwrap(), - number_of_parents: 1, - }, - Commit { - commit_hash: git2::Oid::from_str("2").unwrap(), - number_of_parents: 2, - }, - Commit { - commit_hash: git2::Oid::from_str("3").unwrap(), - number_of_parents: 2, - }, - ]), - }; - - // When/Then - assert!(commits.contains_merge_commits()); -} - -#[test] -fn test_commits_does_not_contain_merge_commits() { - // Given - let commits = Commits { - commits: VecDeque::from(vec![ - Commit { - commit_hash: git2::Oid::from_str("1").unwrap(), - number_of_parents: 1, - }, - Commit { - commit_hash: git2::Oid::from_str("2").unwrap(), - number_of_parents: 1, - }, - Commit { - commit_hash: git2::Oid::from_str("3").unwrap(), - number_of_parents: 1, - }, - ]), - }; - - // When/Then - assert!(!commits.contains_merge_commits()); -} diff --git a/src/commits/mod.rs b/src/commits/mod.rs index e3c1e0b3..acb5abdf 100644 --- a/src/commits/mod.rs +++ b/src/commits/mod.rs @@ -1,11 +1,14 @@ -use std::collections::VecDeque; +use std::collections::{HashMap, VecDeque}; use anyhow::{bail, Context, Result}; use git2::{Oid, Repository, Revwalk}; -use crate::commits::commit::Commit; +use crate::linting_results::{ + CommitError, CommitErrors, CommitsError, CommitsErrors, LintingResults, +}; -mod commit; +pub mod commit; +pub use commit::Commit; /// A representation of a range of commits within a Git repository, which can have various lints performed upon it after construction. pub struct Commits { @@ -20,14 +23,52 @@ impl Commits { get_commits_till_head_from_oid(repository, oid) } - /// A lint that can be performed on the range of commits, which returns true if any of the - /// commits are merge commits, i.e. has multiple parents. - pub fn contains_merge_commits(&self) -> bool { - self.commits.iter().any(|commit| commit.is_merge_commit()) - } + /// Lint all commits and return the linting results if any issues are found. + pub fn lint(&self, max_commits: Option) -> Option { + let mut commit_errors: HashMap> = HashMap::new(); + + // Check each commit for linting errors + for commit in self.commits.iter().cloned() { + let errors = commit.lint(); + + if !errors.is_empty() { + warn!( + "Found {} linting errors for the commit {:?}.", + errors.len(), + commit.hash + ); + commit_errors.insert(commit, errors); + } + } - pub fn len(&self) -> usize { - self.commits.len() + // Check for aggregate errors + let commits_errors = max_commits.and_then(|max| { + if self.commits.len() > max { + Some(vec![CommitsError::MaxCommitsExceeded { + max_commits: max, + actual_commits: self.commits.len(), + }]) + } else { + None + } + }); + + // Return None if no issues found, otherwise build LintingResults + match (&commit_errors.is_empty(), &commits_errors) { + (true, None) => None, + (true, Some(ce)) => Some(LintingResults { + commit_errors: None, + commits_errors: Some(CommitsErrors::new(ce.clone())), + }), + (false, None) => Some(LintingResults { + commit_errors: Some(CommitErrors::new(self.commits.clone(), commit_errors)), + commits_errors: None, + }), + (false, Some(ce)) => Some(LintingResults { + commit_errors: Some(CommitErrors::new(self.commits.clone(), commit_errors)), + commits_errors: Some(CommitsErrors::new(ce.clone())), + }), + } } } @@ -49,14 +90,17 @@ fn get_commits_till_head_from_oid( let revwalker = get_revwalker(repository, from_commit_hash)?; let mut commits = VecDeque::new(); - for commit in revwalker { - let oid = commit?; - - let commit = Commit::from_git(repository, oid) - .context(format!("Can not find a commit with the hash '{oid}'."))?; + for oid in revwalker { + let oid = oid?; + let commit = repository.find_commit(oid)?; + let commit = Commit::from_git(&commit); commits.push_front(commit); } + if commits.is_empty() { + bail!("No Git commits within the provided range."); + } + info!("Found {} commits within the provided range.", commits.len()); Ok(Commits { commits }) } diff --git a/src/linting_results/mod.rs b/src/linting_results/mod.rs new file mode 100644 index 00000000..c4f69f2d --- /dev/null +++ b/src/linting_results/mod.rs @@ -0,0 +1,58 @@ +use std::collections::{HashMap, VecDeque}; + +use crate::commits::commit::Commit; + +mod pretty; + +/// The representation of an error that an individual commit can have. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub enum CommitError { + /// Commit is a merge commit (has multiple parents). + MergeCommit, +} + +/// The representation of an error for the collection of commits as a whole. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CommitsError { + /// The number of commits exceeds the maximum allowed. + MaxCommitsExceeded { + max_commits: usize, + actual_commits: usize, + }, +} + +/// Per-commit linting errors. +pub struct CommitErrors { + pub(crate) order: VecDeque, + pub(crate) errors: HashMap>, +} + +impl CommitErrors { + pub(crate) fn new(order: VecDeque, errors: HashMap>) -> Self { + CommitErrors { order, errors } + } +} + +/// Aggregate linting errors for the commits collection. +pub struct CommitsErrors { + pub(crate) errors: Vec, +} + +impl CommitsErrors { + pub(crate) fn new(errors: Vec) -> Self { + CommitsErrors { errors } + } +} + +/// A representation of all linting errors found in the range of commits. +pub struct LintingResults { + pub commit_errors: Option, + pub commits_errors: Option, +} + +impl LintingResults { + /// Get a pretty representation of the linting results as a string, suitable as output for a user. + pub fn pretty(&self) -> String { + pretty::print_all(self) + } +} diff --git a/src/linting_results/pretty.rs b/src/linting_results/pretty.rs new file mode 100644 index 00000000..e45c04c8 --- /dev/null +++ b/src/linting_results/pretty.rs @@ -0,0 +1,66 @@ +use std::fmt::Write; + +use ansi_term::Colour::Red; + +use super::{CommitError, CommitsError, LintingResults}; + +pub(crate) fn print_all(results: &LintingResults) -> String { + let mut output = String::new(); + let red = Red.bold(); + + // Print per-commit errors + if let Some(commit_errors) = &results.commit_errors { + for commit in &commit_errors.order { + if let Some(errors) = commit_errors.errors.get(commit) { + let _ = writeln!(output, "{} - {}", red.paint("Commit Hash"), commit.hash); + let _ = writeln!(output, "{} - {:?}", red.paint("Message"), commit.message); + + for error in errors { + match error { + CommitError::MergeCommit => { + let _ = writeln!( + output, + "\t{} - Commit is a merge commit.", + red.paint("X") + ); + } + } + } + + let _ = writeln!(output); + } + } + + // Print summary of commit errors + let total_linting_errors: usize = commit_errors.errors.values().map(|x| x.len()).sum(); + + let _ = writeln!( + output, + "{} - Found {total_linting_errors} separate linting errors across {} commits.", + red.paint("X"), + commit_errors.errors.len() + ); + } + + // Print aggregate errors + if let Some(commits_errors) = &results.commits_errors { + for commits_error in &commits_errors.errors { + match commits_error { + CommitsError::MaxCommitsExceeded { + max_commits, + actual_commits, + } => { + let _ = writeln!( + output, + "{} - Maximum commits exceeded: found {} commits, but maximum allowed is {}.", + red.paint("X"), + actual_commits, + max_commits + ); + } + } + } + } + + output +} diff --git a/src/main.rs b/src/main.rs index ebaab67c..7ae3a48c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,13 +2,16 @@ extern crate log; extern crate pretty_env_logger; -use anyhow::{bail, Context, Result}; +use anyhow::{Context, Result}; use clap::Parser; use git2::Repository; mod commits; +mod linting_results; +mod output; use crate::commits::Commits; +use crate::output::Output; const ERROR_EXIT_CODE: i32 = 1; @@ -27,6 +30,13 @@ pub(crate) struct Arguments { )] pub(crate) verbose: bool, + #[arg( + long, + default_value = "default", + help = "Specifies the format for outputting results, acceptable values are quiet, default, and pretty." + )] + pub(crate) output: Output, + #[arg( help = "The Git reference from where to start taking the range of commits from till HEAD to lint. The range is inclusive of HEAD and exclusive of the provided reference.", default_value = "origin/HEAD" @@ -47,29 +57,34 @@ fn main() { info!("Version {}.", env!("CARGO_PKG_VERSION")); debug!("The command line arguments provided are {arguments:?}."); - if let Err(err) = run(arguments) { - error!("{err:?}"); - std::process::exit(ERROR_EXIT_CODE); + match run(arguments) { + Ok(exit_code) => std::process::exit(exit_code), + Err(err) => { + error!("{err:?}"); + std::process::exit(ERROR_EXIT_CODE); + } } } -fn run(arguments: Arguments) -> Result<()> { +fn run(arguments: Arguments) -> Result { let repository = Repository::open_from_env().context("Unable to open the Git repository.")?; let commits = Commits::from_git(&repository, arguments.from)?; - if commits.contains_merge_commits() { - bail!("Contains merge commits."); - } - - if let Some(max_commits) = arguments.max_commits { - if commits.len() > max_commits { - bail!(format!( - "Exceeded the maximum number of commits {:?} with {:?} commits.", - arguments.max_commits, - commits.len() - )); + if let Some(linting_results) = commits.lint(arguments.max_commits) { + match arguments.output { + Output::Quiet => {} + Output::Default => { + println!("{}", linting_results.pretty()); + } + Output::Pretty => { + println!("{}", linting_results.pretty()); + } } + + // As we don't want an error printed but linting failed so want to exit unsuccessfully. + return Ok(1); } - Ok(()) + info!("Successfully linted all commits."); + Ok(0) } diff --git a/src/output.rs b/src/output.rs new file mode 100644 index 00000000..7becdc8f --- /dev/null +++ b/src/output.rs @@ -0,0 +1,8 @@ +use clap::ValueEnum; + +#[derive(Clone, Debug, PartialEq, ValueEnum)] +pub enum Output { + Quiet, + Default, + Pretty, +}