Check common branch names instead of requiring GIT_INSTAFIX_UPSTREAM (#25)

Fixes #23

Author: quodlibetor

src/lib.rs

@@ -4,28 +4,32 @@ use anyhow::{anyhow, bail, Context};
 use console::style;
 use dialoguer::{Confirm, Select};
 use git2::{
-    Branch, Commit, Diff, DiffFormat, DiffStatsFormat, Object, ObjectType, Oid, Rebase, Repository,
+    Branch, BranchType, Commit, Diff, DiffFormat, DiffStatsFormat, Object, Oid, Rebase, Repository,
 };
 use syntect::easy::HighlightLines;
 use syntect::highlighting::ThemeSet;
 use syntect::parsing::SyntaxSet;
 use syntect::util::as_24_bit_terminal_escaped;
 
+const DEFAULT_UPSTREAM_BRANCHES: &[&str] = &["main", "master", "develop", "trunk"];
+
 pub fn instafix(
     squash: bool,
     max_commits: usize,
     message_pattern: Option<String>,
     upstream_branch_name: Option<&str>,
     require_newline: bool,
 ) -> Result<(), anyhow::Error> {
-    let repo = Repository::open(".")?;
-    let diff = create_diff(&repo, require_newline)?;
+    let repo = Repository::open(".").context("opening repo")?;
+    let diff = create_diff(&repo, require_newline).context("creating diff")?;
     let head = repo.head().context("finding head commit")?;
     let head_branch = Branch::wrap(head);
-    let upstream = get_upstream(&repo, &head_branch, upstream_branch_name)?;
-    let commit_to_amend = select_commit_to_amend(&repo, upstream, max_commits, &message_pattern)?;
+    let upstream =
+        get_merge_base(&repo, &head_branch, upstream_branch_name).context("creating merge base")?;
+    let commit_to_amend = select_commit_to_amend(&repo, upstream, max_commits, &message_pattern)
+        .context("selecting commit to amend")?;
     eprintln!("Selected {}", disp(&commit_to_amend));
-    do_fixup_commit(&repo, &head_branch, &commit_to_amend, squash)?;
+    do_fixup_commit(&repo, &head_branch, &commit_to_amend, squash).context("doing fixup commit")?;
     let needs_stash = worktree_is_dirty(&repo)?;
     if needs_stash {
         // TODO: is it reasonable to create a new repo to work around lifetime issues?
@@ -190,24 +194,21 @@ fn disp(commit: &Commit) -> String {
     )
 }
 
-fn get_upstream<'a>(
+fn get_merge_base<'a>(
     repo: &'a Repository,
     head_branch: &'a Branch,
     upstream_name: Option<&str>,
 ) -> Result<Option<Object<'a>>, anyhow::Error> {
-    let upstream = if let Some(upstream_name) = upstream_name {
-        let branch = repo
-            .branches(None)?
-            .filter_map(|branch| branch.ok().map(|(b, _type)| b))
-            .find(|b| {
-                b.name()
-                    .map(|n| n.expect("valid utf8 branchname") == upstream_name)
-                    .unwrap_or(false)
-            })
-            .ok_or_else(|| anyhow!("cannot find branch with name {:?}", upstream_name))?;
-        branch.into_reference().peel(ObjectType::Commit)?
+    let upstream = if let Some(explicit_upstream_name) = upstream_name {
+        let branch = repo.find_branch(explicit_upstream_name, BranchType::Local)?;
+        branch.into_reference().peel_to_commit()?
+    } else if let Some(branch) = DEFAULT_UPSTREAM_BRANCHES
+        .iter()
+        .find_map(|b| repo.find_branch(b, BranchType::Local).ok())
+    {
+        branch.into_reference().peel_to_commit()?
     } else if let Ok(upstream) = head_branch.upstream() {
-        upstream.into_reference().peel(ObjectType::Commit)?
+        upstream.into_reference().peel_to_commit()?
     } else {
         return Ok(None);
     };
@@ -335,6 +336,8 @@ fn select_commit_to_amend<'a>(
         })
         .collect();
     if let Some(message_pattern) = message_pattern.as_ref() {
+        let first = commit_id_and_summary(&commits, commits.len() - 1);
+        let last = commit_id_and_summary(&commits, 0);
         commits
             .into_iter()
             .find(|commit| {
@@ -343,7 +346,13 @@ fn select_commit_to_amend<'a>(
                     .map(|s| s.contains(message_pattern))
                     .unwrap_or(false)
             })
-            .ok_or_else(|| anyhow::anyhow!("No commit contains the pattern in its summary"))
+            .ok_or_else(|| {
+                anyhow::anyhow!(
+                    "No commit contains the pattern in its summary between {}..{}",
+                    first,
+                    last
+                )
+            })
     } else {
         let rev_aliases = commits
             .iter()
@@ -375,6 +384,20 @@ fn select_commit_to_amend<'a>(
     }
 }
 
+fn commit_id_and_summary(commits: &[Commit<'_>], idx: usize) -> String {
+    let first = commits
+        .get(idx)
+        .map(|c| {
+            format!(
+                "{} ({})",
+                &c.id().to_string()[..10],
+                c.summary().unwrap_or("<unknown>")
+            )
+        })
+        .unwrap_or_else(|| "<unknown>".into());
+    first
+}
+
 fn format_ref(rf: &git2::Reference<'_>) -> Result<String, anyhow::Error> {
     let shorthand = rf.shorthand().unwrap_or("<unnamed>");
     let sha = rf.peel_to_commit()?.id().to_string();

tests/basic.rs

@@ -20,14 +20,13 @@ fn test_can_compile() {
 }
 
 #[test]
-fn test_straightforward() {
+fn straightforward() {
     let td = assert_fs::TempDir::new().unwrap();
     git_init(&td);
 
     git_file_commit("a", &td);
     git_file_commit("b", &td);
     git(&["checkout", "-b", "changes", "HEAD~"], &td);
-    git(&["branch", "-u", "main"], &td);
     for n in &["c", "d", "e"] {
         git_file_commit(&n, &td);
     }
@@ -71,6 +70,47 @@ new
     );
 }
 
+#[test]
+fn uses_merge_base_for_all_defaults() {
+    for branch in ["main", "develop", "trunk", "master"] {
+        eprintln!("testing branch {branch}");
+        let td = assert_fs::TempDir::new().unwrap();
+        git_init_default_branch_name(branch, &td);
+
+        git_commits(&["a", "b", "c", "d"], &td);
+        git(&["checkout", "-b", "changes", ":/c"], &td);
+        git_commits(&["f", "g"], &td);
+
+        let expected = format!(
+            "\
+* g HEAD -> changes
+* f
+| * d {branch}
+|/
+* c
+* b
+* a
+"
+        );
+        let actual = git_log(&td);
+        assert_eq!(
+            expected, actual,
+            "expected:\n{}\nactual:\n{}",
+            expected, actual
+        );
+
+        // commits *before* the merge base of a default branch don't get found by
+        // default
+        td.child("new").touch().unwrap();
+        git(&["add", "new"], &td);
+        fixup(&td).args(&["-P", "b"]).assert().failure();
+        // commits *after* the merge base of a default branch *do* get found by default
+        git(&["reset", "HEAD~"], &td);
+        git(&["add", "new"], &td);
+        fixup(&td).args(&["-P", "f"]).assert().success();
+    }
+}
+
 #[test]
 fn simple_straightline_commits() {
     let td = assert_fs::TempDir::new().unwrap();
@@ -335,7 +375,11 @@ fn git_commits(ids: &[&str], tempdir: &assert_fs::TempDir) {
 }
 
 fn git_init(tempdir: &assert_fs::TempDir) {
-    git(&["init", "--initial-branch=main"], &tempdir);
+    git_init_default_branch_name("main", tempdir)
+}
+
+fn git_init_default_branch_name(name: &str, tempdir: &assert_fs::TempDir) {
+    git(&["init", "--initial-branch", name], &tempdir);
     git(&["config", "user.email", "[email protected]"], &tempdir);
     git(&["config", "user.name", "nobody"], &tempdir);
 }
@@ -403,6 +447,7 @@ fn git_inner(args: &[&str], tempdir: &assert_fs::TempDir) -> Command {
 /// Get something that can get args added to it
 fn fixup(dir: &assert_fs::TempDir) -> Command {
     let mut c = Command::cargo_bin("git-instafix").unwrap();
-    c.current_dir(&dir.path());
+    c.current_dir(&dir.path())
+        .env_remove("GIT_INSTAFIX_UPSTREAM");
     c
 }