minor refactor before the actual change

Author: Byron

crates/gitbutler-branch-actions/src/undo_commit.rs

@@ -7,24 +7,26 @@ use gitbutler_repo::{
     rebase::cherry_rebase_group,
 };
 use gitbutler_stack::{OwnershipClaim, Stack, StackId};
+use tracing::instrument;
 
 use crate::VirtualBranchesExt as _;
 
 /// Removes a commit from a branch by rebasing all commits _except_ for it
 /// onto it's parent.
 ///
-/// if successful, it will update the branch head to the new head commit.
+/// If successful, it will update the branch head to the new head commit.
 ///
 /// It intentionally does **not** update the branch tree. It is a feature
 /// of the operation that the branch tree will not be updated as it allows
 /// the user to then re-commit the changes if they wish.
 ///
 /// This may create conflicted commits above the commit that is getting
 /// undone.
+#[instrument(level = tracing::Level::DEBUG, skip(ctx))]
 pub(crate) fn undo_commit(
     ctx: &CommandContext,
     stack_id: StackId,
-    commit_oid: git2::Oid,
+    commit_to_remove: git2::Oid,
 ) -> Result<Stack> {
     let vb_state = ctx.project().virtual_branches();
 
@@ -33,15 +35,15 @@ pub(crate) fn undo_commit(
     let UndoResult {
         new_head: new_head_commit,
         ownership_update,
-    } = inner_undo_commit(ctx.repo(), stack.head(), commit_oid)?;
+    } = rebase_all_but_last(ctx.repo(), stack.head(), commit_to_remove)?;
 
     for ownership in ownership_update {
         stack.ownership.put(ownership);
     }
 
     stack.set_stack_head(ctx, new_head_commit, None)?;
 
-    let removed_commit = ctx.repo().find_commit(commit_oid)?;
+    let removed_commit = ctx.repo().find_commit(commit_to_remove)?;
     stack.replace_head(ctx, &removed_commit, &removed_commit.parent(0)?)?;
 
     crate::integration::update_workspace_commit(&vb_state, ctx)
@@ -55,7 +57,8 @@ struct UndoResult {
     ownership_update: Vec<OwnershipClaim>,
 }
 
-fn inner_undo_commit(
+#[instrument(level = tracing::Level::DEBUG, skip(repository))]
+fn rebase_all_but_last(
     repository: &git2::Repository,
     branch_head_commit: git2::Oid,
     commit_to_remove: git2::Oid,
@@ -82,14 +85,13 @@ fn inner_undo_commit(
                 .hunks
                 .iter()
                 .map(Into::into)
-                .filter(|hunk: &Hunk| hunk.start != 0 && hunk.end != 0)
+                .filter(|hunk: &Hunk| !hunk.is_null())
                 .collect::<Vec<_>>();
             if hunks.is_empty() {
                 return None;
             }
-            Some((file_path, hunks))
+            Some(OwnershipClaim { file_path, hunks })
         })
-        .map(|(file_path, hunks)| OwnershipClaim { file_path, hunks })
         .collect::<Vec<_>>();
 
     // if commit is the head, just set head to the parent
@@ -131,7 +133,7 @@ mod test {
             assert_commit_tree_matches, TestingRepository,
         };
 
-        use crate::undo_commit::{inner_undo_commit, UndoResult};
+        use crate::undo_commit::{rebase_all_but_last, UndoResult};
 
         #[test]
         fn undoing_conflicted_commit_errors() {
@@ -146,7 +148,7 @@ mod test {
 
             // Branch looks like "A -> ConflictedCommit"
 
-            let result = inner_undo_commit(
+            let result = rebase_all_but_last(
                 &test_repository.repository,
                 conflicted_commit.id(),
                 conflicted_commit.id(),
@@ -169,7 +171,7 @@ mod test {
             let UndoResult {
                 new_head,
                 ownership_update,
-            } = inner_undo_commit(&test_repository.repository, c.id(), c.id()).unwrap();
+            } = rebase_all_but_last(&test_repository.repository, c.id(), c.id()).unwrap();
 
             assert_eq!(new_head, b.id(), "The new head should be C's parent");
             assert_eq!(
@@ -208,7 +210,7 @@ mod test {
             let UndoResult {
                 new_head,
                 ownership_update,
-            } = inner_undo_commit(&test_repository.repository, c.id(), b.id()).unwrap();
+            } = rebase_all_but_last(&test_repository.repository, c.id(), b.id()).unwrap();
 
             let new_head_commit: git2::Commit =
                 test_repository.repository.find_commit(new_head).unwrap();

crates/gitbutler-diff/src/hunk.rs

@@ -9,8 +9,12 @@ pub type HunkHash = md5::Digest;
 
 #[derive(Debug, Eq, Clone)]
 pub struct Hunk {
+    /// A hash over the actual lines of the hunk, including the newlines between them
+    /// (i.e. the first character of the first line to the last character of the last line in the input buffer)
     pub hash: Option<HunkHash>,
+    /// The index of the first line this hunk is representing.
     pub start: u32,
+    /// The index of *one past* the last line this hunk is representing.
     pub end: u32,
 }
 
@@ -91,6 +95,7 @@ impl Display for Hunk {
     }
 }
 
+/// Instantiation
 impl Hunk {
     pub fn new(start: u32, end: u32, hash: Option<HunkHash>) -> Result<Self> {
         if start > end {
@@ -104,18 +109,28 @@ impl Hunk {
         self.hash = Some(hash);
         self
     }
+}
 
-    pub(crate) fn contains(&self, line: u32) -> bool {
+/// Access
+impl Hunk {
+    fn contains_line(&self, line: u32) -> bool {
         self.start <= line && self.end >= line
     }
 
     pub fn intersects(&self, another: &diff::GitHunk) -> bool {
-        self.contains(another.new_start)
-            || self.contains(another.new_start + another.new_lines)
+        self.contains_line(another.new_start)
+            || self.contains_line(another.new_start + another.new_lines)
             || another.contains(self.start)
             || another.contains(self.end)
     }
 
+    pub fn is_null(&self) -> bool {
+        self.start == self.end && self.start == 0
+    }
+}
+
+/// Hashing
+impl Hunk {
     /// Produce a hash from `diff` as hex-string, which is **assumed to have a one-line diff header**!
     /// `diff` can also be entirely empty, or not contain a diff header which is when it will just be hashed
     /// with [`Self::hash()`].

crates/gitbutler-oxidize/Cargo.toml

@@ -7,5 +7,5 @@ publish = false
 
 [dependencies]
 anyhow.workspace = true
-gix.workspace = true
-git2.workspace = true
+git2.workspace = true
+gix = { workspace = true, features = ["merge"] }

crates/gitbutler-repo/src/rebase.rs

@@ -14,6 +14,7 @@ use gitbutler_commit::{
 };
 use gitbutler_oxidize::{git2_to_gix_object_id, gix_to_git2_oid, GixRepositoryExt as _};
 use serde::{Deserialize, Serialize};
+use tracing::instrument;
 
 /// cherry-pick based rebase, which handles empty commits
 /// this function takes a commit range and generates a Vector of commit oids
@@ -46,6 +47,7 @@ pub fn cherry_rebase(
 /// rebase empty commits (two commits with identical trees)
 ///
 /// the commit id's to rebase should be ordered such that the child most commit is first
+#[instrument(level = tracing::Level::DEBUG, skip(repository, ids_to_rebase))]
 pub fn cherry_rebase_group(
     repository: &git2::Repository,
     target_commit_oid: git2::Oid,