Skip to content

Remove move branch manual metadata work - #16309

Merged
Caleb-T-Owens merged 16 commits into
masterfrom
caleb/drop-branch-move-meta
Oct 14, 2026
Merged

Caleb-T-Owens merged 16 commits into
masterfrom
caleb/drop-branch-move-meta

Conversation

@Caleb-T-Owens

@Caleb-T-Owens Caleb-T-Owens commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

There were some tests that only asserted metadata. I don't think we should need to assert metadata at this level, so I've changed it to assert workspace outcomes.


Note

High Risk
Changes core branch creation, reordering, and HEAD checkout across API, transactions, and rebase materialization—behavior users depend on for stack projection and ad-hoc workspaces.

Overview
Branch create, move, and tear-off now run entirely through the graph rebase Editor and materialize, instead of overlay graph rebuilds plus separate metadata and checkout steps.

create_reference takes an Editor, places refs in the step graph (including independent stacks as parents of the workspace commit), and returns a SuccessfulRebase. API, transaction, action, and legacy virtual-branch callers were updated to use that path and drop the old new_stack_id / recording-metadata wrappers. Move/tear-off no longer return Outcome with branch_stack_order and new_tip; ad-hoc reordering updates HEAD via new set_head_checkout so materialization keeps the stack projected. Related editor work adds insert_parent, workspace ref/commit selectors, and stricter HEAD checkout change detection.

Graph init adjusts ad-hoc segment splitting at the target (unnamed segments get ref metadata before split). Tests now assert workspace graphs and HEAD rather than persisted branch-order metadata alone.

Reviewed by Cursor Bugbot for commit 99bf5b1. Bugbot is set up for automated code reviews on this repo. Configure here.

Copilot AI balanced review requested due to automatic review settings October 13, 2026 14:10
@Caleb-T-Owens
Caleb-T-Owens added this pull request to stack #16310 October 13, 2026 14:10
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Oct 13, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T14:16:02.346131Z a8be402 PR opened
🔒 Security Review ✅ Completed 2026-10-13T14:13:51.058303Z a8be402 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Materialization can discard ordering for branches not reachable below the current editor checkout.

1 open finding
What changed in this PR

Moves branch ordering responsibility from callers to graph rebase materialization.

Changes:

  • Removes branch_stack_order from move outcomes.
  • Uses graph mutations to derive and persist ordering.
  • Replaces metadata-focused tests with workspace snapshots.
File Description
crates/​but-workspace/​src/​branch/​move_branch.rs Derives ordering through graph edits.
crates/​but-workspace/​tests/​workspace/​branch/​move_branch.rs Asserts projected workspace outcomes.
crates/​but-transaction/​src/​lib.rs Removes queued ordering updates.
crates/​but-api/​src/​branch.rs Removes manual API persistence and preview overrides.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 132 to 140
} else {
// There is no workspace commit to connect to the torn-off branch. Attach only
// its first parent at the target, leaving the remaining stack independent.
editor.add_edge(subject_delimiter.parent, target_selector, 0)?;
Some(
existing_order
.unwrap_or_else(|| stack_branch_order(source_stack))
.into_iter()
.filter(|name| name.as_ref() != subject_branch_name)
.collect(),
)
};
}

Ok(Outcome {
rebase: editor.rebase()?,
new_tip: None,
Copilot AI balanced review requested due to automatic review settings October 13, 2026 14:15

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8be40220b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +330 to +333
return Ok(Outcome {
rebase: editor.rebase()?,
new_tip,
branch_stack_order: Some(new_order),
})
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve the full order before checking out the new tip

When an ad-hoc move takes the checked-out top branch downward (for example, C above A producing B, C, A), this outcome no longer carries new_order. The rebase is materialized while its checkout selector still points to C, so SuccessfulRebase::branch_stacks() sees only C, A and replaces the persisted chain with that truncated order; the API and transaction paths check out B only afterward, without re-persisting metadata. This breaks the existing contract that the successful move persists B, C, A, and can lose ordering that Git cannot reconstruct for empty branches. Preserve the computed order until materialization, or make the rebase checkout follow new_tip before branch stacks are recorded.

AGENTS.md reference: crates/AGENTS.md:L46-L54

Useful? React with 👍 / 👎.

Comment on lines 132 to 135
} else {
// There is no workspace commit to connect to the torn-off branch. Attach only
// its first parent at the target, leaving the remaining stack independent.
editor.add_edge(subject_delimiter.parent, target_selector, 0)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Retain the remaining stack when tearing off HEAD

When the torn-off branch is the current ad-hoc checkout, this branch makes it independent but leaves the editor checkout on that branch. Materialization therefore derives only the singleton checked-out stack, and set_branch_stack_order([subject]) replaces the former chain, deleting the order of all branches left behind; with an empty C, B, A chain, tearing off C loses the otherwise non-reconstructible B, A order. The removed code explicitly persisted the old chain minus the subject, so that remaining order still needs to be supplied or otherwise recorded.

AGENTS.md reference: crates/AGENTS.md:L46-L54

Useful? React with 👍 / 👎.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Earlier transaction metadata updates can overwrite the final graph-derived branch order.

3 open findings

🧠 Review effort: Balanced

Comment thread crates/but-transaction/src/lib.rs Outdated
Comment thread crates/but-workspace/src/branch/move_branch.rs
Base automatically changed from caleb/drop-branch-move-meta-1 to master October 13, 2026 15:25
@Caleb-T-Owens
Caleb-T-Owens force-pushed the caleb/drop-branch-move-meta branch from beb1278 to 25f4ab5 Compare October 13, 2026 15:25
@Caleb-T-Owens
Caleb-T-Owens removed this pull request from stack #16310 October 13, 2026 22:02
@Caleb-T-Owens
Caleb-T-Owens force-pushed the caleb/drop-branch-move-meta branch from 25f4ab5 to fb5d16b Compare October 13, 2026 22:02
Copilot AI balanced review requested due to automatic review settings October 13, 2026 22:02
@github-actions github-actions Bot added the CLI The command-line program `but` label Oct 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Explicit local branch creation incorrectly rejects names shared only by remote-tracking refs.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment on lines +175 to +179
let available = find_unique_refname_excluding(editor.repo(), ref_name, |name| {
editor.try_select_reference(name).is_some()
})?;
if available.as_ref() != ref_name {
bail_precondition!("Reference '{}' already exists", ref_name.shorten());
Copilot AI balanced review requested due to automatic review settings October 14, 2026 09:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Reference creation can lose ad-hoc ordering and incorrectly rejects local names matching remote-tracking branches.

2 open findings
Previously missed (1)

In code that hasn't changed since last review

Medium severity Restore precondition for non-persistable branch order

crates/​but-workspace/​src/​branch/​create_reference.rs:122

Ad-hoc relative placement now succeeds even when M::can_persist_branch_stack_order() is false. BranchStacks::persist skips ad-hoc order in that case (but-rebase/src/graph_rebase/branch_stacks.rs:93-101), so two refs placed on the same commit lose their relative order as soon as the workspace is reloaded. The removed requires_branch_order_metadata test previously guarded this exact case, and move_branch_in_single_branch_mode still rejects it. Restore that precondition for placements whose ordering is not representable in Git, or provide another durable encoding before creating the ref.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 14, 2026 10:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Ad-hoc tear-off can erase remaining branch order, and explicit local names matching remote-tracking branches are incorrectly rejected.

3 open findings

🧠 Review effort: Balanced

Comment on lines 109 to +113
} else {
// There is no workspace commit to connect to the torn-off branch. Attach only
// its first parent at the target, leaving the remaining stack independent.
editor.add_edge(subject_delimiter.parent, target_selector, 0)?;
Some(
existing_order
.unwrap_or_else(|| stack_branch_order(source_stack))
.into_iter()
.filter(|name| name.as_ref() != subject_branch_name)
.collect(),
)
};
}
Copilot AI balanced review requested due to automatic review settings October 14, 2026 12:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Explicit local branch creation now incorrectly rejects names shared with remote-tracking branches.

3 open findings

🧠 Review effort: Balanced

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4d0849b. Configure here.

}
let reference = editor.add_step(reference)?;
editor.add_edge(reference, base, 0)?;
editor.insert_parent(workspace_commit, reference, order.into())?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workspace commit creation drops stacks

High Severity

ensure_workspace_commit runs for every create_reference, including dependent placement, and a minted workspace commit only parents below. branch_stacks persist then records only those parents, so other applied stacks can disappear from the workspace.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4d0849b. Configure here.

Copilot AI balanced review requested due to automatic review settings October 14, 2026 12:39
@Caleb-T-Owens
Caleb-T-Owens force-pushed the caleb/drop-branch-move-meta branch from 4d0849b to 99bf5b1 Compare October 14, 2026 12:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Reference creation can incorrectly reject valid local names and lose ad-hoc ordering on unsupported metadata backends.

4 open findings

🧠 Review effort: Balanced

ref_name: new_ref.to_owned(),
archived: false,
Some(
Anchor::AtReference { ref_name, position }
@Caleb-T-Owens
Caleb-T-Owens merged commit 2d95f37 into master Oct 14, 2026
62 of 92 checks passed
@Caleb-T-Owens
Caleb-T-Owens deleted the caleb/drop-branch-move-meta branch October 14, 2026 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLI The command-line program `but` rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants