-
Notifications
You must be signed in to change notification settings - Fork 893
move-commit: Forget about the workspace projection #13154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
estib-vega
wants to merge
1
commit into
master
Choose a base branch
from
move-commit-forgets-about-the-workspace
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+122
−126
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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,34 +25,17 @@ pub(crate) mod function { | |
| anchor: impl ToSelector, | ||
| side: InsertSide, | ||
| ) -> anyhow::Result<SuccessfulRebase<'ws, 'meta, M>> { | ||
| 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, | ||
| parent: subject_commit_selector, | ||
| }; | ||
|
|
||
| // 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<SelectorSet, anyhow::Error> { | ||
| 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<SelectorSet, anyhow::Error> { | ||
| 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); | ||
|
Comment on lines
+116
to
+117
|
||
|
|
||
| // 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) | ||
| } | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editordoesn't definedirect_children(and there is no trait providing it in scope). A repo-wide search only finds this call site, so this will not compile. Either adddirect_childrentobut_rebase::graph_rebase::Editor(likely returning the incoming neighbors + edge order, normalized toSelectors), or rework this helper to use an existing API for enumerating incoming edges/children.