diff --git a/crates/but-core/src/ref_metadata.rs b/crates/but-core/src/ref_metadata.rs index 44876df7ae..0c4f4fdb32 100644 --- a/crates/but-core/src/ref_metadata.rs +++ b/crates/but-core/src/ref_metadata.rs @@ -36,8 +36,27 @@ pub struct Workspace { pub push_remote: Option, } +impl Workspace {} + /// Mutations impl Workspace { + /// Remove the named segment `branch`, which removes the whole stack if it's empty after removing a segment + /// of that name. + /// Returns `true` if it was removed or `false` if it wasn't found. + pub fn remove_segment(&mut self, branch: &FullNameRef) -> bool { + let Some((stack_idx, segment_idx)) = self.find_owner_indexes_by_name(branch) else { + return false; + }; + + let stack = &mut self.stacks[stack_idx]; + stack.branches.remove(segment_idx); + + if stack.branches.is_empty() { + self.stacks.remove(stack_idx); + } + true + } + /// Insert `branch` as new stack if it's not yet contained in the workspace and if `order` is not `None` or push /// it to the end of the stack list. /// Note that `order` is only relevant at insertion time. @@ -68,6 +87,28 @@ impl Workspace { } true } + + /// Insert `branch` as new segment if it's not yet contained in the workspace, + /// and insert it above the given `anchor` segment name, which maybe the tip of a stack or any segment within one + /// Returns `true` if the ref was newly added, or `false` if it already existed, or `None` if `anchor` didn't exist. + pub fn insert_new_segment_above_anchor_if_not_present( + &mut self, + branch: &FullNameRef, + anchor: &FullNameRef, + ) -> Option { + if self.contains_ref(branch) { + return Some(false); + }; + let (stack_idx, segment_idx) = self.find_owner_indexes_by_name(anchor)?; + self.stacks[stack_idx].branches.insert( + segment_idx, + WorkspaceStackBranch { + ref_name: branch.to_owned(), + archived: false, + }, + ); + Some(true) + } } /// Access diff --git a/crates/but-core/tests/core/ref_metadata.rs b/crates/but-core/tests/core/ref_metadata.rs index 39ff6bd565..09ff9edf37 100644 --- a/crates/but-core/tests/core/ref_metadata.rs +++ b/crates/but-core/tests/core/ref_metadata.rs @@ -2,7 +2,7 @@ mod workspace { use but_core::ref_metadata::Workspace; #[test] - fn add_new_stack_if_not_present() { + fn add_new_stack_if_not_present_journey() { let mut ws = Workspace::default(); assert_eq!(ws.stacks.len(), 0); @@ -18,6 +18,100 @@ mod workspace { let c_ref = r("refs/heads/C"); assert!(ws.add_or_insert_new_stack_if_not_present(c_ref, None)); assert_eq!(ws.stack_names().collect::>(), [b_ref, a_ref, c_ref]); + + assert!(ws.remove_segment(a_ref)); + assert!(ws.remove_segment(b_ref)); + assert!(!ws.remove_segment(b_ref)); + assert!(ws.remove_segment(c_ref)); + assert!(!ws.remove_segment(c_ref)); + + // Everything should be removed. + insta::assert_debug_snapshot!(ws, @r" + Workspace { + ref_info: RefInfo { created_at: None, updated_at: None }, + stacks: [], + target_ref: None, + push_remote: None, + } + "); + } + + #[test] + fn insert_new_segment_above_anchor_if_not_present_journey() { + let mut ws = Workspace::default(); + assert_eq!(ws.stacks.len(), 0); + + let a_ref = r("refs/heads/A"); + let b_ref = r("refs/heads/B"); + assert_eq!( + ws.insert_new_segment_above_anchor_if_not_present(b_ref, a_ref), + None, + "anchor doesn't exist" + ); + assert!(ws.add_or_insert_new_stack_if_not_present(a_ref, None)); + assert_eq!( + ws.insert_new_segment_above_anchor_if_not_present(b_ref, a_ref), + Some(true), + "anchor existed and it was added" + ); + assert_eq!( + ws.insert_new_segment_above_anchor_if_not_present(b_ref, a_ref), + Some(false), + "anchor existed and it was NOT added as it already existed" + ); + + let c_ref = r("refs/heads/C"); + assert_eq!( + ws.insert_new_segment_above_anchor_if_not_present(c_ref, a_ref), + Some(true) + ); + + insta::assert_snapshot!(but_testsupport::sanitize_uuids_and_timestamps(format!("{ws:#?}")), @r#" + Workspace { + ref_info: RefInfo { created_at: None, updated_at: None }, + stacks: [ + WorkspaceStack { + id: 1, + branches: [ + WorkspaceStackBranch { + ref_name: FullName( + "refs/heads/B", + ), + archived: false, + }, + WorkspaceStackBranch { + ref_name: FullName( + "refs/heads/C", + ), + archived: false, + }, + WorkspaceStackBranch { + ref_name: FullName( + "refs/heads/A", + ), + archived: false, + }, + ], + }, + ], + target_ref: None, + push_remote: None, + } + "#); + + assert!(ws.remove_segment(b_ref)); + assert!(ws.remove_segment(a_ref)); + assert!(ws.remove_segment(c_ref)); + + // Everything should be removed. + insta::assert_debug_snapshot!(ws, @r" + Workspace { + ref_info: RefInfo { created_at: None, updated_at: None }, + stacks: [], + target_ref: None, + push_remote: None, + } + "); } fn r(name: &str) -> &gix::refs::FullNameRef { diff --git a/crates/but-testing/src/args.rs b/crates/but-testing/src/args.rs index 98eb12d751..3f1eb7b99e 100644 --- a/crates/but-testing/src/args.rs +++ b/crates/but-testing/src/args.rs @@ -36,6 +36,14 @@ pub struct Args { #[derive(Debug, clap::Subcommand)] pub enum Subcommands { + /// Make an existing branch appear in the workspace. + Apply { + /// The 0-based place in the worktree into which the branch should be inserted. + #[clap(long, short = 'o')] + order: Option, + /// The name of the branch to apply to the workspace, like `feature` or `origin/other`. + branch_name: String, + }, /// Add the given Git repository as project for use with GitButler. AddProject { /// The long name of the remote reference to track, like `refs/remotes/origin/main`, diff --git a/crates/but-testing/src/command/mod.rs b/crates/but-testing/src/command/mod.rs index cf4917cc6c..e51aa2aa00 100644 --- a/crates/but-testing/src/command/mod.rs +++ b/crates/but-testing/src/command/mod.rs @@ -3,6 +3,9 @@ use but_core::UnifiedDiff; use but_db::poll::ItemKind; use but_graph::VirtualBranchesTomlMetadata; use but_settings::AppSettings; +use but_workspace::branch::OnWorkspaceMergeConflict; +use but_workspace::branch::apply::{IntegrationMode, WorkspaceReferenceNaming}; +use but_workspace::branch::checkout::UncommitedWorktreeChanges; use but_workspace::branch::create_reference::{Anchor, Position}; use but_workspace::{DiffSpec, HunkHeader}; use gitbutler_project::{Project, ProjectId}; @@ -634,6 +637,32 @@ pub fn remove_reference( Ok(()) } +pub fn apply(args: &super::Args, short_name: &str, order: Option) -> anyhow::Result<()> { + let (repo, project, graph, mut meta) = + repo_and_maybe_project_and_graph(args, RepositoryOpenMode::Merge)?; + let branch = repo.find_reference(short_name)?; + let ws = graph.to_workspace()?; + _ = but_workspace::branch::apply( + branch.name(), + &ws, + &repo, + &mut *meta, + but_workspace::branch::apply::Options { + integration_mode: IntegrationMode::AlwaysMerge, + on_workspace_conflict: OnWorkspaceMergeConflict::MaterializeAndReportConflictingStacks, + workspace_reference_naming: WorkspaceReferenceNaming::Default, + uncommitted_changes: UncommitedWorktreeChanges::KeepAndAbortOnConflict, + order, + }, + )?; + + if project.is_some() { + // write metadata if there are projects - this is a special case while we use vb.toml. + ManuallyDrop::into_inner(meta); + } + Ok(()) +} + pub fn create_reference( args: &super::Args, short_name: &str, diff --git a/crates/but-testing/src/main.rs b/crates/but-testing/src/main.rs index 7a6a110c4b..58efe67c6c 100644 --- a/crates/but-testing/src/main.rs +++ b/crates/but-testing/src/main.rs @@ -7,6 +7,7 @@ use command::parse_diff_spec; use gix::bstr::BString; mod args; +use crate::args::Subcommands; use crate::command::{RepositoryOpenMode, repo_and_maybe_project}; use args::Args; @@ -31,6 +32,7 @@ async fn main() -> Result<()> { } match &args.cmd { + Subcommands::Apply { branch_name, order } => command::apply(&args, branch_name, *order), args::Subcommands::AddProject { switch_to_workspace, path, @@ -39,7 +41,6 @@ async fn main() -> Result<()> { path.to_owned(), switch_to_workspace.to_owned(), ), - args::Subcommands::RemoveReference { permit_empty_stacks, keep_metadata, diff --git a/crates/but-workspace/src/branch/apply.rs b/crates/but-workspace/src/branch/apply.rs index 287fd850fc..439f4cc002 100644 --- a/crates/but-workspace/src/branch/apply.rs +++ b/crates/but-workspace/src/branch/apply.rs @@ -157,6 +157,7 @@ pub(crate) mod function { repo, checkout::Options { uncommitted_changes, + skip_head_update: false, }, )?; let graph = workspace.graph.redo_traversal_with_overlay( @@ -335,8 +336,9 @@ pub(crate) mod function { ); } // At this point, the workspace-metadata already knows the new branch(es), but the workspace itself - // doesn't see one or more of to-be-applied branches. These are, however, part of the graph by now, - // and we want to try to create a workspace merge. + // doesn't see one or more of to-be-applied branches (to become stacks). + // These are, however, part of the graph by now, and we want to try to create a workspace + // merge. let mut in_memory_repo = repo.clone().for_tree_diffing()?.with_object_memory(); let merge_result = WorkspaceCommit::from_new_merge_with_metadata( &ws_md.stacks, @@ -353,6 +355,110 @@ pub(crate) mod function { }); } + let prev_head_id = graph + .entrypoint_commit() + .context("BUG: how is it possible that there is no head commit?")? + .id; + let mut new_head_id = merge_result.workspace_commit_id; + let overlay = + overlay.with_entrypoint(new_head_id, Some(workspace_ref_name_to_update.clone())); + let mut graph = + graph.redo_traversal_with_overlay(&in_memory_repo, meta, overlay.clone())?; + + let workspace = graph.to_workspace()?; + let collect_unapplied_branches = |workspace: &but_graph::projection::Workspace| { + branches_to_apply + .iter() + .filter(|rn| workspace.refname_is_segment(rn)) + .collect::>() + }; + let unapplied_branches = collect_unapplied_branches(&workspace); + if !unapplied_branches.is_empty() { + // Now that the merge is done, try to redo the operation one last time with dependent branches instead. + // Only do that for the still unapplied branches, which should always find some sort of anchor. + let ws_mut: &mut Workspace = &mut ws_md; + for branch_to_remove in &unapplied_branches { + ws_mut.remove_segment(branch_to_remove); + } + for rn in &unapplied_branches { + // Here we have to check if the new ref would be able to become its own stack, + // or if it has to be a dependent branch. Stacks only work if the ref rests on a base + // outside the workspace, so if we find it in the workspace (in an ambiguous spot) it must be + // a dependent branch + if let Some(segment_to_insert_above) = workspace + .stacks + .iter() + .flat_map(|stack| stack.segments.iter()) + .find_map(|segment| { + segment.commits.iter().flat_map(|c| c.refs.iter()).find_map( + |ambiguous_rn| { + (ambiguous_rn.as_ref() == **rn) + .then_some(segment.ref_name.as_ref()) + .flatten() + }, + ) + }) + { + if ws_mut + .insert_new_segment_above_anchor_if_not_present( + rn, + segment_to_insert_above.as_ref(), + ) + .is_none() + { + // For now bail, until we know it's worth fixing this case automatically. + bail!( + "Missing reference {segment_to_insert_above} which should be known to workspace metadata to serve as insertion position for {rn}" + ); + } + } else { + bail!( + "Unexpectedly failed to find anchor for {rn} to make it a dependent branch" + ) + } + } + + // Redo the merge, with the different stack configuration. + // Note that this is the exception, typically using stacks will be fine. + let merge_result = WorkspaceCommit::from_new_merge_with_metadata( + &ws_md.stacks, + workspace.graph, + &in_memory_repo, + Some(branch), + )?; + + if merge_result.has_conflicts() && on_workspace_conflict.should_abort() { + return Ok(Outcome { + graph: Cow::Owned(graph), + workspace_ref_created: needs_ws_ref_creation, + workspace_merge: Some(merge_result), + }); + } + new_head_id = merge_result.workspace_commit_id; + let ws_md_override = Some((workspace_ref_name_to_update.clone(), (*ws_md).clone())); + + graph = graph.redo_traversal_with_overlay( + &in_memory_repo, + meta, + overlay + .with_entrypoint(new_head_id, Some(workspace_ref_name_to_update.clone())) + .with_workspace_metadata_override(ws_md_override), + )?; + let workspace = graph.to_workspace()?; + let unapplied_branches = collect_unapplied_branches(&workspace); + + if !unapplied_branches.is_empty() { + bail!( + "Unexpectedly failed to apply {branches} which is/are still not in the workspace", + branches = unapplied_branches + .iter() + .map(|rn| rn.shorten().to_string()) + .collect::>() + .join(", ") + ) + } + } + // All work is done, persist and exit. // Note that it could be that some stacks aren't merged in, // while being present in the workspace metadata. @@ -361,16 +467,6 @@ pub(crate) mod function { storage.persist(repo)?; drop(in_memory_repo); } - let prev_head_id = graph - .entrypoint_commit() - .context("BUG: how is it possible that there is no head commit?")? - .id; - let new_head_id = merge_result.workspace_commit_id; - let graph = graph.redo_traversal_with_overlay( - repo, - meta, - overlay.with_entrypoint(new_head_id, Some(workspace_ref_name_to_update.clone())), - )?; persist_metadata(meta, &branches_to_apply, &ws_md)?; crate::branch::safe_checkout( prev_head_id, @@ -378,6 +474,7 @@ pub(crate) mod function { repo, checkout::Options { uncommitted_changes, + skip_head_update: true, }, )?; @@ -416,10 +513,9 @@ pub(crate) mod function { new_ref_target: gix::ObjectId, new_ref: Option<&gix::refs::FullNameRef>, ) -> anyhow::Result<()> { - // This also means we want HEAD to point to it. - let head_message = "GitButler switch to workspace during apply-branch".into(); let edits = match new_ref { None => { + let head_message = "GitButler checkout workspace during apply-branch".into(); vec![RefEdit { change: Change::Update { log: LogChange { @@ -435,6 +531,8 @@ pub(crate) mod function { }] } Some(new_ref) => { + // This also means we want HEAD to point to it. + let head_message = "GitButler switch to workspace during apply-branch".into(); vec![ RefEdit { change: Change::Update { diff --git a/crates/but-workspace/src/branch/checkout/function.rs b/crates/but-workspace/src/branch/checkout/function.rs index e6b2507bf9..9af03df3c8 100644 --- a/crates/but-workspace/src/branch/checkout/function.rs +++ b/crates/but-workspace/src/branch/checkout/function.rs @@ -50,6 +50,7 @@ pub fn safe_checkout( repo: &gix::Repository, Options { uncommitted_changes: conflicting_worktree_changes_opts, + skip_head_update, }: Options, ) -> anyhow::Result { let source_tree = current_head_id.attach(repo).object()?.peel_to_tree()?; @@ -138,7 +139,7 @@ pub fn safe_checkout( } let mut head_update = None; - if new_object.kind.is_commit() { + if new_object.kind.is_commit() && !skip_head_update { let needs_update = repo .head()? .id() diff --git a/crates/but-workspace/src/branch/checkout/mod.rs b/crates/but-workspace/src/branch/checkout/mod.rs index 2fe2c34894..764874582c 100644 --- a/crates/but-workspace/src/branch/checkout/mod.rs +++ b/crates/but-workspace/src/branch/checkout/mod.rs @@ -17,6 +17,10 @@ pub enum UncommitedWorktreeChanges { pub struct Options { /// How to deal with uncommitted changes. pub uncommitted_changes: UncommitedWorktreeChanges, + /// If `true`, do not change `HEAD` to the new commit. + /// + /// This is typically to be avoided, but may be used if you want to change the HEAD location yourself. + pub skip_head_update: bool, } /// The successful outcome of [super::safe_checkout()] operation. diff --git a/crates/but-workspace/tests/fixtures/scenario/no-ws-ref-stack-and-dependent-branch.sh b/crates/but-workspace/tests/fixtures/scenario/no-ws-ref-stack-and-dependent-branch.sh new file mode 100644 index 0000000000..638aa56608 --- /dev/null +++ b/crates/but-workspace/tests/fixtures/scenario/no-ws-ref-stack-and-dependent-branch.sh @@ -0,0 +1,21 @@ +#!/usr/bin/env bash + +set -eu -o pipefail + +source "${BASH_SOURCE[0]%/*}/shared.sh" + +### General Description + +# A target branch along with a stack and a depdendent branch at the same tip, without workspace commit. +git init +commit M +setup_target_to_match_main +git checkout -b A +commit A1 +git branch D +git branch E +commit A2 +git branch B +git branch C + +git checkout main diff --git a/crates/but-workspace/tests/workspace/branch/apply_unapply_commit_uncommit.rs b/crates/but-workspace/tests/workspace/branch/apply_unapply_commit_uncommit.rs index 16aa00331b..712d494634 100644 --- a/crates/but-workspace/tests/workspace/branch/apply_unapply_commit_uncommit.rs +++ b/crates/but-workspace/tests/workspace/branch/apply_unapply_commit_uncommit.rs @@ -521,6 +521,84 @@ fn detached_head_journey() -> anyhow::Result<()> { Ok(()) } +#[test] +fn apply_two_ambiguous_stacks_with_target() -> anyhow::Result<()> { + let (_tmp, graph, repo, mut meta, _description) = + named_writable_scenario_with_description_and_graph( + "no-ws-ref-stack-and-dependent-branch", + |_meta| {}, + )?; + insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" + * f084d61 (C, B, A) A2 + * 7076dee (E, D) A1 + * 85efbe4 (HEAD -> main, origin/main) M + "); + + let ws = graph.to_workspace()?; + insta::assert_snapshot!(graph_workspace(&ws), @r" + ⌂:0:main <> ✓! + └── ≡:0:main + └── :0:main + └── ·85efbe4 + "); + + // Apply `A` first. + let out = + but_workspace::branch::apply(r("refs/heads/A"), &ws, &repo, &mut meta, default_options())?; + insta::assert_debug_snapshot!(out, @r" + Outcome { + workspace_changed: true, + workspace_ref_created: true, + } + "); + let graph = out.graph; + let ws = graph.to_workspace()?; + insta::assert_snapshot!(graph_workspace(&ws), @r" + 📕🏘️:0:gitbutler/workspace <> ✓refs/remotes/origin/main on 85efbe4 + └── ≡📙:3:A on 85efbe4 + └── 📙:3:A + ├── ·f084d61 (🏘️) ►B, ►C + └── ·7076dee (🏘️) ►D, ►E + "); + insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" + * 6a706b7 (HEAD -> gitbutler/workspace) GitButler Workspace Commit + * f084d61 (C, B, A) A2 + * 7076dee (E, D) A1 + * 85efbe4 (origin/main, main) M + "); + + // Apply `B` - the only sane way is to make it its own stack, but allow it to diverge. + let out = + but_workspace::branch::apply(r("refs/heads/B"), &ws, &repo, &mut meta, default_options()) + .expect("apply actually works"); + insta::assert_debug_snapshot!(out, @r" + Outcome { + workspace_changed: true, + workspace_ref_created: false, + } + "); + + let graph = out.graph; + let ws = graph.to_workspace()?; + insta::assert_snapshot!(graph_workspace(&ws), @r" + 📕🏘️:0:gitbutler/workspace <> ✓refs/remotes/origin/main on 85efbe4 + └── ≡📙:4:B on 85efbe4 + ├── 📙:4:B + └── 📙:5:A + ├── ·f084d61 (🏘️) ►C + └── ·7076dee (🏘️) ►D, ►E + "); + insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" + * badd1b4 (HEAD -> gitbutler/workspace) GitButler Workspace Commit + * f084d61 (C, B, A) A2 + * 7076dee (E, D) A1 + * 85efbe4 (origin/main, main) M + "); + + // TODO: add all other dependent branches as well. + Ok(()) +} + #[test] fn auto_checkout_of_enclosing_workspace_flat() -> anyhow::Result<()> { let (_tmp, graph, repo, mut meta, _description) = diff --git a/crates/but-workspace/tests/workspace/branch/checkout.rs b/crates/but-workspace/tests/workspace/branch/checkout.rs index 8ebf456c9e..fcd780f9db 100644 --- a/crates/but-workspace/tests/workspace/branch/checkout.rs +++ b/crates/but-workspace/tests/workspace/branch/checkout.rs @@ -827,6 +827,7 @@ fn unrelated_additions_do_not_affect_worktree_changes() -> anyhow::Result<()> { fn overwrite_options() -> checkout::Options { checkout::Options { uncommitted_changes: UncommitedWorktreeChanges::KeepConflictingInSnapshotAndOverwrite, + skip_head_update: false, } } diff --git a/crates/gitbutler-branch-actions/src/base.rs b/crates/gitbutler-branch-actions/src/base.rs index 1ef95769fa..10af619f14 100644 --- a/crates/gitbutler-branch-actions/src/base.rs +++ b/crates/gitbutler-branch-actions/src/base.rs @@ -70,6 +70,7 @@ fn go_back_to_integration(ctx: &CommandContext, default_target: &Target) -> Resu &gix_repo, but_workspace::branch::checkout::Options { uncommitted_changes: UncommitedWorktreeChanges::KeepAndAbortOnConflict, + skip_head_update: false, }, )?; } else { diff --git a/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs b/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs index 75281a63ea..e7799d036c 100644 --- a/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs +++ b/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs @@ -168,7 +168,7 @@ impl BranchManager<'_> { let applied_branch_stack_id = ws .find_segment_and_stack_by_refname(branch_to_apply) .with_context(|| - format!("Failed to apply branch and we probably have to handle more of the outcome\n{out:?}") + format!("BUG: Can't find the branch to apply in workspace, but the 'apply' function should have failed instead \n{out:?}") )? .0 .id