diff --git a/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs b/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs index dc8cd2c0f..b2d829d36 100644 --- a/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs +++ b/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, diff --git a/crates/gitbutler-branch-actions/src/stack.rs b/crates/gitbutler-branch-actions/src/stack.rs index 3c5b93247..00a37abd1 100644 --- a/crates/gitbutler-branch-actions/src/stack.rs +++ b/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 = 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 = 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() { - let commit = first_non_duplicate( - &patches, - commit_by_oid_or_change_id(patch, ctx.repository(), branch.head(), merge_base)?, - )?; + for commit in series.clone().local_commits.iter().rev() { 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() { - let remote_head = ctx - .repository() - .find_reference(&upstream_reference)? - .peel_to_commit()?; - for patch in series.upstream_only_commits { - if let Ok(commit) = commit_by_oid_or_change_id( - &patch, - ctx.repository(), - remote_head.id(), - merge_base, - ) { - 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); - }; - } + for commit in series.upstream_only_commits { + 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> { - 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")) - } -} diff --git a/crates/gitbutler-commit/src/commit_ext.rs b/crates/gitbutler-commit/src/commit_ext.rs index 7782a63e0..2f2a06848 100644 --- a/crates/gitbutler-commit/src/commit_ext.rs +++ b/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>, +{ + 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> { + fn contains_by_commit_or_change_id(&self, commit: &git2::Commit) -> bool { + contains(self.iter().cloned(), commit) + } +} diff --git a/crates/gitbutler-stack/src/series.rs b/crates/gitbutler-stack/src/series.rs index 85b04a4f1..a6f199e68 100644 --- a/crates/gitbutler-stack/src/series.rs +++ b/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)] -pub struct Series { +#[derive(Debug, Clone)] +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, + pub local_commits: Vec>, /// 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, + pub remote_commits: Vec>, /// 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, + pub upstream_only_commits: Vec>, /// 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, } -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 { - self.remote_commits.contains(patch) + pub fn remote(&self, patch: &Commit<'_>) -> bool { + self.remote_commits.contains_by_commit_or_change_id(patch) } } diff --git a/crates/gitbutler-stack/src/stack.rs b/crates/gitbutler-stack/src/stack.rs index 054545122..fd12637db 100644 --- a/crates/gitbutler-stack/src/stack.rs +++ b/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> { + pub fn list_series<'a>(&self, ctx: &'a CommandContext) -> Result>> { 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(); - 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); - } + local_patches.push(commit); } - dbg!(&local_patches); - let mut remote_patches: Vec = vec![]; + let mut remote_patches: Vec> = vec![]; let mut remote_commit_ids_by_change_id: HashMap = 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(commit_or_change_id); + remote_patches.push(c); }); } @@ -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()); } } diff --git a/crates/gitbutler-stack/tests/mod.rs b/crates/gitbutler-stack/tests/mod.rs index ed3dee5bc..89344559e 100644 --- a/crates/gitbutler-stack/tests/mod.rs +++ b/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 - .commits - .iter() - .map(|c| CommitOrChangeId::ChangeId(c.change_id().unwrap())) - .collect_vec(); - assert_eq!(result[0].local_commits, expected_patches); + assert_eq!( + result[0].local_commits.iter().map(|c| c.id()).collect_vec(), + test_ctx.commits.iter().map(|c| c.id()).collect_vec() + ); 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 - .commits - .iter() - .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].local_commits.iter().map(|c| c.id()).collect_vec(), + test_ctx.commits.iter().map(|c| c.id()).collect_vec() + ); 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 - .commits - .iter() - .map(|c| CommitOrChangeId::ChangeId(c.change_id().unwrap())) - .collect_vec(); - assert_eq!(result[0].local_commits, vec![expected_patches.remove(0)]); // the first patch is in the first series + let mut expected_patches = test_ctx.commits.iter().map(|c| c.id()).collect_vec(); + assert_eq!( + result[0].local_commits.iter().map(|c| c.id()).collect_vec(), + vec![expected_patches.remove(0)] + ); 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(())