Merge pull request #91 from rbartlensky/one-fixup-per-commit

Add feature to generate one fixup per commit.

2 files changed, 197 insertions, 8 deletions

diff --git a/src/lib.rs b/src/lib.rs
index 96a9478..c61f997 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -14,13 +14,17 @@ pub struct Config<'a> {
     pub base: Option<&'a str>,
     pub and_rebase: bool,
     pub whole_file: bool,
+    pub one_fixup_per_commit: bool,
     pub logger: &'a slog::Logger,
 }
 
 pub fn run(config: &Config) -> Result<()> {
     let repo = git2::Repository::open_from_env()?;
     debug!(config.logger, "repository found"; "path" => repo.path().to_str());
+    run_with_repo(config, &repo)
+}
 
+fn run_with_repo(config: &Config, repo: &git2::Repository) -> Result<()> {
     let stack = stack::working_stack(&repo, config.base, config.force, config.logger)?;
     if stack.is_empty() {
         crit!(config.logger, "No commits available to fix up, exiting");
@@ -80,6 +84,8 @@ pub fn run(config: &Config) -> Result<()> {
         .or_else(|_| git2::Signature::now("nobody", "nobody@example.com"))?;
     let mut head_commit = repo.head()?.peel_to_commit()?;
 
+    let mut hunks_with_commit = vec![];
+
     let mut patches_considered = 0usize;
     'patch: for index_patch in index.iter() {
         let old_path = index_patch.new_path.as_slice();
@@ -174,7 +180,10 @@ pub fn run(config: &Config) -> Result<()> {
                 // cases, might be helpful to just match the first commit touching the same
                 // file as the current hunk. Use this option with care!
                 if config.whole_file {
-                    debug!(c_logger, "Commit touches the hunk file and match whole file is enabled");
+                    debug!(
+                        c_logger,
+                        "Commit touches the hunk file and match whole file is enabled"
+                    );
                     dest_commit = Some(commit);
                     break 'commit;
                 }
@@ -223,18 +232,56 @@ pub fn run(config: &Config) -> Result<()> {
                 }
             };
 
+            let hunk_with_commit = HunkWithCommit {
+                hunk_to_apply,
+                dest_commit,
+                index_patch,
+            };
+            hunks_with_commit.push(hunk_with_commit);
+
+            applied_hunks_offset += hunk_offset;
+        }
+    }
+
+    hunks_with_commit.sort_by_key(|h| h.dest_commit.id());
+    // * apply all hunks that are going to be fixed up into `dest_commit`
+    // * commit the fixup
+    // * repeat for all `dest_commit`s
+    //
+    // the `.zip` here will gives us something similar to `.windows`, but with
+    // an extra iteration for the last element (otherwise we would have to
+    // special case the last element and commit it separately)
+    for (current, next) in hunks_with_commit
+        .iter()
+        .zip(hunks_with_commit.iter().skip(1).map(Some).chain([None]))
+    {
+        head_tree = apply_hunk_to_tree(
+            &repo,
+            &head_tree,
+            &current.hunk_to_apply,
+            &current.index_patch.old_path,
+        )?;
+
+        // whether there are no more hunks to apply to `dest_commit`
+        let commit_fixup = next.map_or(true, |next| {
+            // if the next hunk is for a different commit -- commit what we have so far
+            !config.one_fixup_per_commit || next.dest_commit.id() != current.dest_commit.id()
+        });
+        if commit_fixup {
             // TODO: the git2 api only supports utf8 commit messages,
             // so it's okay to use strings instead of bytes here
             // https://docs.rs/git2/0.7.5/src/git2/repo.rs.html#998
             // https://libgit2.org/libgit2/#HEAD/group/commit/git_commit_create
-            let dest_commit_id = dest_commit.id().to_string();
-            let dest_commit_locator = dest_commit
+            let dest_commit_id = current.dest_commit.id().to_string();
+            let dest_commit_locator = current
+                .dest_commit
                 .summary()
                 .filter(|&msg| summary_counts[msg] == 1)
                 .unwrap_or(&dest_commit_id);
+            let diff = repo
+                .diff_tree_to_tree(Some(&head_commit.tree()?), Some(&head_tree), None)?
+                .stats()?;
             if !config.dry_run {
-                head_tree =
-                    apply_hunk_to_tree(&repo, &head_tree, &hunk_to_apply, &index_patch.old_path)?;
                 head_commit = repo.find_commit(repo.commit(
                     Some("HEAD"),
                     &signature,
@@ -245,15 +292,14 @@ pub fn run(config: &Config) -> Result<()> {
                 )?)?;
                 info!(config.logger, "committed";
                       "commit" => head_commit.id().to_string(),
-                      "header" => hunk_to_apply.header(),
+                      "header" => format!("+{},-{}", diff.insertions(), diff.deletions()),
                 );
             } else {
                 info!(config.logger, "would have committed";
                       "fixup" => dest_commit_locator,
-                      "header" => hunk_to_apply.header(),
+                      "header" => format!("+{},-{}", diff.insertions(), diff.deletions()),
                 );
             }
-            applied_hunks_offset += hunk_offset;
         }
     }
 
@@ -290,6 +336,12 @@ pub fn run(config: &Config) -> Result<()> {
     Ok(())
 }
 
+struct HunkWithCommit<'c, 'r, 'p> {
+    hunk_to_apply: owned::Hunk,
+    dest_commit: &'c git2::Commit<'r>,
+    index_patch: &'p owned::Patch,
+}
+
 fn apply_hunk_to_tree<'repo>(
     repo: &'repo git2::Repository,
     base: &git2::Tree,
@@ -364,3 +416,132 @@ fn split_lines_after(content: &[u8], n: usize) -> (&[u8], &[u8]) {
     };
     content.split_at(split_index)
 }
+
+#[cfg(test)]
+mod tests {
+    use std::path::{Path, PathBuf};
+
+    use super::*;
+
+    struct Context {
+        repo: git2::Repository,
+        dir: tempfile::TempDir,
+    }
+
+    impl Context {
+        fn join(&self, p: &Path) -> PathBuf {
+            self.dir.path().join(p)
+        }
+    }
+
+    /// Prepare a fresh git repository with an initial commit and a file.
+    fn prepare_repo() -> (Context, PathBuf) {
+        let dir = tempfile::tempdir().unwrap();
+        let repo = git2::Repository::init(dir.path()).unwrap();
+
+        let path = PathBuf::from("test-file.txt");
+        std::fs::write(
+            dir.path().join(&path),
+            br#"
+line
+line
+
+more
+lines
+"#,
+        )
+        .unwrap();
+
+        // make the borrow-checker happy by introducing a new scope
+        {
+            let tree = add(&repo, &path);
+            let signature = repo
+                .signature()
+                .or_else(|_| git2::Signature::now("nobody", "nobody@example.com"))
+                .unwrap();
+            repo.commit(
+                Some("HEAD"),
+                &signature,
+                &signature,
+                "Initial commit.",
+                &tree,
+                &[],
+            )
+            .unwrap();
+        }
+
+        (Context { repo, dir }, path)
+    }
+
+    /// Stage the changes made to `path`.
+    fn add<'r>(repo: &'r git2::Repository, path: &Path) -> git2::Tree<'r> {
+        let mut index = repo.index().unwrap();
+        index.add_path(&path).unwrap();
+        index.write().unwrap();
+
+        let tree_id = index.write_tree_to(&repo).unwrap();
+        repo.find_tree(tree_id).unwrap()
+    }
+
+    /// Prepare an empty repo, and stage some changes.
+    fn prepare_and_stage() -> Context {
+        let (ctx, file_path) = prepare_repo();
+
+        // add some lines to our file
+        let path = ctx.join(&file_path);
+        let contents = std::fs::read_to_string(&path).unwrap();
+        let modifications = format!("new_line1\n{contents}\nnew_line2");
+        std::fs::write(&path, &modifications).unwrap();
+
+        // stage it
+        add(&ctx.repo, &file_path);
+
+        ctx
+    }
+
+    #[test]
+    fn multiple_fixups_per_commit() {
+        let ctx = prepare_and_stage();
+
+        // run 'git-absorb'
+        let drain = slog::Discard;
+        let logger = slog::Logger::root(drain, o!());
+        let config = Config {
+            dry_run: false,
+            force: false,
+            base: None,
+            and_rebase: false,
+            whole_file: false,
+            one_fixup_per_commit: false,
+            logger: &logger,
+        };
+        run_with_repo(&config, &ctx.repo).unwrap();
+
+        let mut revwalk = ctx.repo.revwalk().unwrap();
+        revwalk.push_head().unwrap();
+        assert_eq!(revwalk.count(), 3);
+    }
+
+    #[test]
+    fn one_fixup_per_commit() {
+        let ctx = prepare_and_stage();
+
+        // run 'git-absorb'
+        let drain = slog::Discard;
+        let logger = slog::Logger::root(drain, o!());
+        let config = Config {
+            dry_run: false,
+            force: false,
+            base: None,
+            and_rebase: false,
+            whole_file: false,
+            one_fixup_per_commit: true,
+            logger: &logger,
+        };
+        run_with_repo(&config, &ctx.repo).unwrap();
+
+        let mut revwalk = ctx.repo.revwalk().unwrap();
+        revwalk.push_head().unwrap();
+        assert_eq!(revwalk.count(), 2);
+    }
+}
diff --git a/src/main.rs b/src/main.rs
index 7cb83b9..5ee6fef 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -59,6 +59,13 @@ fn main() {
                 .short("w")
                 .long("whole-file")
                 .takes_value(false),
+        )
+        .arg(
+            clap::Arg::with_name("one-fixup-per-commit")
+                .help("Only generate one fixup per commit")
+                .short("F")
+                .long("one-fixup-per-commit")
+                .takes_value(false),
         );
     let mut args_clone = args.clone();
     let args = args.get_matches();
@@ -112,6 +119,7 @@ fn main() {
         base: args.value_of("base"),
         and_rebase: args.is_present("and-rebase"),
         whole_file: args.is_present("whole-file"),
+        one_fixup_per_commit: args.is_present("one-fixup-per-commit"),
         logger: &logger,
     }) {
         crit!(logger, "absorb failed"; "err" => e.to_string());