refactor: rename Commit -> CommitId and remove 1 layer of indentation

nik-rev · nik-rev · commit 280268afbbcc · 2025-06-29T23:39:40.000+01:00
diff --git a/src/cli.rs b/src/cli.rs
@@ -7,7 +7,7 @@ use clap::{
 
 use crate::{
     commands,
-    config::{BranchName, Commit, PatchName, PrNumber, Remote},
+    config::{BranchName, CommitId, PatchName, PrNumber, Remote},
 };
 
 /// A tool which makes it easy to declaratively manage personal forks by automatically merging pull requests
@@ -35,7 +35,7 @@ pub enum Command {
     /// Generate a .patch file from a commit hash
     GenPatch {
         /// Transform this commit into a `.patch` file
-        commit: Commit,
+        commit: CommitId,
         /// Choose a custom file name for the `.patch` file
         #[arg(short, long)]
         filename: Option<PatchName>,
@@ -55,7 +55,7 @@ pub enum Command {
         branch: Option<BranchName>,
         /// When fetching this PR, reset to this commit
         #[arg(short = 'C', long)]
-        commit: Option<Commit>,
+        commit: Option<CommitId>,
         /// Check out the first fetched pull request
         #[arg(short, long)]
         checkout: bool,
@@ -68,7 +68,7 @@ pub enum Command {
         remote: Remote,
         /// When fetching this branch, reset to this commit
         #[arg(short = 'C', long)]
-        commit: Option<Commit>,
+        commit: Option<CommitId>,
         /// Check out the fetched branch
         #[arg(short, long)]
         checkout: bool,
@@ -131,7 +131,7 @@ pub struct Branch {
     /// Name of this branch in the remote
     pub name: String,
     /// When fetching this PR, reset to this commit
-    pub commit: Option<Commit>,
+    pub commit: Option<CommitId>,
 }
 
 #[cfg(test)]
@@ -159,7 +159,7 @@ mod test {
                 owner: RepoOwner::try_new("helix-editor").unwrap(),
                 repo: RepoName::try_new("helix").unwrap(),
                 branch: BranchName::try_new("master").unwrap(),
-                commit: Some(Commit::try_new("1a2b3c").unwrap())
+                commit: Some(CommitId::try_new("1a2b3c").unwrap())
             }
         );
         "helix-editor".parse::<Remote>().unwrap_err();
diff --git a/src/commands/branch_fetch.rs b/src/commands/branch_fetch.rs
@@ -2,14 +2,14 @@
 
 use colored::Colorize as _;
 
-use crate::config::{Commit, Remote};
+use crate::config::{CommitId, Remote};
 use crate::git::{fetch_branch, git};
 use anyhow::anyhow;
 
 /// Fetch the given branch
 pub async fn branch_fetch(
     remote: Remote,
-    commit: Option<Commit>,
+    commit: Option<CommitId>,
     checkout: bool,
 ) -> anyhow::Result<()> {
     let (_, info) = fetch_branch(&remote).await?;
diff --git a/src/commands/gen_patch.rs b/src/commands/gen_patch.rs
@@ -6,12 +6,12 @@ use std::path::PathBuf;
 use anyhow::bail;
 
 use crate::CONFIG_PATH;
-use crate::config::{Commit, PatchName};
+use crate::config::{CommitId, PatchName};
 use crate::git::git;
 use crate::utils::normalize_commit_msg;
 
 /// Generate patch `filename` at the given `Commit`
-pub fn gen_patch(commit: Commit, filename: Option<PatchName>) -> anyhow::Result<()> {
+pub fn gen_patch(commit: CommitId, filename: Option<PatchName>) -> anyhow::Result<()> {
     if !CONFIG_PATH.exists() {
         log::info!(
             "Config directory {} does not exist, creating it...",
diff --git a/src/commands/pr_fetch.rs b/src/commands/pr_fetch.rs
@@ -3,7 +3,7 @@
 use anyhow::{Context as _, anyhow};
 use colored::Colorize as _;
 
-use crate::config::{BranchName, Commit, PrNumber, Remote, RepoName, RepoOwner};
+use crate::config::{BranchName, CommitId, PrNumber, Remote, RepoName, RepoOwner};
 use crate::git::{fetch_pull_request, git};
 
 /// Fetch the given `pr` of `remote` at `commit` and store it in local `branch`
@@ -13,7 +13,7 @@ pub async fn pr_fetch(
     pr: PrNumber,
     remote: Option<Remote>,
     branch: Option<BranchName>,
-    commit: Option<Commit>,
+    commit: Option<CommitId>,
     checkout: bool,
 ) -> anyhow::Result<()> {
     pub const GITHUB_REMOTE_PREFIX: &str = "git@github.com:";
@@ -45,58 +45,53 @@ pub async fn pr_fetch(
         Ok,
     )?;
 
-    match fetch_pull_request(
+    let Ok((response, info)) = fetch_pull_request(
         &format!("{}/{}", remote.owner, remote.repo),
         // TODO: make fetch_pull_request accept a u32 instead
         pr,
         branch,
         commit.as_ref(),
     )
     .await
-    {
-        Ok((response, info)) => {
-            log::info!(
-                "Fetched pull request {} available at branch {}{}",
-                crate::utils::display_link(
-                    &format!(
-                        "{}{}{}{}",
-                        "#".bright_blue(),
-                        pr.to_string().bright_blue(),
-                        " ".bright_blue(),
-                        response.title.bright_blue().italic()
-                    ),
-                    &response.html_url
-                ),
-                info.branch.local_branch_name.as_ref().bright_cyan(),
-                commit
-                    .clone()
-                    .map(|commit_hash| {
-                        format!(", at commit {}", commit_hash.as_ref().bright_yellow())
-                    })
-                    .unwrap_or_default()
-            );
+    .inspect_err(|err| {
+        log::error!("{err}");
+    }) else {
+        return Ok(());
+    };
 
-            // Attempt to cleanup after ourselves
-            let _ = git(["remote", "remove", &info.remote.local_remote_alias]);
+    log::info!(
+        "Fetched pull request {} available at branch {}{}",
+        crate::utils::display_link(
+            &format!(
+                "{}{}{}{}",
+                "#".bright_blue(),
+                pr.to_string().bright_blue(),
+                " ".bright_blue(),
+                response.title.bright_blue().italic()
+            ),
+            &response.html_url
+        ),
+        info.branch.local_branch_name.as_ref().bright_cyan(),
+        commit
+            .clone()
+            .map(|commit_hash| { format!(", at commit {}", commit_hash.as_ref().bright_yellow()) })
+            .unwrap_or_default()
+    );
 
-            if checkout {
-                if let Err(cant_checkout) =
-                    git(["checkout", info.branch.local_branch_name.as_ref()])
-                {
-                    log::error!(
-                        "Could not check out branch {}:\n{cant_checkout}",
-                        info.branch.local_branch_name
-                    );
-                } else {
-                    log::info!(
-                        "Automatically checked out the first branch: {}",
-                        info.branch.local_branch_name
-                    );
-                }
-            }
-        }
-        Err(err) => {
-            log::error!("{err}");
+    // Attempt to cleanup after ourselves
+    let _ = git(["remote", "remove", &info.remote.local_remote_alias]);
+
+    if checkout {
+        if let Err(checkout_err) = git(["checkout", info.branch.local_branch_name.as_ref()]) {
+            log::error!(
+                "Could not check out branch {}:\n{checkout_err}",
+                info.branch.local_branch_name
+            );
+        } else {
+            log::info!(
+                "Automatically checked out the first branch: {}",
+                info.branch.local_branch_name
+            );
         }
     }
 
diff --git a/src/config.rs b/src/config.rs
@@ -40,7 +40,7 @@ pub struct Remote {
     /// e.g. `master`
     pub branch: BranchName,
     /// e.g. `1a2b3c`
-    pub commit: Option<Commit>,
+    pub commit: Option<CommitId>,
 }
 
 impl Remote {
@@ -104,7 +104,7 @@ pub struct PullRequest {
     /// Number of the pull request
     pub number: PrNumber,
     /// Commit to checkout of the pull request. If none, uses the latest commit
-    pub commit: Option<Commit>,
+    pub commit: Option<CommitId>,
 }
 
 impl FromStr for PullRequest {
@@ -132,7 +132,7 @@ pub struct Branch {
     /// Name of the branch
     pub name: BranchName,
     /// Commit to checkout when fetching this branch
-    pub commit: Option<Commit>,
+    pub commit: Option<CommitId>,
 }
 
 impl FromStr for Branch {
@@ -158,7 +158,7 @@ pub struct Ref {
     /// Git item. E.g. branch, or remote which may associate with the `commit`
     pub item: String,
     /// Commit to checkout of the `item`. If none, uses the latest commit
-    pub commit: Option<Commit>,
+    pub commit: Option<CommitId>,
 }
 
 impl FromStr for Ref {
@@ -180,7 +180,7 @@ impl FromStr for Ref {
         } else {
             // They want to use a specific commit
             let head: String = parts[0..len - 1].iter().map(|s| String::from(*s)).collect();
-            let commit = (parts[len - 1].to_owned()).parse::<Commit>().ok();
+            let commit = (parts[len - 1].to_owned()).parse::<CommitId>().ok();
             Self { item: head, commit }
         }
         .pipe(Ok)
@@ -259,12 +259,11 @@ impl Display for PatchName {
 }
 
 /// Represents a git commit hash
-// #[cfg_attr(test, nutype_test_util::derive(From))]
 #[nutype(
     validate(not_empty, predicate = is_valid_commit_hash),
-    derive(Debug, Eq, PartialEq, Ord, PartialOrd, Clone, AsRef, TryFrom)
+    derive(Debug, Eq, PartialEq, Ord, PartialOrd, Clone, AsRef, TryFrom, FromStr)
 )]
-pub struct Commit(String);
+pub struct CommitId(String);
 
 /// Does not check if the commit exists, just checks if it is potentially valid
 ///
@@ -273,17 +272,6 @@ pub fn is_valid_commit_hash(hash: &str) -> bool {
     hash.chars().all(|ch| ch.is_ascii_hexdigit())
 }
 
-impl FromStr for Commit {
-    type Err = String;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        Self::try_new(s).map_err(|err| match err {
-            CommitError::NotEmptyViolated => "commit cannot be empty".to_string(),
-            CommitError::PredicateViolated => format!("invalid commit hash: {s}"),
-        })
-    }
-}
-
 /// Implement `Deserialize` for these types, given that they have a `FromStr` impl
 // This is not a blanket impl as that would violate the orphan rule
 macro_rules! impl_deserialize_for {
diff --git a/src/git.rs b/src/git.rs
@@ -4,7 +4,7 @@
 //! - Extract into a separate module, put it behind some more nice API
 //! - Use `gix`? Or anyways, we could go without spawning an entire process each
 //!   time we want to interact with Git
-use crate::config::{BranchName, Commit, PrNumber};
+use crate::config::{BranchName, CommitId, PrNumber};
 use std::path::{Path, PathBuf};
 use std::process::{self, Output};
 use std::sync::LazyLock;
@@ -72,7 +72,7 @@ pub static CLIENT: LazyLock<Client> = LazyLock::new(|| *Box::new(Client::new()))
 
 /// Fetches a branch of a remote into local. Optionally accepts a commit hash
 /// for versioning.
-pub fn add_remote_branch(remote_branch: &RemoteBranch, commit: Option<&Commit>) -> Result<()> {
+pub fn add_remote_branch(remote_branch: &RemoteBranch, commit: Option<&CommitId>) -> Result<()> {
     if let Err(err) = git([
         "remote",
         "add",
@@ -347,7 +347,7 @@ pub async fn fetch_pull_request(
     repo: &str,
     pull_request: PrNumber,
     custom_branch_name: Option<BranchName>,
-    commit_hash: Option<&Commit>,
+    commit_hash: Option<&CommitId>,
 ) -> Result<(GitHubResponse, RemoteBranch)> {
     let url = format!("https://api.github.com/repos/{repo}/pulls/{pull_request}");
 
