Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
Adds a deterministic commit-ordering utility to but-workspace that sorts selected commits parent-first (topological order) and uses workspace traversal order as the tie-breaker for unrelated commits—intended to support multi-commit in-memory graph mutations with fewer conflicts.
Changes:
- Introduce
but_workspace::ordering::order_commit_selectors_by_parentage(neworderingmodule). - Add integration tests covering linear chains, disjoint commits, mixed ancestry, deduplication, and singleton behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but-workspace/src/lib.rs | Exposes the new ordering module from the crate root. |
| crates/but-workspace/src/ordering/mod.rs | Defines the ordering module and re-exports the ordering function. |
| crates/but-workspace/src/ordering/commit_parentage.rs | Implements parentage-based ordering with workspace-order tie-breaks. |
| crates/but-workspace/tests/workspace/main.rs | Registers the new ordering test module. |
| crates/but-workspace/tests/workspace/ordering/mod.rs | Wires up commit-parentage ordering tests. |
| crates/but-workspace/tests/workspace/ordering/commit_parentage.rs | Adds test coverage for the ordering behavior and invariants. |
estib-vega
force-pushed
the
commit-ordering
branch
3 times, most recently
from
4c46bce to
e916590
Compare
estib-vega
force-pushed
the
commit-ordering
branch
from
e916590 to
b91d1b3
Compare
Create but-workspace commit ordering function that sorts the commits by their parentage, according to the workspace appearance.
estib-vega
force-pushed
the
commit-ordering
branch
from
b91d1b3 to
9aa529c
Compare
| SegmentRelation::Ancestor => return Ok(Relation::LeftIsAncestorOfRight), | ||
| SegmentRelation::Descendant => return Ok(Relation::RightIsAncestorOfLeft), | ||
| SegmentRelation::Disjoint | SegmentRelation::Diverged => return Ok(Relation::Unrelated), | ||
| SegmentRelation::Identity => { | ||
| // Commits can still be in parent/child relation inside one segment. | ||
| } |
There was a problem hiding this comment.
SegmentRelation::Ancestor/Descendant is being treated as a definitive commit-level ancestry relationship and short-circuits without checking the actual commits. In but_graph, edges can originate from a specific commit index within a segment, so two commits in different segments can be in segments with an ancestor/descendant relationship even when the specific commits are not ancestor/descendant. This can incorrectly add ordering constraints for unrelated commits.
To ensure correctness, verify commit-level ancestry (e.g., via repo.merge_base(left.id, right.id)) before returning LeftIsAncestorOfRight/RightIsAncestorOfLeft, or only use SegmentRelation as a fast-path when you can prove it implies commit ancestry for the chosen commits (otherwise fall back to merge-base logic).
| SegmentRelation::Ancestor => return Ok(Relation::LeftIsAncestorOfRight), | |
| SegmentRelation::Descendant => return Ok(Relation::RightIsAncestorOfLeft), | |
| SegmentRelation::Disjoint | SegmentRelation::Diverged => return Ok(Relation::Unrelated), | |
| SegmentRelation::Identity => { | |
| // Commits can still be in parent/child relation inside one segment. | |
| } | |
| SegmentRelation::Disjoint | SegmentRelation::Diverged => { | |
| return Ok(Relation::Unrelated); | |
| } | |
| SegmentRelation::Ancestor | SegmentRelation::Descendant | SegmentRelation::Identity => { | |
| // Segment-level ancestry is not sufficient to prove commit-level ancestry: | |
| // segments may relate through edges that start at specific commit indices, so | |
| // selected commits in those segments can still be unrelated. Confirm with | |
| // merge-base before returning a commit ancestry relation. | |
| } |
| The implementation prefers segment-level relation checks first, then falls back to commit-level merge-base logic only when needed. | ||
|
|
||
| ### Segment-first classification | ||
|
|
||
| For selected commits `left` and `right`, call: | ||
|
|
||
| - `editor.workspace.graph.relation_between(left.segment_id, right.segment_id)` | ||
|
|
||
| Mapping used: | ||
|
|
||
| - `Ancestor` -> `LeftIsAncestorOfRight` | ||
| - `Descendant` -> `RightIsAncestorOfLeft` | ||
| - `Disjoint` or `Diverged` -> `Unrelated` | ||
| - `Identity` -> unresolved at segment level, so use commit-level fallback | ||
|
|
There was a problem hiding this comment.
The doc currently states that SegmentRelation::Ancestor/Descendant directly map to commit-level ancestry (LeftIsAncestorOfRight / RightIsAncestorOfLeft). Because segment edges can originate at a specific commit index within a segment, segment ancestry doesn’t necessarily imply the selected commits are in an ancestor/descendant relationship. The documentation should be updated to reflect the additional commit-level verification (or the limitations) once the implementation is corrected.
| } | ||
| } | ||
|
|
||
| let merge_base = match editor.repo().merge_base(left.id, right.id) { |
There was a problem hiding this comment.
Something I’ve been thinking about in some of my graph overlay work is how to reconcile the fact that a pick statement’s OIDs may not imply the same history as the commit graph. For example, with editor.insert_below(target, pick, below), the target will still have the same OID, but the editor graph itself has changed relative to what git would be telling you.
In most cases, that divergence should be fine. However, if more drastic operations are being performed, such as reparenting commits, it could lead to unexpected issues.
In https://github.com/gitbutlerapp/gitbutler/pull/13080, I’ve actually removed public access to the workspace property from the editor, because it always reflects the version of the workspace at the time the editor was created and therefore isn’t representative of the current graph state.
I’ve also modified the functions that were using it so that they call .rebase() and retrieve the latest overlay graph from SuccessfulRebase. That means that when we call into_editor again, we can work with the most up-to-date graph, aligned with the editor’s current state.
I’d love @Byron’s thoughts on this, but I’ve been wondering more and more whether it would be helpful to have some graph-inspection operations that run specifically on the editor’s step graph, so we can find things like merge bases there as well.
It would mean an additional implementation of logic similar to what already exists in but_graph, but it would also let us operate without the same desync concerns.
Perhaps I’m missing some context here, though. Do let me know.
Create but-workspace commit ordering function that sorts the commits by
their parentage, according to the workspace appearance.
Cool, but why?
In order to enable graph mutations with multiple commits, I need to sort the commits from parent-most to child-most.
That way I can do one operation at a time in memory, parent to child. And then materialize it at the very end.
Ordering should reduce the probability of running into conflicts.
Disclaimer
This heavily uses AI for the generation of this algorithm
Comprehensive docs
https://github.com/gitbutlerapp/gitbutler/pull/13153/changes#diff-bb49765eeedd900a433bba2fc60faf0d82730392d9053af079c8c1d2407aa905