(don't review, just testing CI)#14524
Open
jonathantanmy2 wants to merge 3 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces new libgit2-based checkout helpers in but-core, switches the rebase materialization path to use the new checkout entrypoint, and updates many worktree checkout tests accordingly. It also adjusts the workspace merge-base override computation used for “consumed hunks” and pins git2 to a specific git revision with an additional feature flag.
Changes:
- Added
checkout_tree,checkout_commit, andcheckout_unconflicted_commithelpers and re-exported them frombut_core::worktree. - Switched rebase materialization checkout from
safe_checkout_from_headtocheckout_tree. - Updated many worktree checkout tests to use the new checkout helpers; updated
git2dependency source/feature.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but-workspace/src/commit/mod.rs | Simplifies merge-base override computation used for checkout baselines derived from consumed changes. |
| crates/but-rebase/src/graph_rebase/materialize.rs | Switches rebase materialization to use the new checkout_tree function. |
| crates/but-core/tests/core/worktree/checkout.rs | Migrates several tests from safe_checkout to the new checkout helpers and updates assertions accordingly. |
| crates/but-core/src/worktree/mod.rs | Re-exports newly added checkout helpers from the worktree module. |
| crates/but-core/src/worktree/checkout/function.rs | Adds new checkout helper functions backed by libgit2. |
| Cargo.toml | Enables an additional git2 feature and patches git2 to a specific git revision. |
| Cargo.lock | Updates lockfile entries for git2 / libgit2-sys to match the git-sourced patch. |
| head_update: "Update refs/heads/main to Some(Object(Sha1(31ec8eacfba4051fd673e4fe23c775e87896a463)))", | ||
| } | ||
| "#); | ||
| checkout_commit(&repo, head_commit.id, Some(empty_tree))?; |
Comment on lines
322
to
326
| assert_eq!( | ||
| err.to_string(), | ||
| "Uncommitted files would be overwritten by checkout: \"file\"", | ||
| "1 conflict prevents checkout; class=Checkout (20); code=Conflict (-13)", | ||
| "we check for conflict markers, and fail." | ||
| ); |
| // given baseline. We also need it to pretend that the index is the | ||
| // given baseline. That is not possible with the current API, so for | ||
| // now, write to the index to make it the given baseline. | ||
| git2_repo.index()?.read_tree(&baseline)?; |
Comment on lines
+38
to
+39
| let mut opts = git2::build::CheckoutBuilder::new(); | ||
| if let Some(ref baseline) = baseline { |
| commit: gix::ObjectId, | ||
| baseline_treeish: Option<gix::ObjectId>, | ||
| ) -> anyhow::Result<()> { | ||
| let new_object = commit.attach(repo).object()?; |
| Ok(spec) | ||
| }) | ||
| .collect(); | ||
| let mut specs: Vec<_> = consumed.into_iter().map(Ok).collect(); |
Comment on lines
46
to
49
| // 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 { | ||
| skip_head_update: true, | ||
| merge_base_override, | ||
| allow_conflicted_commit_checkout: true, | ||
| }, | ||
| )?; | ||
| checkout_tree(&repo, new_head, merge_base_override)?; | ||
| } |
1219d73 to
902666a
Compare
902666a to
a713bdd
Compare
Comment on lines
+23
to
+36
| let git2_repo = git2::Repository::open(repo.git_dir())?; | ||
| let baseline = if let Some(baseline_treeish) = baseline_treeish { | ||
| let baseline = git2_repo | ||
| .find_object(baseline_treeish.to_git2(), None)? | ||
| .peel_to_tree()?; | ||
| // libgit2 only pretends that HEAD's tree (and not the index) is the | ||
| // given baseline. We also need it to pretend that the index is the | ||
| // given baseline. That is not possible with the current API, so for | ||
| // now, write to the index to make it the given baseline. | ||
| git2_repo.index()?.read_tree(&baseline)?; | ||
| Some(baseline) | ||
| } else { | ||
| None | ||
| }; |
Comment on lines
+321
to
326
| let err = checkout_commit(&repo, new_commit.id, Some(head_commit.id)).unwrap_err(); | ||
| assert_eq!( | ||
| err.to_string(), | ||
| "Uncommitted files would be overwritten by checkout: \"file\"", | ||
| "1 conflict prevents checkout; class=Checkout (20); code=Conflict (-13)", | ||
| "we check for conflict markers, and fail." | ||
| ); |
| "#); | ||
| checkout_commit(&repo, new_commit.id, None)?; | ||
|
|
||
| // Nothing changed as the checkout was aborted. |
Comment on lines
314
to
+317
| [patch.crates-io] | ||
| # Keep workspace crates that use the `gix 0.84` line on the same git revision. | ||
| gix = { git = "https://github.com/GitoxideLabs/gitoxide", rev = "66f70f3063724b4145b21c6691c4f57892c7a74e" } | ||
| git2 = { git = "https://github.com/jonathantanmy2/git2-rs", rev = "9e2ec81c8d37ba5142e5fb97c5afbc2f43c6d7eb" } |
a713bdd to
26d078d
Compare
Switch the rebase engine to use it. All other uses will be switched in future PRs. Most tests in crates/but-core/tests/core/worktree/checkout.rs were switched to the new function too, to increase test coverage.
26d078d to
c1c9324
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
crates/but-api/src/branch.rs:936
- PR description says only the rebase engine is switched and that other call sites will be migrated in future PRs, but this change also migrates the branch checkout API path (and this PR touches multiple other non-rebase call sites). Consider updating the PR description/splitting the change so the scope matches what reviewers/CI expectations are based on.
checkout_tree(&repo, target, Some(current_head)).with_context(|| {
format!(
"Could not safely check out '{}' from {current_head} to {target}",
reference_name.as_bstr()
)
| // given baseline. We also need it to pretend that the index is the | ||
| // given baseline. That is not possible with the current API, so for | ||
| // now, write to the index to make it the given baseline. | ||
| git2_repo.index()?.read_tree(&baseline)?; |
Comment on lines
+16
to
+17
| /// Update the index and working directory (but not any refs). If | ||
| /// `baseline_treeish` is None, `HEAD^{tree}` is used instead. |
Comment on lines
321
to
325
| assert_eq!( | ||
| err.to_string(), | ||
| "Uncommitted files would be overwritten by checkout: \"file\"", | ||
| "1 conflict prevents checkout; class=Checkout (20); code=Conflict (-13)", | ||
| "we check for conflict markers, and fail." | ||
| ); |
| "#); | ||
| checkout_commit(&repo, new_commit.id, None)?; | ||
|
|
||
| // Nothing changed as the checkout was aborted. |
Comment on lines
348
to
350
| #[test] | ||
| fn checkout_handles_directory_and_file_replacements() -> anyhow::Result<()> { | ||
| if but_testsupport::gix_testtools::is_ci::cached() { | ||
| // TODO(gix): remove this once `gitoxide` unconditional reset/checkout is available. | ||
| // Fails on checkout on CI Linux as it can't deal with `file`. | ||
| // Probably the `git2` OS error code handling isn't working cross-platform? | ||
| eprintln!("SKIPPING TEST KNOWN TO FAIL ON CI ONLY"); | ||
| return Ok(()); | ||
| } | ||
| let (repo, _tmp) = writable_scenario("merge-with-two-branches-line-offset"); |
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.
Switch the rebase engine to use it. All other uses will be switched in future PRs.
Most tests in crates/but-core/tests/core/worktree/checkout.rs were switched to the new function too, to increase test coverage.