libgit2 safe checkout demo - #14456
Closed
jonathantanmy2 wants to merge 1 commit into
Closed
Conversation
jonathantanmy2
requested review from
Byron,
Caleb-T-Owens,
estib-vega and
krlvi
as code owners
June 26, 2026 02:58
github-actions
Bot
added
rust
Contributor
There was a problem hiding this comment.
Pull request overview
This PR explores a libgit2-based “safe checkout” path (via a new checkout_tree() helper) and threads “consumed” DiffSpecs through commit creation/batch commit plumbing to compute a merge-base override intended to prevent already-consumed changes from reappearing after checkout.
Changes:
- Added a libgit2-backed
checkout_tree()helper inbut-coreand switched graph-rebase materialization to use it. - Extended commit creation APIs to return/accept “consumed”
DiffSpecs and updated batch commit to pass that through. - Added/adjusted tests and test utilities related to commits and TUI commit flows.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but/tests/but/command/commit.rs | Adds a regression test asserting commits don’t rewrite worktree files (mtime-based). |
| crates/but/src/command/legacy/status/tui/tests/utils.rs | Adds a debug() helper for the TUI test harness. |
| crates/but/src/command/legacy/status/tui/tests/commit_tests.rs | Temporary debug output/commented debug hook added to a TUI test. |
| crates/but/src/command/legacy/status/tui/mod.rs | Updates commit creation call to pass the new “existing_consumed” argument. |
| crates/but/src/command/legacy/commit.rs | Threads “consumed” through batch commits to compute merge-base override across iterations. |
| crates/but-workspace/src/commit/mod.rs | Alters merge-base override computation logic for consumed specs. |
| crates/but-workspace/src/commit/commit_create.rs | Introduces commit_create_ex() and a consumed field on CommitCreateOutcome. |
| crates/but-transaction/src/lib.rs | Extends transaction create_commit() and intermediate result to carry “consumed”. |
| crates/but-rebase/src/graph_rebase/materialize.rs | Switches materialize checkout to checkout_tree() (with old code commented out). |
| crates/but-core/src/worktree/mod.rs | Re-exports checkout_tree. |
| crates/but-core/src/worktree/checkout/function.rs | Adds the new libgit2-based checkout_tree() implementation. |
| crates/but-api/src/legacy/workspace.rs | Adjusts destructuring to ignore new CommitCreateOutcome fields. |
| crates/but-api/src/commit/create.rs | Adjusts destructuring to ignore new CommitCreateOutcome fields. |
| Cargo.toml | Enables git2 unstable-sha256 feature and patches git2 to a specific git revision. |
| Cargo.lock | Locks git2/libgit2-sys to the patched git source. |
Comments suppressed due to low confidence (1)
crates/but/src/command/legacy/status/tui/tests/commit_tests.rs:560
- Leftover
eprintln!in a snapshot-style TUI test will add noise to test output and can obscure failures in CI.
eprintln!("about to commit");
// commit the moved file
tui.input_then_render('c');
tui.input_then_render(KeyCode::Down);
tui.input_then_render('i');
Comment on lines
+688
to
+697
| let mut consumed = Vec::new(); | ||
| for planned_commit in planned_commits { | ||
| let outcome = tx.create_commit( | ||
| position.0.clone(), | ||
| position.1, | ||
| planned_commit.diff_specs, | ||
| planned_commit.message, | ||
| consumed.clone(), | ||
| )?; | ||
| consumed.extend(outcome.consumed); |
Comment on lines
+25
to
+40
| let mut specs: Vec<_> = consumed.into_iter().map(|mut spec| Ok(spec)).collect(); | ||
| // let mut specs: Vec<_> = consumed | ||
| // .into_iter() | ||
| // .filter(|spec| { | ||
| // if spec.hunk_headers.is_empty() { | ||
| // return workdir | ||
| // .join(gix::path::from_bstr(spec.path.as_bstr())) | ||
| // .exists(); | ||
| // } | ||
| // true | ||
| // }) | ||
| // .map(|mut spec| { | ||
| // spec.previous_path = None; | ||
| // Ok(spec) | ||
| // }) | ||
| // .collect(); |
Comment on lines
25
to
+29
| /// Rejected diff specs from commit creation. See [`create_commit`] for | ||
| /// more details. | ||
| pub rejected_specs: Vec<(but_core::tree::create_tree::RejectionReason, DiffSpec)>, | ||
| /// | ||
| pub consumed: Vec<DiffSpec>, |
| ) | ||
| } | ||
|
|
||
| /// |
Comment on lines
+1025
to
+1026
| /// | ||
| pub consumed: Vec<DiffSpec>, |
Comment on lines
561
to
565
| tui.input_then_render(KeyCode::Enter); | ||
| tui.input_then_render("move test.txt to moved-test.txt"); | ||
| tui.input_then_render(KeyCode::Enter); | ||
| // tui.debug(); | ||
|
|
Comment on lines
46
to
+50
| // If the head has changed (which means it's in the | ||
| // commit mapping), perform a safe checkout. | ||
| safe_checkout_from_head( | ||
| new_head, | ||
| &repo, | ||
| Options { | ||
| uncommitted_changes: UncommitedWorktreeChanges::KeepAndAbortOnConflict, | ||
| skip_head_update: true, | ||
| merge_base_override, | ||
| allow_conflicted_commit_checkout: true, | ||
| }, | ||
| )?; | ||
| // safe_checkout_from_head( | ||
| // new_head, | ||
| // &repo, |
Comment on lines
+1571
to
+1573
| let old_time = std::fs::metadata(env.projects_root().join("A"))?.modified()?; | ||
| env.but("commit -m test").assert().success(); | ||
| let new_time = std::fs::metadata(env.projects_root().join("A"))?.modified()?; |
Comment on lines
+16
to
+40
| /// Update the index and working directory (but not any refs). If | ||
| /// `baseline_treeish` is None, `HEAD^{tree}` is used instead. | ||
| pub fn checkout_tree( | ||
| repo: &gix::Repository, | ||
| treeish: gix::ObjectId, | ||
| baseline_treeish: Option<gix::ObjectId>, | ||
| ) -> anyhow::Result<()> { | ||
| let git2_repo = git2::Repository::open(repo.git_dir())?; | ||
|
|
||
| // eprintln!("HEAD is {:?}, treeish is {:?}", repo.head_id(), treeish); | ||
| if let Some(baseline_treeish) = baseline_treeish { | ||
| let baseline = git2_repo | ||
| .find_object(baseline_treeish.to_git2(), None)? | ||
| .peel_to_tree()?; | ||
| git2_repo.index()?.read_tree(&baseline)?; | ||
| // eprintln!("baseline tree is {:?}", baseline.id()); | ||
| let mut opts = git2::build::CheckoutBuilder::new(); | ||
| opts.baseline(&baseline); | ||
| git2_repo.checkout_tree( | ||
| &git2_repo.find_object(treeish.to_git2(), None)?, | ||
| Some(&mut opts), | ||
| )?; | ||
| } else { | ||
| // eprintln!("baseline tree not given"); | ||
| git2_repo.checkout_tree(&git2_repo.find_object(treeish.to_git2(), None)?, None)?; |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Some things need to happen before we can merge this PR:
baseline{,_index}rust-lang/git2-rs#1284