No quarter! Refactor! #refactor

Co-authored-by: Eyal Kalderon <[email protected]>

src/active_copirate.rs

@@ -0,0 +1,41 @@
+use std::fs;
+use std::io::prelude::*;
+use crate::BoxResult;
+use std::path::Path;
+use crate::copirate::CoPirate;
+use std::fs::OpenOptions;
+
+const ACTIVE_COPIRATES_PATH: &str = ".git/.git-rmob-template";
+
+#[derive(Debug)]
+pub struct ActiveCoPirates {
+    file: fs::File,
+}
+
+impl ActiveCoPirates {
+    pub fn create_empty(repo_dir: &Path) -> BoxResult<ActiveCoPirates> {
+        let active_copirates_path = repo_dir.join(ACTIVE_COPIRATES_PATH);
+        fs::write(&active_copirates_path, "")?;
+
+        let file = OpenOptions::new()
+            .append(true)
+            .open(active_copirates_path)?;
+
+        Ok(ActiveCoPirates { file })
+    }
+
+    pub fn save(mut self, copirates: &[&CoPirate]) -> BoxResult<()> {
+        for pirate in copirates {
+            writeln!(self.file, "Co-authored-by: {} <{}>", pirate.name, pirate.email)?;
+        }
+
+        Ok(())
+    }
+
+    pub fn get(repo_dir: &Path) -> BoxResult<String> {
+        let active_copirates = fs::read_to_string(repo_dir.join(ACTIVE_COPIRATES_PATH))?;
+
+        Ok(active_copirates)
+    }
+}
+

src/copirate.rs

@@ -1,7 +1,9 @@
+// Factor out active copirates as a session type with two states: empty and nonempty
 use serde::Deserialize;
 use std::fs;
-use std::collections::{HashMap};
-use crate::{BoxResult, ACTIVE_COPIRATES_FILE};
+use std::collections::HashMap;
+use crate::BoxResult;
+use std::path::Path;
 
 #[derive(Deserialize, Debug)]
 pub struct CoPirate {
@@ -15,10 +17,18 @@ pub struct CoPirates {
 }
 
 impl CoPirates {
-    pub fn empty_copirates_file() -> BoxResult {
-        fs::write(ACTIVE_COPIRATES_FILE, "")?;
+    pub fn open(copirates_path: &Path) -> BoxResult<CoPirates> {
+        let raw_copirates = fs::read_to_string(copirates_path)?;
+        let existing_copirates = serde_json::from_str(&raw_copirates[..])?;
 
-        Ok(())
+        Ok(existing_copirates)
     }
+
+    pub fn get(&self, copirate: &String) -> BoxResult<&CoPirate> {
+        let copirate = self.copirates.get(copirate).ok_or("Shiver me timbers! This be pirate be a stranger around these ports. Hint: Add it to ~/.git-copirates!")?;
+
+        Ok(copirate)
+    }
+
 }
 

src/embark.rs

@@ -1,30 +1,23 @@
 //! embark sub-command
 
-use crate::{BoxResult, ACTIVE_COPIRATES_FILE};
+use crate::{BoxResult, HOOK_PATH};
 use std::os::unix::fs::OpenOptionsExt;
 use std::path::Path;
 use std::{fs, io};
 
-pub fn embark() -> BoxResult {
-    // TODO: Find the path to the top-level git hooks dir from anywhere, use libgit2?
-    let hook_file = ".git/hooks/prepare-commit-msg";
-    let template_file = ACTIVE_COPIRATES_FILE;
-
-    if Path::new(hook_file).exists() {
+pub fn embark(repo_dir: &Path) -> BoxResult<()> {
+    let hook_path = repo_dir.join(HOOK_PATH);
+    if hook_path.exists() {
         // TODO: Want to bail! here, do I need a custom error type for that?
         panic!("You have an existing prepare-commit-msg hook, which we need to overwrite. Please back it up and remove it!");
     } else {
-        create_hook(hook_file)?;
-    }
-
-    if !Path::new(template_file).exists() {
-        fs::write(template_file, "")?;
+        create_hook(&hook_path)?;
     }
 
     Ok(())
 }
 
-pub fn create_hook(hook_file: &str) -> io::Result<()> {
+pub fn create_hook(hook_file: &Path) -> io::Result<()> {
     let hook_code = "#!/bin/bash
 
 rmob prepare-commit-msg \"$1\"";
@@ -36,7 +29,7 @@ rmob prepare-commit-msg \"$1\"";
 }
 
 // TODO: Make OS-agnostic
-pub fn write_executable(file: &str, contents: &str) -> io::Result<()> {
+pub fn write_executable(file: &Path, contents: &str) -> io::Result<()> {
     fs::OpenOptions::new()
         .create(true)
         .write(true)

src/lib.rs

@@ -2,18 +2,19 @@ use std::error::Error;
 use std::path::{PathBuf};
 
 use structopt::StructOpt;
+use git2::Repository;
 
 mod copirate;
+mod active_copirate;
 mod embark;
 mod prepare_commit_msg;
 mod sail;
 mod solo;
 
-pub const HOOK_NAME: &str = "prepare-commit-msg";
-pub const COPIRATES_FILE: &str = ".git-copirates";
-pub const ACTIVE_COPIRATES_FILE: &str = ".git/.git-rmob-template";
+pub const HOOK_PATH: &str = ".git/hooks/prepare-commit-msg";
+const COPIRATES_PATH: &str = ".git-copirates";
 
-pub type BoxResult = Result<(), Box<dyn Error>>;
+pub type BoxResult<T> = Result<T, Box<dyn Error>>;
 
 #[derive(StructOpt, Clone, Debug)]
 #[structopt(name = "Rmob", version = "0.1.0", author = "")]
@@ -36,26 +37,27 @@ enum Rmob {
     },
 }
 
-pub fn run() -> BoxResult {
+pub fn run() -> BoxResult<()> {
     let rmob = Rmob::from_args();
 
+    let repo = Repository::discover(".")?;
+    let repo_dir = repo.workdir().ok_or("You're ON LAND, stupid.")?;
+
     match rmob {
-        Rmob::Embark {} => embark::embark()?,
+        Rmob::Embark {} => embark::embark(repo_dir)?,
         Rmob::Sail { copirates } => {
             if copirates == ["solo"] {
-                solo::solo()?
+                solo::solo(repo_dir)?
             } else {
-                sail::sail(&copirates)?
+                sail::sail(&copirates, repo_dir)?
             }
         },
-        Rmob::Solo {} => solo::solo()?,
+        Rmob::Solo {} => solo::solo(repo_dir)?,
         Rmob::PrepareCommitMessage {
             commit_message_file,
         } => {
             prepare_commit_msg::inject_into_commit_message_file(
-                commit_message_file
-                    .to_str()
-                    .ok_or("Ayyyr, what's on that hook laddy?")?,
+                &commit_message_file, repo_dir
             )?;
         }
     }

src/main.rs

@@ -1,10 +1,8 @@
 use std::process;
 
-
 fn main() {
     if let Err(e) = rmob::run() {
         eprintln!("{}", e);
         process::exit(1);
     }
 }
-

src/prepare_commit_msg.rs

@@ -1,13 +1,15 @@
 //! prepare-commit-msg subcommand
 
-use crate::{BoxResult, ACTIVE_COPIRATES_FILE};
+use crate::{BoxResult};
 use std::fs;
+use std::path::Path;
+use crate::active_copirate::ActiveCoPirates;
 
-pub fn inject_into_commit_message_file(commit_message_file: &str) -> BoxResult {
+pub fn inject_into_commit_message_file(commit_message_file: &Path, repo_dir: &Path) -> BoxResult<()> {
     const PATTERN: &str = "\n# ";
 
     let commit_message = fs::read_to_string(commit_message_file)?;
-    let mob = fs::read_to_string(ACTIVE_COPIRATES_FILE)?;
+    let mob = ActiveCoPirates::get(repo_dir)?;
     // Can I transform that into .unwrap_or_else()
     let comment_pos = if let Some(message) = commit_message.find(PATTERN) {
         message

src/sail.rs

@@ -1,55 +1,29 @@
 //! start sub-command
 
 extern crate dirs;
-use crate::{BoxResult, COPIRATES_FILE, ACTIVE_COPIRATES_FILE};
+use crate::{BoxResult, COPIRATES_PATH};
 
-use std::fs;
-use std::collections::{HashSet};
-use std::fs::OpenOptions;
-use std::io::prelude::*;
-use crate::copirate::CoPirates;
+use crate::copirate::{CoPirates};
+use std::path::Path;
+use crate::active_copirate::ActiveCoPirates;
 
-pub fn sail(copirates: &[String]) -> BoxResult {
+pub fn sail(copirates: &[String], repo_dir: &Path) -> BoxResult<()> {
     let ship = dirs::home_dir().ok_or("Could not find yer ship oy!")?;
-    let raw_copirates = fs::read_to_string(ship.join(COPIRATES_FILE))?;
-    let existing_copirates: CoPirates = serde_json::from_str(&raw_copirates[..])?;
+    let existing_copirates = CoPirates::open(&ship.join(COPIRATES_PATH))?;
 
-    fail_if_pirate_not_present(copirates, &existing_copirates)?;
-
-    empty_copirates_file()?;
-
-    save_copirates(copirates, existing_copirates)?;
+    save_copirates(copirates, existing_copirates, repo_dir)?;
 
     Ok(())
 }
 
-fn save_copirates(copirates: &[String], existing_copirates: CoPirates) -> BoxResult {
-    let mut file = OpenOptions::new()
-        .append(true)
-        .open(ACTIVE_COPIRATES_FILE)
-        .unwrap();
-    for pirate in copirates {
-        let existing_pirate = existing_copirates.copirates.get(pirate).ok_or("Wait what Sally it was right there?")?;
-        writeln!(file, "Co-authored-by: {} <{}>", existing_pirate.name, existing_pirate.email)?;
-    }
-
-    println!("{:?}", copirates);
-
-    Ok(())
-}
+fn save_copirates(copirates: &[String], existing_copirates: CoPirates, repo_dir: &Path) -> BoxResult<()> {
 
-fn empty_copirates_file() -> BoxResult {
-    fs::write(ACTIVE_COPIRATES_FILE, "")?;
+    let copirates: Vec<_> = copirates.iter().map(| initial| existing_copirates.get(initial)).collect::<Result<_, _>>()?;
 
-    Ok(())
-}
+    let active_copirates = ActiveCoPirates::create_empty(repo_dir)?;
+    active_copirates.save(&copirates[..])?;
 
-fn fail_if_pirate_not_present(copirates: &[String], existing_copirates: &CoPirates) -> BoxResult {
-    let existing_copirates: HashSet<&String> = existing_copirates.copirates.keys().collect();
-    let copirates: HashSet<&String> = copirates.into_iter().collect();
-    if !copirates.is_subset(&existing_copirates) {
-        return Err(Box::from("We didn't recognize this pirate's initials. Please add to your ~/.git-copirates file!"));
-    }
+    println!("Sail away!");
 
     Ok(())
 }

src/solo.rs

@@ -1,10 +1,11 @@
 //! start sub-command
 
 use crate::{BoxResult};
-use crate::copirate::CoPirates;
+use crate::active_copirate::ActiveCoPirates;
+use std::path::Path;
 
-pub fn solo() -> BoxResult {
-    CoPirates::empty_copirates_file()?;
+pub fn solo(repo_dir: &Path) -> BoxResult<()> {
+    ActiveCoPirates::create_empty(repo_dir)?;
 
     println!("All th' gold shall be yers alone.");
 

tests/integration_tests.rs

@@ -5,7 +5,7 @@ use std::process::Command;
 use tempfile::{tempdir, TempDir};
 
 #[test]
-fn it_works() -> BoxResult {
+fn it_works() -> BoxResult<()> {
     let dir = tempdir()?;
     let repo = Repository::init(dir.path())?;
 
@@ -20,34 +20,54 @@ fn it_works() -> BoxResult {
 
     assert_multiline_commit_message_contains_coauthors(&dir, &repo)?;
 
-
     Ok(())
 }
 
-fn init(dir: &TempDir) -> BoxResult {
+fn init(dir: &TempDir) -> BoxResult<()> {
     let mut rmob = Command::cargo_bin("rmob")?;
-    rmob.current_dir(dir.path()).arg("embark").assert().success();
-    let hook = dir.path().join(".git/hooks/").join(HOOK_NAME);
+    rmob.current_dir(dir.path())
+        .arg("embark")
+        .assert()
+        .success();
+    let hook = dir.path().join(".git/hooks/").join(HOOK_PATH);
     assert!(hook.exists());
 
     Ok(())
 }
 
-fn sail(dir: &TempDir) -> BoxResult {
+fn sail(dir: &TempDir) -> BoxResult<()> {
     let mut rmob = Command::cargo_bin("rmob")?;
-    rmob.current_dir(dir.path()).arg("sail").arg("ek").assert().success();
+    rmob.current_dir(dir.path())
+        .arg("sail")
+        .arg("ek")
+        .assert()
+        .success();
 
     Ok(())
 }
 
-fn init_git_repo(dir: &TempDir) -> BoxResult {
-    Command::new("git").current_dir(dir.path()).arg("init").assert().success();
+fn init_git_repo(dir: &TempDir) -> BoxResult<()> {
+    Command::new("git")
+        .current_dir(dir.path())
+        .arg("init")
+        .assert()
+        .success();
 
     Ok(())
 }
 
-fn assert_oneliner_commit_message_contains_coauthors(dir: &TempDir, repo: &Repository) -> BoxResult {
-    Command::new("git").current_dir(dir.path()).arg("commit").arg("-m").arg("Arrrrrr!").arg("--allow-empty").assert().success();
+fn assert_oneliner_commit_message_contains_coauthors(
+    dir: &TempDir,
+    repo: &Repository,
+) -> BoxResult<()> {
+    Command::new("git")
+        .current_dir(dir.path())
+        .arg("commit")
+        .arg("-m")
+        .arg("Arrrrrr!")
+        .arg("--allow-empty")
+        .assert()
+        .success();
 
     let commit = find_last_commit(&repo)?;
     assert!(
@@ -61,7 +81,10 @@ fn assert_oneliner_commit_message_contains_coauthors(dir: &TempDir, repo: &Repos
     Ok(())
 }
 
-fn assert_multiline_commit_message_contains_coauthors(dir: &TempDir, repo: &Repository) -> BoxResult {
+fn assert_multiline_commit_message_contains_coauthors(
+    dir: &TempDir,
+    repo: &Repository,
+) -> BoxResult<()> {
     const MULTILINE_MESSAGE: &str = r#"
 # Please enter the commit message for your changes. Lines starting
 # with '#' will be ignored, and an empty message aborts the commit.
@@ -71,7 +94,14 @@ fn assert_multiline_commit_message_contains_coauthors(dir: &TempDir, repo: &Repo
 #	modified:   tests/integration_tests.rs
 #
     "#;
-    Command::new("git").current_dir(dir.path()).arg("commit").arg("-m").arg(MULTILINE_MESSAGE).arg("--allow-empty").assert().success();
+    Command::new("git")
+        .current_dir(dir.path())
+        .arg("commit")
+        .arg("-m")
+        .arg(MULTILINE_MESSAGE)
+        .arg("--allow-empty")
+        .assert()
+        .success();
 
     let commit = find_last_commit(&repo)?;
     assert!(