Use target branch tip as base for new independent branches#13626
Conversation
dcef811 to
31a10a1
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts how “independent” branches are based in a workspace by preferring the newest (closest-to-target) merge base among all stacks, instead of always using the workspace lower bound. This better aligns new independent branches with the most recent common history when stacks diverge at different points from the target.
Changes:
- Added
Workspace::newest_base_among_stacks()to compute a newer merge base across stacks vs. the target. - Updated independent-branch creation to prefer
newest_base_among_stacks()and fall back tolower_bound. - Added workspace tests covering expected “newest base” selection and target configuration edge cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/but-workspace/src/branch/create_reference.rs | Uses the new workspace API to choose a more recent base for independent branches. |
| crates/but-graph/src/projection/workspace/api.rs | Introduces newest_base_among_stacks() to compute a newer merge base among stacks and the target. |
| crates/but-graph/tests/graph/workspace/newest_base_among_stacks.rs | Adds tests validating newest-base selection and behavior without a configured target. |
| crates/but-graph/tests/graph/workspace/mod.rs | Registers the new test module. |
31a10a1 to
0590302
Compare
0590302 to
6e87b22
Compare
6e87b22 to
b6dd81e
Compare
b6dd81e to
cc3bb1f
Compare
6cac6cd to
4718902
Compare
4718902 to
06366aa
Compare
620682c to
b9be514
Compare
|
@mtsgrd This PR reads like it's trying to optimize the location of newly created refs, such that more recent locations are preferred. While this makes sense, I don't know where this is coming from:
In theory, that independent branch can still be created on the lower bound of the workspace. Needing a target, always, goes against supporting all states that Git can legitimately be in, like local-only workflows. Maybe it's just the description that is somewhat stale, I didn't look at the implementation in detail. |
b9be514 to
12d229a
Compare
12d229a to
c3789dd
Compare
Previously, new independent branches were based on the workspace's lower bound (the lowest common ancestor of all stacks). When stacks have different merge bases with the target, this placed the new branch on an unnecessarily old commit. Now we prefer the stored target commit (target.sha) via a new `Workspace::target_tip_commit_id()` helper, which checks `target_commit` first, then `target_ref`. Falls back to the workspace lower bound for local-only workflows without a target. Also extracts `target_segment_index()` to deduplicate the target resolution logic shared with `merge_base_with_target_branch()`. Tested with four cases in `target_tip_commit_id.rs`: - Two stacks with different bases — returns the target tip - One stack above the target — still returns the target tip - No target configured — returns None - Only extra_target set — returns None (not sufficient)
c3789dd to
4eabdc1
Compare
|
@Byron could you have another look? I'd like to get this merged asap so we can build a nightly. 🙏 |
I think we are past that, I am many PRs behind. |
Problem
When creating a new independent branch, we used the workspace's lower bound — the lowest common ancestor of all stacks. If stacks have different merge bases with the target, this places the new branch on an unnecessarily old commit:
Solution
Use the stored target commit (
target.sha) as the base for new independent branches. If unavailable, fall back to the target ref tip, then to the workspace lower bound (for local-only workflows without a target).Key changes
but-graph/.../workspace/api.rsresolved_target_commit_id()— returns the storedtarget_commitID, falling back to thetarget_refsegment tiptarget_segment_index()— extracted shared helper to deduplicate target resolution withmerge_base_with_target_branch()but-workspace/.../create_reference.rs— useresolved_target_commit_id().or(lower_bound)instead of justlower_boundTest plan
Five tests in
resolved_target_commit_id.rs:target_commitpreferred overtarget_refwhen both are setNoneextra_targetset — returnsNone(not sufficient)