Restack fixes#48
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors floating-base detection in src/stack.rs to return validated matches with candidate indices, truncates target candidate lists by a patch-id boundary, validates matches by aligning first-parent chains, and adds integration tests covering zero-private-lineage, duplicate patch-id candidates, --pick behavior, and multi-commit rebases. ChangesFloating-base detection with match validation
Sequence DiagramsequenceDiagram
participant Branch
participant FindFloatingMatch
participant TargetContext
participant MatchValidator
Branch->>FindFloatingMatch: branch_id, target_oid
FindFloatingMatch->>TargetContext: lookup candidate_positions
alt candidate-id fast path
FindFloatingMatch->>MatchValidator: FloatingBaseMatch(branch_id, target_index)
else other match paths (tree/metadata/patch-id)
FindFloatingMatch->>FindFloatingMatch: compute target_index
FindFloatingMatch->>MatchValidator: FloatingBaseMatch(branch_id, target_index)
end
MatchValidator->>MatchValidator: align first-parent chain with target candidates
alt validation succeeds
MatchValidator-->>FindFloatingMatch: Ok(match)
FindFloatingMatch-->>Branch: Some(oid)
else validation fails
MatchValidator-->>FindFloatingMatch: rejected
FindFloatingMatch-->>Branch: None
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/pr.rs`:
- Around line 1391-1395: Add documentation and an inline comment clarifying that
the KIN_TEST_PR_EDIT_TITLE environment variable used in the non-interactive
branch (the code that checks !std::io::stdin().is_terminal() and returns
test_title) is strictly for tests; annotate that block (near the read of
KIN_TEST_PR_EDIT_TITLE) with a comment like "test-only: override PR edit title
in CI/non-interactive environments" and add a short note to the repository
testing guide or README describing the variable's purpose and usage to avoid
accidental use in production.
In `@src/commands/split.rs`:
- Around line 210-216: The apply_split function mutates refs (see apply_split
and the variables next_branches, new_branch_map, initial_branches, allowed_ids)
but currently writes HEAD last via set_head without a persisted recovery state;
add a durable state file (e.g., .kin/split-state) that records the planned ref
changes and metadata immediately after validation succeeds and before performing
any ref deletions/creations/moves, ensure every mutation step checks/updates or
at least leaves the state intact, and only remove/clear the .kin/split-state
after the final set_head (and all other operations) succeed; also add an
integration test that injects failures at each mutation step to verify that the
presence of .kin/split-state enables a continue/abort recovery path that reads
and replays or rolls back the recorded operations.
In `@tests/split_tests.rs`:
- Around line 268-273: The test currently only verifies branch creation and that
HEAD is not detached; update it to assert that HEAD points to the new branch and
still peels to the original commit id: call repo.head() (not just
repo.head_detached()) and assert its reference name / shorthand corresponds to
"new-feat" (the same branch found by repo.find_branch("new-feat", ...)), then
verify the peeled target of that HEAD reference equals head_id (similar to the
existing branch.get().target() check) so the test fails if HEAD is attached to
the wrong branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 087aa07d-1ac5-4a3c-814b-b714ce1fc3f9
📒 Files selected for processing (10)
README.mddocs/cli_reference.mdsrc/commands/mod.rssrc/commands/pr.rssrc/commands/split.rssrc/main.rssrc/stack.rstests/pr_tests.rstests/restack_tests.rstests/split_tests.rs
| if !std::io::stdin().is_terminal() { | ||
| if let Ok(test_title) = std::env::var("KIN_TEST_PR_EDIT_TITLE") { | ||
| println!(" [non-interactive] Using title override: {}", test_title); | ||
| return Ok(test_title); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting this test-only environment variable.
The KIN_TEST_PR_EDIT_TITLE environment variable is added for non-interactive testing, which is a reasonable approach. Consider adding a comment noting this is for testing purposes only, or documenting it in a testing guide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/pr.rs` around lines 1391 - 1395, Add documentation and an inline
comment clarifying that the KIN_TEST_PR_EDIT_TITLE environment variable used in
the non-interactive branch (the code that checks !std::io::stdin().is_terminal()
and returns test_title) is strictly for tests; annotate that block (near the
read of KIN_TEST_PR_EDIT_TITLE) with a comment like "test-only: override PR edit
title in CI/non-interactive environments" and add a short note to the repository
testing guide or README describing the variable's purpose and usage to avoid
accidental use in production.
| fn apply_split( | ||
| repo: &Repository, | ||
| next_branches: HashMap<String, String>, | ||
| new_branch_map: Vec<(String, String)>, | ||
| initial_branches: Vec<String>, | ||
| allowed_ids: HashSet<String>, | ||
| ) -> Result<()> { |
There was a problem hiding this comment.
Persist recovery state before the final HEAD rewrite.
apply_split mutates multiple refs (branch deletions, creations, and moves) without persisting a recovery state. This new set_head operation adds a late failure point after destructive ref updates may have already succeeded. If any error occurs during the final HEAD adjustment or afterward, kin split is left in a partially applied state with no continue/abort recovery path. Implement a persisted state file (e.g., .kin/split-state) that records the planned ref changes before any mutations occur. Write this state immediately after validation succeeds but before the first deletion. Only clear the state file after all operations—including the final HEAD rewrite—complete successfully. This also requires an integration test that simulates failure at each mutation step and verifies that the incomplete state allows recovery.
Also applies to: 329-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/split.rs` around lines 210 - 216, The apply_split function
mutates refs (see apply_split and the variables next_branches, new_branch_map,
initial_branches, allowed_ids) but currently writes HEAD last via set_head
without a persisted recovery state; add a durable state file (e.g.,
.kin/split-state) that records the planned ref changes and metadata immediately
after validation succeeds and before performing any ref
deletions/creations/moves, ensure every mutation step checks/updates or at least
leaves the state intact, and only remove/clear the .kin/split-state after the
final set_head (and all other operations) succeed; also add an integration test
that injects failures at each mutation step to verify that the presence of
.kin/split-state enables a continue/abort recovery path that reads and replays
or rolls back the recorded operations.
| assert!(!repo.head_detached().unwrap()); | ||
| let branch = repo | ||
| .find_branch("new-feat", git2::BranchType::Local) | ||
| .unwrap(); | ||
| assert_eq!(branch.get().target().unwrap(), head_id); | ||
| } |
There was a problem hiding this comment.
Assert the checked-out ref, not just branch creation.
These assertions still pass if kin split attaches HEAD to the wrong branch, as long as new-feat exists and HEAD stops being detached. Please also assert that HEAD is on new-feat (and ideally still peels to head_id) so this regression actually proves the checkout behavior.
💡 Tighten the assertion
assert!(!repo.head_detached().unwrap());
+ assert_eq!(repo.head().unwrap().shorthand(), Some("new-feat"));
+ assert_eq!(repo.head().unwrap().peel_to_commit().unwrap().id(), head_id);
let branch = repo
.find_branch("new-feat", git2::BranchType::Local)
.unwrap();
assert_eq!(branch.get().target().unwrap(), head_id);As per coding guidelines, "Any identified bug or edge case (e.g., panics, incorrect state) MUST be reproduced with a permanent test case before the fix is applied".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/split_tests.rs` around lines 268 - 273, The test currently only
verifies branch creation and that HEAD is not detached; update it to assert that
HEAD points to the new branch and still peels to the original commit id: call
repo.head() (not just repo.head_detached()) and assert its reference name /
shorthand corresponds to "new-feat" (the same branch found by
repo.find_branch("new-feat", ...)), then verify the peeled target of that HEAD
reference equals head_id (similar to the existing branch.get().target() check)
so the test fails if HEAD is attached to the wrong branch.
Pajn
force-pushed
the
optimize-sync
branch
4 times, most recently
from
00e0d8d to
ade4767
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stack.rs (1)
932-956:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't stop at the first tree/patch candidate.
Both paths pick the first locally-matching
target_indexand only then callvalidate_floating_match. In histories with duplicate trees or repeated patch-ids, the first candidate can fail validation while an older candidate would pass, so this returns a false negative. Keep scanning until one candidate both matches locally and survives validation.As per coding guidelines,
tests/**/*.rs: Any identified bug or edge case (e.g., panics, incorrect state) MUST be reproduced with a permanent test case before the fix is applied.Also applies to: 1059-1087
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stack.rs` around lines 932 - 956, The loop currently captures the first locally-matching target_index then validates it, causing false negatives when that candidate fails validation; change the logic in the loop over target.candidates so that for each candidate where candidate.tree_id == commit.tree_id() and floating_parent_matches_candidate(...) returns true you construct a FloatingBaseMatch (using branch_id=oid and the current target_index) and call validate_floating_match(repo, branch_tip, matching, target, patch_id_cache), returning Ok(Some(matching)) only if validation succeeds, otherwise continue scanning remaining candidates; apply the same change to the analogous block around lines 1059-1087; add a permanent test under tests/**/*.rs that reproduces the duplicate-tree/duplicate-patch-id false-negative before the fix and verifies the corrected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/stack.rs`:
- Around line 811-823: The code resets private_len to commit_ids.len() when it
computes to 0, which re-includes the full history and breaks the private-lineage
filter; instead preserve a zero private_len so target.candidates can be empty
and ensure find_floating_match returns Ok(None) when target.candidates is empty;
specifically, stop the fallback that sets private_len = commit_ids.len()
(originating from floating_patch_id_boundary and the private_len calculation),
update find_floating_match to handle an empty target.candidates by returning
Ok(None) rather than erroring, and add a permanent test under tests/**/*.rs that
reproduces the zero-private-lineage case so the fix is covered.
---
Outside diff comments:
In `@src/stack.rs`:
- Around line 932-956: The loop currently captures the first locally-matching
target_index then validates it, causing false negatives when that candidate
fails validation; change the logic in the loop over target.candidates so that
for each candidate where candidate.tree_id == commit.tree_id() and
floating_parent_matches_candidate(...) returns true you construct a
FloatingBaseMatch (using branch_id=oid and the current target_index) and call
validate_floating_match(repo, branch_tip, matching, target, patch_id_cache),
returning Ok(Some(matching)) only if validation succeeds, otherwise continue
scanning remaining candidates; apply the same change to the analogous block
around lines 1059-1087; add a permanent test under tests/**/*.rs that reproduces
the duplicate-tree/duplicate-patch-id false-negative before the fix and verifies
the corrected behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8ecb675-c2ce-4857-81dc-ca23ed30337d
📒 Files selected for processing (2)
src/stack.rstests/restack_tests.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit
Bug Fixes
Tests