From 94e345aeb098116c63c1400a18b1fccb25ea24f4 Mon Sep 17 00:00:00 2001 From: Kiril Videlov Date: Wed, 30 Oct 2024 12:26:34 +0100 Subject: [PATCH 1/2] Change pruning of integrated heads in the stack to just flag them as archived --- crates/gitbutler-branch-actions/src/stack.rs | 1 + .../src/upstream_integration.rs | 2 +- crates/gitbutler-patch-reference/src/lib.rs | 4 ++ crates/gitbutler-stack/src/stack.rs | 16 +++--- crates/gitbutler-stack/tests/mod.rs | 50 +++++++++++++++---- 5 files changed, 53 insertions(+), 20 deletions(-) diff --git a/crates/gitbutler-branch-actions/src/stack.rs b/crates/gitbutler-branch-actions/src/stack.rs index 90712b685..a04c381fb 100644 --- a/crates/gitbutler-branch-actions/src/stack.rs +++ b/crates/gitbutler-branch-actions/src/stack.rs @@ -46,6 +46,7 @@ pub fn create_series( name: req.name, description: req.description, forge_id: Default::default(), + archived: Default::default(), }, req.preceding_head, ) diff --git a/crates/gitbutler-branch-actions/src/upstream_integration.rs b/crates/gitbutler-branch-actions/src/upstream_integration.rs index fcab10fa5..4c29d6780 100644 --- a/crates/gitbutler-branch-actions/src/upstream_integration.rs +++ b/crates/gitbutler-branch-actions/src/upstream_integration.rs @@ -322,7 +322,7 @@ pub(crate) fn integrate_upstream( }; branch.set_stack_head(command_context, *head, Some(*tree))?; - branch.prune_integrated_heads(command_context)?; + branch.archive_integrated_heads(command_context)?; } // checkout_branch_trees won't checkout anything if there are no diff --git a/crates/gitbutler-patch-reference/src/lib.rs b/crates/gitbutler-patch-reference/src/lib.rs index 9a0879835..f5ae3ddb2 100644 --- a/crates/gitbutler-patch-reference/src/lib.rs +++ b/crates/gitbutler-patch-reference/src/lib.rs @@ -21,6 +21,10 @@ pub struct PatchReference { /// None if is no review unit, eg. no Pull Request has been created. #[serde(default)] pub forge_id: Option, + /// Archived represents the state when series/branch has been integrated and is below the merge base of the branch. + /// This would occur when the branch has been merged at the remote and the workspace has been updated with that change. + #[serde(default)] + pub archived: bool, } /// Represents identifiers for the series at possible forges, eg. GitHub PR numbers. diff --git a/crates/gitbutler-stack/src/stack.rs b/crates/gitbutler-stack/src/stack.rs index 85cec08c1..bca1fe1ec 100644 --- a/crates/gitbutler-stack/src/stack.rs +++ b/crates/gitbutler-stack/src/stack.rs @@ -231,6 +231,7 @@ impl Stack { }, description: None, forge_id: Default::default(), + archived: Default::default(), }; let state = branch_state(ctx); @@ -305,6 +306,7 @@ impl Stack { name, description, forge_id: Default::default(), + archived: Default::default(), }; self.add_series(ctx, new_head, Some(current_top_head.name.clone())) } @@ -438,20 +440,18 @@ impl Stack { } /// Removes any heads that are refering to commits that are no longer between the stack head and the merge base - pub fn prune_integrated_heads(&mut self, ctx: &CommandContext) -> Result<()> { + pub fn archive_integrated_heads(&mut self, ctx: &CommandContext) -> Result<()> { if !self.initialized() { return Err(anyhow!("Stack has not been initialized")); } self.updated_timestamp_ms = gitbutler_time::time::now_ms(); let state = branch_state(ctx); let commit_ids = stack_patches(ctx, &state, self.head(), true)?; - let new_heads = self - .heads - .iter() - .filter(|h| commit_ids.contains(&h.target)) - .cloned() - .collect_vec(); - self.heads = new_heads; + for head in self.heads.iter_mut() { + if !commit_ids.contains(&head.target) { + head.archived = true; + } + } state.set_branch(self.clone()) } diff --git a/crates/gitbutler-stack/tests/mod.rs b/crates/gitbutler-stack/tests/mod.rs index 27e377b92..1024eaa12 100644 --- a/crates/gitbutler-stack/tests/mod.rs +++ b/crates/gitbutler-stack/tests/mod.rs @@ -61,6 +61,7 @@ fn add_series_success() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: Some("my description".into()), forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference, None); assert!(result.is_ok()); @@ -116,6 +117,7 @@ fn add_series_top_base() -> Result<()> { target: CommitOrChangeId::CommitId(merge_base.id().to_string()), description: Some("my description".into()), forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference, None); println!("{:?}", result); @@ -142,6 +144,7 @@ fn add_multiple_series() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits.last().unwrap().change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx .branch @@ -154,6 +157,7 @@ fn add_multiple_series() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits.last().unwrap().change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, head_2, None); assert!(result.is_ok()); @@ -167,6 +171,7 @@ fn add_multiple_series() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits.first().unwrap().change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, head_1, None); @@ -176,9 +181,9 @@ fn add_multiple_series() -> Result<()> { vec!["head_1", "head_2", "a-branch-2", "head_4"] ); - // prune is noop + // archive is noop let before_prune = test_ctx.branch.heads.clone(); - test_ctx.branch.prune_integrated_heads(&ctx)?; + test_ctx.branch.archive_integrated_heads(&ctx)?; assert_eq!(before_prune, test_ctx.branch.heads); Ok(()) } @@ -193,6 +198,7 @@ fn add_series_commit_id_when_change_id_available() -> Result<()> { target: CommitOrChangeId::CommitId(test_ctx.commits[1].id().to_string()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference, None); assert_eq!( @@ -215,6 +221,7 @@ fn add_series_invalid_name_fails() -> Result<()> { target: CommitOrChangeId::CommitId(test_ctx.commits[0].id().to_string()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference, None); assert_eq!(result.err().unwrap().to_string(), "Invalid branch name"); @@ -231,6 +238,7 @@ fn add_series_duplicate_name_fails() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference.clone(), None); assert!(result.is_ok()); @@ -252,6 +260,7 @@ fn add_series_matching_git_ref_is_ok() -> Result<()> { target: test_ctx.commits[0].clone().into(), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference.clone(), None); assert!(result.is_ok()); // allow this @@ -268,6 +277,7 @@ fn add_series_including_refs_head_fails() -> Result<()> { target: CommitOrChangeId::CommitId(test_ctx.commits[0].id().to_string()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference.clone(), None); assert_eq!( @@ -287,6 +297,7 @@ fn add_series_target_commit_doesnt_exist() -> Result<()> { target: CommitOrChangeId::CommitId("30696678319e0fa3a20e54f22d47fc8cf1ceaade".into()), // does not exist description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference.clone(), None); assert!(result @@ -307,6 +318,7 @@ fn add_series_target_change_id_doesnt_exist() -> Result<()> { target: CommitOrChangeId::ChangeId("does-not-exist".into()), // does not exist description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference.clone(), None); assert_eq!( @@ -327,6 +339,7 @@ fn add_series_target_commit_not_in_stack() -> Result<()> { target: CommitOrChangeId::CommitId(other_commit_id.clone()), // does not exist description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, reference.clone(), None); assert_eq!( @@ -384,6 +397,7 @@ fn remove_series_with_multiple_last_heads() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits.last().unwrap().change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, to_stay.clone(), None); assert!(result.is_ok()); @@ -416,6 +430,7 @@ fn remove_series_no_orphan_commits() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits.first().unwrap().change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; // references the oldest commit let result = test_ctx.branch.add_series(&ctx, to_stay.clone(), None); assert!(result.is_ok()); @@ -579,6 +594,7 @@ fn update_series_target_success() -> Result<()> { target: commit_0_change_id.clone(), description: None, forge_id: Default::default(), + archived: Default::default(), }; let result = test_ctx.branch.add_series(&ctx, series_1, None); assert!(result.is_ok()); @@ -681,6 +697,7 @@ fn list_series_two_heads_same_commit() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits.last().unwrap().change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; // add `head_before` before the initial head let result = test_ctx.branch.add_series(&ctx, head_before, None); @@ -717,6 +734,7 @@ fn list_series_two_heads_different_commit() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits.first().unwrap().change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; // add `head_before` before the initial head let result = test_ctx.branch.add_series(&ctx, head_before, None); @@ -782,6 +800,7 @@ fn replace_head_single() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; test_ctx.branch.add_series(&ctx, from_head, None)?; // replace with previous head @@ -814,6 +833,7 @@ fn replace_head_single_with_merge_base() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; test_ctx.branch.add_series(&ctx, from_head, None)?; // replace with merge base @@ -850,6 +870,7 @@ fn replace_head_with_invalid_commit_error() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; test_ctx.branch.add_series(&ctx, from_head, None)?; let stack = test_ctx.branch.clone(); @@ -877,6 +898,7 @@ fn replace_head_with_same_noop() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; test_ctx.branch.add_series(&ctx, from_head, None)?; let stack = test_ctx.branch.clone(); @@ -963,12 +985,14 @@ fn replace_head_multiple() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; let from_head_2 = PatchReference { name: "from_head_2".into(), target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; // both references point to the same commit test_ctx.branch.add_series(&ctx, from_head_1, None)?; @@ -1009,6 +1033,7 @@ fn replace_head_top_of_stack_multiple() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; // an extra head just beneath the top of the stack test_ctx.branch.add_series(&ctx, extra_head, None)?; @@ -1075,6 +1100,7 @@ fn set_legacy_refname_multiple_heads() -> Result<()> { target: CommitOrChangeId::ChangeId(test_ctx.commits[1].change_id().unwrap()), description: None, forge_id: Default::default(), + archived: Default::default(), }; // an extra head just beneath the top of the stack test_ctx.branch.add_series(&ctx, extra_head, None)?; @@ -1117,12 +1143,12 @@ fn set_legacy_refname_pushed() -> Result<()> { } #[test] -fn prune_heads_noop() -> Result<()> { +fn archive_heads_noop() -> Result<()> { let (ctx, _temp_dir) = command_ctx("multiple-commits")?; let mut test_ctx = test_ctx(&ctx)?; test_ctx.branch.initialize(&ctx)?; let initial_state = test_ctx.branch.heads.clone(); - test_ctx.branch.prune_integrated_heads(&ctx)?; + test_ctx.branch.archive_integrated_heads(&ctx)?; assert_eq!(initial_state, test_ctx.branch.heads); // Assert persisted assert_eq!( @@ -1133,11 +1159,10 @@ fn prune_heads_noop() -> Result<()> { } #[test] -fn prune_heads_success() -> Result<()> { +fn archive_heads_success() -> Result<()> { let (ctx, _temp_dir) = command_ctx("multiple-commits")?; let mut test_ctx = test_ctx(&ctx)?; test_ctx.branch.initialize(&ctx)?; - let initial_state = test_ctx.branch.heads.clone(); // adding a commit that is not in the stack test_ctx.branch.heads.insert( 0, @@ -1146,12 +1171,14 @@ fn prune_heads_success() -> Result<()> { name: "foo".to_string(), description: None, forge_id: Default::default(), + archived: Default::default(), }, ); assert_eq!(test_ctx.branch.heads.len(), 2); - test_ctx.branch.prune_integrated_heads(&ctx)?; - assert_eq!(test_ctx.branch.heads.len(), 1); - assert_eq!(initial_state, test_ctx.branch.heads); + test_ctx.branch.archive_integrated_heads(&ctx)?; + assert_eq!(test_ctx.branch.heads.len(), 2); + assert!(test_ctx.branch.heads[0].archived); + assert!(!test_ctx.branch.heads[1].archived); // Assert persisted assert_eq!( test_ctx.branch, @@ -1161,7 +1188,7 @@ fn prune_heads_success() -> Result<()> { } #[test] -fn does_not_prune_head_on_merge_base() -> Result<()> { +fn does_not_archive_head_on_merge_base() -> Result<()> { let (ctx, _temp_dir) = command_ctx("multiple-commits")?; let mut test_ctx = test_ctx(&ctx)?; test_ctx.branch.initialize(&ctx)?; @@ -1176,11 +1203,12 @@ fn does_not_prune_head_on_merge_base() -> Result<()> { name: "bottom".to_string(), description: None, forge_id: Default::default(), + archived: Default::default(), }, None, )?; let initial_state = test_ctx.branch.heads.clone(); - test_ctx.branch.prune_integrated_heads(&ctx)?; + test_ctx.branch.archive_integrated_heads(&ctx)?; assert_eq!(initial_state, test_ctx.branch.heads); // Assert persisted assert_eq!( From bd10692afb1d51f68c9681e6a2ddeb90d8a9ef09 Mon Sep 17 00:00:00 2001 From: Kiril Videlov Date: Wed, 30 Oct 2024 12:52:55 +0100 Subject: [PATCH 2/2] Only show non-archived branches in the UI --- apps/desktop/src/lib/stack/StackSeries.svelte | 4 ++- apps/desktop/src/lib/vbranches/types.ts | 5 ++++ crates/gitbutler-branch-actions/src/stack.rs | 1 + .../gitbutler-branch-actions/src/virtual.rs | 3 +++ crates/gitbutler-stack/src/series.rs | 3 +++ crates/gitbutler-stack/src/stack.rs | 27 ++++++++++--------- 6 files changed, 29 insertions(+), 14 deletions(-) diff --git a/apps/desktop/src/lib/stack/StackSeries.svelte b/apps/desktop/src/lib/stack/StackSeries.svelte index cfbc9afd6..acf7784d5 100644 --- a/apps/desktop/src/lib/stack/StackSeries.svelte +++ b/apps/desktop/src/lib/stack/StackSeries.svelte @@ -18,13 +18,15 @@ branch.series.flatMap((s) => s.patches).some((patch) => patch.conflicted) ); + const nonArchivedSeries = $derived(branch.series.filter((s) => !s.archived)); + const reorderDropzoneManagerFactory = getContext(ReorderDropzoneManagerFactory); const reorderDropzoneManager = $derived( reorderDropzoneManagerFactory.build(branch, [...branch.localCommits, ...branch.remoteCommits]) ); -{#each branch.series as currentSeries, idx (currentSeries.name)} +{#each nonArchivedSeries as currentSeries, idx (currentSeries.name)} {@const isTopSeries = idx === 0} {#if !isTopSeries} diff --git a/apps/desktop/src/lib/vbranches/types.ts b/apps/desktop/src/lib/vbranches/types.ts index 81e090681..f7cc1345a 100644 --- a/apps/desktop/src/lib/vbranches/types.ts +++ b/apps/desktop/src/lib/vbranches/types.ts @@ -450,6 +450,11 @@ export class PatchSeries { * The list is empty if there is no review units, eg. no Pull Request has been created. */ forgeId?: ForgeIdentifier | undefined; + /** + * Archived represents the state when series/branch has been integrated and is below the merge base of the branch. + * This would occur when the branch has been merged at the remote and the workspace has been updated with that change. + */ + archived!: boolean; get localCommits() { return this.patches.filter((c) => c.status === 'local'); diff --git a/crates/gitbutler-branch-actions/src/stack.rs b/crates/gitbutler-branch-actions/src/stack.rs index a04c381fb..5a96fea80 100644 --- a/crates/gitbutler-branch-actions/src/stack.rs +++ b/crates/gitbutler-branch-actions/src/stack.rs @@ -284,6 +284,7 @@ pub(crate) fn stack_series( patches, upstream_patches, forge_id: series.head.forge_id, + archived: series.head.archived, }); } api_series.reverse(); diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index bf9f2b6be..8e719b901 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -100,6 +100,9 @@ pub struct PatchSeries { /// A list of identifiers for the review unit at possible forges (eg. Pull Request). /// The list is empty if there is no review units, eg. no Pull Request has been created. pub forge_id: Option, + /// Archived represents the state when series/branch has been integrated and is below the merge base of the branch. + /// This would occur when the branch has been merged at the remote and the workspace has been updated with that change. + pub archived: bool, } #[derive(Debug, PartialEq, Clone, Serialize)] diff --git a/crates/gitbutler-stack/src/series.rs b/crates/gitbutler-stack/src/series.rs index a6f199e68..f0b6c6827 100644 --- a/crates/gitbutler-stack/src/series.rs +++ b/crates/gitbutler-stack/src/series.rs @@ -28,6 +28,9 @@ pub struct Series<'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, + /// Archived represents the state when series/branch has been integrated and is below the merge base of the branch. + /// This would occur when the branch has been merged at the remote and the workspace has been updated with that change. + pub archived: bool, } impl Series<'_> { diff --git a/crates/gitbutler-stack/src/stack.rs b/crates/gitbutler-stack/src/stack.rs index bca1fe1ec..630865e78 100644 --- a/crates/gitbutler-stack/src/stack.rs +++ b/crates/gitbutler-stack/src/stack.rs @@ -498,19 +498,19 @@ impl Stack { let mut previous_head = repo.merge_base(self.head(), default_target.sha)?; for head in self.heads.clone() { let head_commit = - match commit_by_oid_or_change_id(&head.target, repo, self.head(), merge_base) { - Ok(commits_for_id) => commits_for_id.head.id(), - Err(e) => { - // The series may have been integrated - tracing::warn!( - "Failed to find commit with commit_or_change_id: {} for head: {}, {}", - head.target, - head.name, - e - ); - continue; - } - }; + commit_by_oid_or_change_id(&head.target, repo, self.head(), merge_base); + if head.archived || head_commit.is_err() { + all_series.push(Series { + head: head.clone(), + local_commits: vec![], + remote_commits: vec![], + upstream_only_commits: vec![], + remote_commit_ids_by_change_id: HashMap::new(), + archived: head.archived, + }); + continue; + } + let head_commit = head_commit?.head.id(); let mut local_patches = vec![]; for commit in repo @@ -559,6 +559,7 @@ impl Stack { remote_commits: remote_patches, upstream_only_commits: upstream_only, remote_commit_ids_by_change_id, + archived: head.archived, }); previous_head = head_commit; }