Merge pull request #5286 from gitbutlerapp/kv-branch-1

Refactor list series for Stack

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

@@ -48,7 +48,9 @@ pub fn integrate_upstream_commits_for_series(
     )?
     .head;
 
-    let do_rebease = branch.allow_rebasing || Some(subject_series) != all_series.first();
+    let do_rebease = branch.allow_rebasing
+        || Some(subject_series.head.name.clone())
+            != all_series.first().map(|s| s.head.name.clone());
     let integrate_upstream_context = IntegrateUpstreamContext {
         repository: repo,
         target_branch_head: default_target.sha,

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

@@ -1,13 +1,12 @@
 use std::collections::HashMap;
 
-use anyhow::anyhow;
 use anyhow::{Context, Result};
 use gitbutler_command_context::CommandContext;
 use gitbutler_commit::commit_ext::CommitExt;
 use gitbutler_patch_reference::{CommitOrChangeId, PatchReference};
 use gitbutler_project::Project;
 use gitbutler_repo_actions::RepoActionsExt;
-use gitbutler_stack::{commit_by_oid_or_change_id, CommitsForId, PatchReferenceUpdate};
+use gitbutler_stack::PatchReferenceUpdate;
 use gitbutler_stack::{Stack, StackId, Target};
 use serde::{Deserialize, Serialize};
 
@@ -169,9 +168,6 @@ pub(crate) fn stack_series(
     let mut requires_force = false;
     let mut api_series: Vec<PatchSeries> = vec![];
     let stack_series = branch.list_series(ctx)?;
-    let merge_base = ctx
-        .repository()
-        .merge_base(branch.head(), default_target.sha)?;
     for series in stack_series.clone() {
         let remote = default_target.push_remote_name();
         let upstream_reference = if series.head.pushed(remote.as_str(), ctx)? {
@@ -182,15 +178,11 @@ pub(crate) fn stack_series(
         let mut patches: Vec<VirtualBranchCommit> = vec![];
         let mut is_integrated = false;
         // Reverse first instead of later, so that we catch the first integrated commit
-        for patch in series.clone().local_commits.iter().rev() {
+        for commit in series.clone().local_commits.iter().rev() {
-            let commit = first_non_duplicate(
-                &patches,
-                commit_by_oid_or_change_id(patch, ctx.repository(), branch.head(), merge_base)?,
-            )?;
             if !is_integrated {
-                is_integrated = check_commit.is_integrated(&commit)?;
+                is_integrated = check_commit.is_integrated(commit)?;
             }
-            let copied_from_remote_id = CommitData::try_from(&commit)
+            let copied_from_remote_id = CommitData::try_from(commit)
                 .ok()
                 .and_then(|data| remote_commit_data.get(&data).copied());
             let remote_commit_id = commit
@@ -202,7 +194,7 @@ pub(crate) fn stack_series(
                         .cloned()
                 })
                 .or(copied_from_remote_id)
-                .or(if series.remote(patch) {
+                .or(if series.remote(commit) {
                     Some(commit.id())
                 } else {
                     None
@@ -213,9 +205,9 @@ pub(crate) fn stack_series(
             let vcommit = commit_to_vbranch_commit(
                 ctx,
                 branch,
-                &commit,
+                commit,
                 is_integrated,
-                series.remote(patch),
+                series.remote(commit),
                 copied_from_remote_id,
                 remote_commit_id,
             )?;
@@ -225,32 +217,18 @@ pub(crate) fn stack_series(
         patches.dedup_by(|a, b| a.id == b.id);
 
         let mut upstream_patches = vec![];
-        if let Some(upstream_reference) = upstream_reference.clone() {
+        for commit in series.upstream_only_commits {
-            let remote_head = ctx
+            let is_integrated = check_commit.is_integrated(&commit)?;
-                .repository()
+            let vcommit = commit_to_vbranch_commit(
-                .find_reference(&upstream_reference)?
+                ctx,
-                .peel_to_commit()?;
+                branch,
-            for patch in series.upstream_only_commits {
+                &commit,
-                if let Ok(commit) = commit_by_oid_or_change_id(
+                is_integrated,
-                    &patch,
+                true, // per definition
-                    ctx.repository(),
+                None, // per definition
-                    remote_head.id(),
+                Some(commit.id()),
-                    merge_base,
+            )?;
-                ) {
+            upstream_patches.push(vcommit);
-                    let commit = first_non_duplicate(&upstream_patches, commit)?;
-                    let is_integrated = check_commit.is_integrated(&commit)?;
-                    let vcommit = commit_to_vbranch_commit(
-                        ctx,
-                        branch,
-                        &commit,
-                        is_integrated,
-                        true, // per definition
-                        None, // per definition
-                        Some(commit.id()),
-                    )?;
-                    upstream_patches.push(vcommit);
-                };
-            }
         }
         upstream_patches.reverse();
         // There should be no duplicates, but dedup because the UI cant handle duplicates
@@ -277,25 +255,3 @@ pub(crate) fn stack_series(
 
     Ok((api_series, requires_force))
 }
-
-fn first_non_duplicate<'a>(
-    vb_commits: &[VirtualBranchCommit],
-    commits_for_id: CommitsForId<'a>,
-) -> Result<git2::Commit<'a>> {
-    dbg!(&commits_for_id);
-    if vb_commits.is_empty() {
-        // Nothing to compare against - pick the first one
-        return Ok(commits_for_id.head);
-    }
-    if !vb_commits.iter().any(|c| c.id == commits_for_id.head.id()) {
-        // The head commit is not a duplicate - pick that
-        Ok(commits_for_id.head)
-    } else {
-        // Iterate the rest of the commits and pick the first one that is not a duplicate
-        commits_for_id
-            .tail
-            .into_iter()
-            .find(|cfi| !vb_commits.iter().any(|c| c.id == cfi.id()))
-            .ok_or_else(|| anyhow!("No non-duplicate commit found"))
-    }
-}

crates/gitbutler-commit/src/commit_ext.rs

@@ -31,3 +31,28 @@ impl<'repo> CommitExt for git2::Commit<'repo> {
             .unwrap_or(false)
     }
 }
+
+fn contains<'a, I>(iter: I, item: &git2::Commit<'a>) -> bool
+where
+    I: IntoIterator<Item = git2::Commit<'a>>,
+{
+    iter.into_iter().any(|iter_item| {
+        // Return true if the commits match by commit id, or alternatively if both have a change id and they match.
+        if iter_item.id() == item.id() {
+            return true;
+        }
+        matches!((iter_item.change_id(), item.change_id()), (Some(iter_item_id), Some(iter_id)) if iter_item_id == iter_id)
+    })
+}
+
+pub trait CommitVecExt {
+    /// Returns `true` if the provided commit is part of the commits in this series.
+    /// Compares the commits by commit id first and also by change ID
+    fn contains_by_commit_or_change_id(&self, commit: &git2::Commit) -> bool;
+}
+
+impl CommitVecExt for Vec<git2::Commit<'_>> {
+    fn contains_by_commit_or_change_id(&self, commit: &git2::Commit) -> bool {
+        contains(self.iter().cloned(), commit)
+    }
+}

crates/gitbutler-stack/src/series.rs

@@ -1,4 +1,6 @@
-use gitbutler_patch_reference::{CommitOrChangeId, PatchReference};
+use git2::Commit;
+use gitbutler_commit::commit_ext::CommitVecExt;
+use gitbutler_patch_reference::PatchReference;
 use std::collections::HashMap;
 
 /// Series or (patch) Series is a set of patches (commits) that are dependent on each other.
@@ -6,8 +8,8 @@ use std::collections::HashMap;
 /// The difference from a branch is that only the patches (commits) unique to the series are included.
 ///
 /// The `pushed` status, as well as the `remote_reference` can be obtained from the methods on `head` (PatchReference).
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone)]
-pub struct Series {
+pub struct Series<'a> {
     /// The GitButler-managed head reference for this series. It points to a commit ID or a change ID in the stack.
     /// This head may or may not be part of the commits that are in the series
     /// There may be multiple "series" that point to the same head (e.g. when a new series / head is created)
@@ -15,22 +17,22 @@ pub struct Series {
     /// The local commits that are part of this series.
     /// The commits in one "series" never overlap with the commits in another series.
     /// Topologically ordered, the first entry is the newest in the series.
-    pub local_commits: Vec<CommitOrChangeId>,
+    pub local_commits: Vec<Commit<'a>>,
     /// The remote commits that are part of this series.
     /// If the branch/series have never been pushed, this list will be empty.
     /// Topologically ordered, the first entry is the newest in the series.
-    pub remote_commits: Vec<CommitOrChangeId>,
+    pub remote_commits: Vec<Commit<'a>>,
     /// The list of patches that are only in the upstream (remote) and not in the local commits,
     /// as determined by the commit ID or change ID.
-    pub upstream_only_commits: Vec<CommitOrChangeId>,
+    pub upstream_only_commits: Vec<Commit<'a>>,
     /// The commit IDs of the remote commits that are part of this series, grouped by change id.
     /// Since we don't have a change_id to commit_id index, this is used to determine
     pub remote_commit_ids_by_change_id: HashMap<String, git2::Oid>,
 }
 
-impl Series {
+impl Series<'_> {
     /// Returns `true` if the provided patch is part of the remote commits in this series (i.e. has been pushed).
-    pub fn remote(&self, patch: &CommitOrChangeId) -> bool {
+    pub fn remote(&self, patch: &Commit<'_>) -> bool {
-        self.remote_commits.contains(patch)
+        self.remote_commits.contains_by_commit_or_change_id(patch)
     }
 }

crates/gitbutler-stack/src/stack.rs

@@ -8,6 +8,7 @@ use anyhow::Result;
 use git2::Commit;
 use gitbutler_command_context::CommandContext;
 use gitbutler_commit::commit_ext::CommitExt;
+use gitbutler_commit::commit_ext::CommitVecExt;
 use gitbutler_id::id::Id;
 use gitbutler_patch_reference::{CommitOrChangeId, PatchReference};
 use gitbutler_reference::{normalize_branch_name, Refname, RemoteRefname, VirtualRefname};
@@ -458,7 +459,7 @@ impl Stack {
     /// Returns a list of all branches/series in the stack.
     /// This operation will compute the current list of local and remote commits that belong to each series.
     /// The first entry is the newest in the Stack (i.e. the top of the stack).
-    pub fn list_series(&self, ctx: &CommandContext) -> Result<Vec<Series>> {
+    pub fn list_series<'a>(&self, ctx: &'a CommandContext) -> Result<Vec<Series<'a>>> {
         if !self.initialized() {
             return Err(anyhow!("Stack has not been initialized"));
         }
@@ -477,20 +478,13 @@ impl Stack {
             let mut local_patches = vec![];
             for commit in repo
                 .log(head_commit, LogUntil::Commit(previous_head), false)?
-                .iter()
+                .into_iter()
                 .rev()
             {
-                let id: CommitOrChangeId = commit.clone().into();
+                local_patches.push(commit);
-                if local_patches.contains(&id) {
-                    // Duplication - use the commit id instead
-                    local_patches.push(CommitOrChangeId::CommitId(commit.id().to_string()));
-                } else {
-                    local_patches.push(id);
-                }
             }
-            dbg!(&local_patches);
 
-            let mut remote_patches: Vec<CommitOrChangeId> = vec![];
+            let mut remote_patches: Vec<Commit<'_>> = vec![];
             let mut remote_commit_ids_by_change_id: HashMap<String, git2::Oid> = HashMap::new();
             let remote_name = default_target.push_remote_name();
             if head.pushed(&remote_name, ctx).unwrap_or_default() {
@@ -505,8 +499,7 @@ impl Stack {
                         if let Some(change_id) = c.change_id() {
                             remote_commit_ids_by_change_id.insert(change_id.to_string(), c.id());
                         }
-                        let commit_or_change_id: CommitOrChangeId = c.into();
+                        remote_patches.push(c);
-                        remote_patches.push(commit_or_change_id);
                     });
             }
 
@@ -515,11 +508,10 @@ impl Stack {
                 .log(head_commit, LogUntil::Commit(merge_base), true)?
                 .into_iter()
                 .rev() // oldest commit first
-                .map(|c| c.into())
                 .collect_vec();
             let mut upstream_only = vec![];
             for patch in remote_patches.iter() {
-                if !local_patches_including_merge.contains(patch) {
+                if !local_patches_including_merge.contains_by_commit_or_change_id(patch) {
                     upstream_only.push(patch.clone());
                 }
             }

crates/gitbutler-stack/tests/mod.rs

@@ -644,12 +644,10 @@ fn list_series_default_head() -> Result<()> {
     // the number of series matches the number of heads
     assert_eq!(result.len(), test_ctx.branch.heads.len());
     assert_eq!(result[0].head.name, "a-branch-2");
-    let expected_patches = test_ctx
+    assert_eq!(
-        .commits
+        result[0].local_commits.iter().map(|c| c.id()).collect_vec(),
-        .iter()
+        test_ctx.commits.iter().map(|c| c.id()).collect_vec()
-        .map(|c| CommitOrChangeId::ChangeId(c.change_id().unwrap()))
+    );
-        .collect_vec();
-    assert_eq!(result[0].local_commits, expected_patches);
     Ok(())
 }
 
@@ -674,15 +672,15 @@ fn list_series_two_heads_same_commit() -> Result<()> {
     // the number of series matches the number of heads
     assert_eq!(result.len(), test_ctx.branch.heads.len());
 
-    let expected_patches = test_ctx
+    assert_eq!(
-        .commits
+        result[0].local_commits.iter().map(|c| c.id()).collect_vec(),
-        .iter()
+        test_ctx.commits.iter().map(|c| c.id()).collect_vec()
-        .map(|c| CommitOrChangeId::ChangeId(c.change_id().unwrap()))
+    );
-        .collect_vec();
-    // Expect the commits to be part of the `head_before`
-    assert_eq!(result[0].local_commits, expected_patches);
     assert_eq!(result[0].head.name, "head_before");
-    assert_eq!(result[1].local_commits, vec![]);
+    assert_eq!(
+        result[1].local_commits.iter().map(|c| c.id()).collect_vec(),
+        vec![]
+    );
     assert_eq!(result[1].head.name, "a-branch-2");
     Ok(())
 }
@@ -706,15 +704,17 @@ fn list_series_two_heads_different_commit() -> Result<()> {
     let result = result?;
     // the number of series matches the number of heads
     assert_eq!(result.len(), test_ctx.branch.heads.len());
-    let mut expected_patches = test_ctx
+    let mut expected_patches = test_ctx.commits.iter().map(|c| c.id()).collect_vec();
-        .commits
+    assert_eq!(
-        .iter()
+        result[0].local_commits.iter().map(|c| c.id()).collect_vec(),
-        .map(|c| CommitOrChangeId::ChangeId(c.change_id().unwrap()))
+        vec![expected_patches.remove(0)]
-        .collect_vec();
+    );
-    assert_eq!(result[0].local_commits, vec![expected_patches.remove(0)]); // the first patch is in the first series
     assert_eq!(result[0].head.name, "head_before");
     assert_eq!(expected_patches.len(), 2);
-    assert_eq!(result[1].local_commits, expected_patches); // the other two patches are in the second series
+    assert_eq!(
+        result[1].local_commits.iter().map(|c| c.id()).collect_vec(),
+        expected_patches
+    ); // the other two patches are in the second series
     assert_eq!(result[1].head.name, "a-branch-2");
 
     Ok(())