cli: wire commit command to commit_create API#12576
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c41ec99af2
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Wires the legacy but commit CLI implementation in crates/but to use the newer but_api::commit::commit_create API, aligning CLI behavior with the graph-based commit creation flow in but-api.
Changes:
- Replace legacy
workspace::create_commit_from_worktree_changes(...)usage withcommit_create(...). - Add a human-visible warning (and tracing warning) when some selected
DiffSpecs are rejected.
Caleb-T-Owens
left a comment
There was a problem hiding this comment.
LGTM.
There is one bug fix in the rebase engine WRGT multiple empty parallel lanes that needs to be landed before we can make a release with this though.
I'm hoping to put out the fix tomorrow so it shouldn't be a blocker to merging
@Caleb-T-Owens btw i updated the PR description - PTAL. Basically im still not 100% sure this is correct. It seems correct tho |
|
Sorry - I assumed you were happy with the test change and skipped it. I do agree that it looks off. It seems related to the empty-branch issue I mentioned. Let's hold off on merging and see if the test can be reverted after my fix for the problem is resolved. |
8b77ab1 to
ec8734d
Compare
17888e8 to
056101a
Compare
056101a to
3ef8e17
Compare
3ef8e17 to
cc4054c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/but/src/command/legacy/commit.rs:496
- When all diff specs are rejected by the underlying API,
outcome.new_commitwill beNoneandoutcome.rejected_specswill be non-empty. In this scenario, the new code at lines 470–482 correctly warns the user, but then line 489–496 still outputs "✓ Created commit unknown on branch X" — a falsely successful message immediately after a warning that nothing could be committed. This is a misleading combination: the rejected-specs warning was introduced by this PR, and it makes the existingNone => "unknown"fallback more visible as a contradiction. Whenoutcome.new_commitisNone, the function should return an error (or at least skip the success output) rather than printing both a warning and a success message.
if let Some(out) = out.for_human() {
let commit_short = match outcome.new_commit {
Some(id) => id.to_hex_with_len(7).to_string(),
None => "unknown".to_string(),
};
writeln!(
out,
"{} {} {} {}",
"✓ Created commit".green(),
commit_short.magenta(),
"on branch".green(),
target_branch.name.to_str_lossy().yellow()
)?;
| // Insert relative to the branch reference itself so only that branch tip is advanced. | ||
| let outcome = commit_create( | ||
| ctx, | ||
| target_stack_id, | ||
| Some(HexHash::from(parent_commit_id)), | ||
| but_api::commit::ui::RelativeTo::Reference(target_branch.reference.clone()), | ||
| InsertSide::Below, | ||
| diff_specs, | ||
| final_commit_message, | ||
| target_branch.name.to_string(), | ||
| )?; |
There was a problem hiding this comment.
The PR description specifically highlights that the key behavioral difference between the old and new API is that using RelativeTo::Reference instead of RelativeTo::Commit prevents multiple branch references from being advanced when they point to the same commit. However, there is no test covering the scenario of a stacked branch stack (e.g., branch B stacked on top of branch A) where committing to the lower branch A should only advance A's reference and properly rebase B, without B's reference being incorrectly advanced. This is the most critical behavioral difference mentioned in the PR description and should have explicit test coverage.
@Caleb-T-Owens so... the semantic of the new API is every so slightly different. Take a look at the changes to the test.
The previous api (create_commit_from_worktree_changes) also carried stack_id + stack_branch_name, which scoped which branch ref gets advanced.
With commit_create, targeting a commit node can rewire incoming refs at that node, so multiple refs pointing at the same commit may move. Thats why im trying here to use RelativeTo::Reference(target_branch.reference) + InsertSide::Below: it keeps parent behavior equivalent while scoping ref movement to the selected branch reference.
Edit:
This is now based on branch-creation-2.0