diff --git a/crates/but-rebase/src/graph_rebase/mutate.rs b/crates/but-rebase/src/graph_rebase/mutate.rs index 6f4a29e5b23..743ee62bd97 100644 --- a/crates/but-rebase/src/graph_rebase/mutate.rs +++ b/crates/but-rebase/src/graph_rebase/mutate.rs @@ -250,6 +250,46 @@ impl Editor<'_, '_, M> { None } + /// Returns all direct children of `target` together with their edge order. + /// + /// Children are represented as incoming edges into `target` in the step graph. + pub fn direct_children(&self, target: impl ToSelector) -> Result> { + let target = self.history.normalize_selector(target.to_selector(self)?)?; + Ok(self + .graph + .edges_directed(target.id, Direction::Incoming) + .map(|edge| { + ( + Selector { + id: edge.source(), + revision: self.history.current_revision(), + }, + edge.weight().order, + ) + }) + .collect()) + } + + /// Returns all direct parents of `target` together with their edge order. + /// + /// Parents are represented as outgoing edges from `target` in the step graph. + pub fn direct_parents(&self, target: impl ToSelector) -> Result> { + let target = self.history.normalize_selector(target.to_selector(self)?)?; + Ok(self + .graph + .edges_directed(target.id, Direction::Outgoing) + .map(|edge| { + ( + Selector { + id: edge.target(), + revision: self.history.current_revision(), + }, + edge.weight().order, + ) + }) + .collect()) + } + /// Replaces the node that the function was pointing to. /// /// Returns the replaced step. diff --git a/crates/but-workspace/src/commit/move_commit.rs b/crates/but-workspace/src/commit/move_commit.rs index c65bc47ed0f..c029b77c949 100644 --- a/crates/but-workspace/src/commit/move_commit.rs +++ b/crates/but-workspace/src/commit/move_commit.rs @@ -1,20 +1,15 @@ //! Move a commit within or across branches and stacks. pub(crate) mod function { - use anyhow::{Context, bail}; use but_core::RefMetadata; use but_rebase::graph_rebase::{ - Editor, SuccessfulRebase, ToCommitSelector, ToSelector, + Editor, LookupStep as _, SuccessfulRebase, ToCommitSelector, ToSelector, mutate::{InsertSide, SegmentDelimiter, SelectorSet, SomeSelectors}, }; /// Move a commit. /// - /// `editor` is assumed to have been generated from the given `workspace` - /// and therefore aligned. - /// - /// `workspace` - Used for getting the surrounding context of the commit being moved. - /// In the future, we should not rely on the projection and do it fully on the graph. + /// `editor` is assumed to be aligned with the graph being mutated. /// /// `subject_commit` - The commit to be moved. /// @@ -30,12 +25,7 @@ pub(crate) mod function { anchor: impl ToSelector, side: InsertSide, ) -> anyhow::Result> { - let (subject_commit_selector, subject_commit) = - editor.find_selectable_commit(subject_commit)?; - - let subject = retrieve_commit_and_containers(editor.workspace, &subject_commit)?; - - let (source_stack, source_segment, _) = subject; + let (subject_commit_selector, _) = editor.find_selectable_commit(subject_commit)?; let commit_delimiter = SegmentDelimiter { child: subject_commit_selector, @@ -43,21 +33,9 @@ pub(crate) mod function { }; // Step 1: Determine the parents and children to disconnect. - let index_of_subject_commit = source_segment - .commits - .iter() - .position(|commit| commit.id == subject_commit.id) - .context("BUG: Subject commit is not in the source segment.")?; + let child_to_disconnect = determine_child_selector(&editor, subject_commit_selector)?; - let child_to_disconnect = - determine_child_selector(&editor, source_segment, index_of_subject_commit)?; - - let parent_to_disconnect = determine_parent_selector( - &editor, - source_stack, - source_segment, - index_of_subject_commit, - )?; + let parent_to_disconnect = determine_parent_selector(&editor, subject_commit_selector)?; // Step 2: Disconnect editor.disconnect_segment_from( @@ -72,120 +50,98 @@ pub(crate) mod function { editor.rebase() } - /// Determine the surrounding context of the commit to be moved - /// - /// Currently, this looks into the workspace projection in order to determine **where to take the commit from**. + /// Determine which child to disconnect from the subject commit. /// - /// ### The issue - /// It's impossible to know for sure what is the exact intention of 'moving a commit' inside a complex git graph. - /// A commit, can have N children and M parents. 'Moving' it somewhere else can imply: - /// - Disconnecting all parents and children, and inserting it somewhere else. - /// - Disconnecting the first parent and all children, and then inserting. - /// - Disconnecting *some* parents and *some* children, and then inserting it. + /// Preference rules: + /// - Prefer a `Reference` child first. In GitButler's linear segment model, + /// the top commit is commonly attached to a branch/workspace reference. + /// Detaching that reference edge preserves the expected "take this commit + /// out of its current stack position" behavior. + /// - If there is no reference child, fall back to a `Pick` child, which is + /// the linear commit child edge in the graph. /// - /// ### The GitButler assumption - /// In the context of a GitButler workspace (as of this writing), we want to disconnect the commit from the linear - /// segments and move them to another position in the same or other segment. That way, any other parents and - /// children that are not part of the source segment are kept. - /// - /// ### What the future holds - /// In the future, where we're not afraid of complex graphs, we've figured out UX and data wrangling, - /// the concept of a segment might not hold, and hence we'll have to figure out a better way of determining - /// what to cut (e.g. letting the clients decide what to cut). - fn retrieve_commit_and_containers<'a>( - workspace: &'a but_graph::projection::Workspace, - subject_commit: &but_core::CommitOwned, - ) -> anyhow::Result<( - &'a but_graph::projection::Stack, - &'a but_graph::projection::StackSegment, - &'a but_graph::projection::StackCommit, - )> { - let Some(subject) = workspace.find_commit_and_containers(subject_commit.id) else { - bail!("Failed to find the commit to move in the workspace."); - }; - Ok(subject) - } - - /// Determine which children to disconnect, based on the position of the subject commit in the segment. + /// We only disconnect one preferred child edge on purpose to preserve + /// segment-like move semantics instead of performing broad graph surgery. fn determine_child_selector<'ws, 'meta, M: RefMetadata>( editor: &Editor<'ws, 'meta, M>, - source_segment: &but_graph::projection::StackSegment, - index_of_subject_commit: usize, + subject_commit_selector: but_rebase::graph_rebase::Selector, ) -> Result { - let child_to_disconnect = if index_of_subject_commit == 0 { - // The commit is at the top of the branch. - // We just need to disconnect the ref from it. - let ref_name = source_segment - .ref_name() - .context("Source segment doesn't have a reference name.")?; - let reference_selector = editor.select_reference(ref_name)?; - let selectors = SomeSelectors::new(vec![reference_selector])?; - SelectorSet::Some(selectors) - } else if let Some(child_of_subject) = - source_segment.commits.get(index_of_subject_commit - 1) - { - let child_commit_selector = editor.select_commit(child_of_subject.id)?; - let selectors = SomeSelectors::new(vec![child_commit_selector])?; - SelectorSet::Some(selectors) - } else { - bail!( - "BUG: Subject commit is not the first child in segment but also can't find its child." - ); + let mut children = editor.direct_children(subject_commit_selector)?; + children.sort_by_key(|(_, order)| *order); + + // Prefer references first (segment head relationship), then pick children. + let preferred = children + .iter() + .find(|(selector, _)| { + matches!( + editor.lookup_step(*selector), + Ok(but_rebase::graph_rebase::Step::Reference { .. }) + ) + }) + .or_else(|| { + children.iter().find(|(selector, _)| { + matches!( + editor.lookup_step(*selector), + Ok(but_rebase::graph_rebase::Step::Pick(_)) + ) + }) + }) + .map(|(selector, _)| *selector); + + let child_to_disconnect = match preferred { + Some(selector) => { + let selectors = SomeSelectors::new(vec![selector])?; + SelectorSet::Some(selectors) + } + None => SelectorSet::None, }; Ok(child_to_disconnect) } - /// Determine which parents to disconnect, based on the position of the subject commit in the segment - /// and the position of the source segment in the source stack. + /// Determine which parent to disconnect from the subject commit. + /// + /// Preference rules: + /// - Prefer a `Pick` parent first. This matches first-parent linear history + /// semantics, which is the primary ancestry edge we want to detach when + /// moving a commit within or across stacks. + /// - If there is no commit parent edge, fall back to a `Reference` parent. + /// + /// If no explicit parent candidate is available (e.g. truncated history or + /// root-like scenarios), we use `SelectorSet::All` as a safe fallback, + /// matching prior behavior for these edge cases. fn determine_parent_selector<'ws, 'meta, M: RefMetadata>( editor: &Editor<'ws, 'meta, M>, - source_stack: &but_graph::projection::Stack, - source_segment: &but_graph::projection::StackSegment, - index_of_subject_commit: usize, + subject_commit_selector: but_rebase::graph_rebase::Selector, ) -> Result { - let parent_to_disconnect = if index_of_subject_commit == source_segment.commits.len() - 1 { - // Subject commit is the last one, then we need to disconnect the parent ref. - // If this is `None` but the `base_segment_id` is defined, it means that the parent segment lives - // outside the workspace and it's probably the target branch. - let stack_base_segment_ref_name = - source_segment.base_segment_id.and_then(|base_segment_id| { - source_stack.segments.iter().find_map(|segment| { - if segment.id == base_segment_id { - segment.ref_name() - } else { - None - } - }) - }); + let mut parents = editor.direct_parents(subject_commit_selector)?; + parents.sort_by_key(|(_, order)| *order); - // Look for the base segment in the graph data, as a fallback if there's no stack segment found. - let graph_base_segment_ref_name = source_segment - .base_segment_id - .map(|base_segment_id| &editor.workspace.graph[base_segment_id]) - .and_then(|segment| segment.ref_name()); + // Prefer parent commit first (linear segment), then reference fallback. + let preferred = parents + .iter() + .find(|(selector, _)| { + matches!( + editor.lookup_step(*selector), + Ok(but_rebase::graph_rebase::Step::Pick(_)) + ) + }) + .or_else(|| { + parents.iter().find(|(selector, _)| { + matches!( + editor.lookup_step(*selector), + Ok(but_rebase::graph_rebase::Step::Reference { .. }) + ) + }) + }) + .map(|(selector, _)| *selector); - match stack_base_segment_ref_name.or(graph_base_segment_ref_name) { - Some(ref_name) => { - let reference_selector = editor.select_reference(ref_name)?; - let selectors = SomeSelectors::new(vec![reference_selector])?; - SelectorSet::Some(selectors) - } - None => { - // No explicit base segment/ref available (e.g., root commit or traversal stopped early). - // Fall back to disconnecting from all parents; downstream code can handle this. - SelectorSet::All - } + let parent_to_disconnect = match preferred { + Some(selector) => { + let selectors = SomeSelectors::new(vec![selector])?; + SelectorSet::Some(selectors) } - } else if let Some(parent_of_subject) = - source_segment.commits.get(index_of_subject_commit + 1) - { - let parent_commit_selector = editor.select_commit(parent_of_subject.id)?; - let selectors = SomeSelectors::new(vec![parent_commit_selector])?; - SelectorSet::Some(selectors) - } else { - bail!( - "BUG: Subject commit is not the last commit in the segment but can't find its parent either." - ) + // No explicit parent available (e.g. root commit/truncated history). + None => SelectorSet::All, }; Ok(parent_to_disconnect) }