Skip to content

Add typed merge and rebase recovery commands#71

Merged
cosentinode merged 3 commits into
developfrom
issue-54-merge-rebase-recovery
Jul 14, 2026
Merged

Add typed merge and rebase recovery commands#71
cosentinode merged 3 commits into
developfrom
issue-54-merge-rebase-recovery

Conversation

@cosentinode

Copy link
Copy Markdown
Owner

Summary

  • add typed merge/rebase continue, abort, and rebase-skip execution at the Git boundary
  • revalidate matching operation state and reject unsupported or unresolved recovery commands
  • disable interactive editors while preserving standard Git hooks and signing behavior
  • cover every action, state rejection, next-conflict reporting, hooks, and signing in temporary repositories

Verification

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

Fixes #54

@cosentinode

Copy link
Copy Markdown
Owner Author

Review findings

  1. [P1] Preserve rebase identity when a rebase is recreating a merge commit (crates/bitbygit-git/src/lib.rs:549-558, affecting recover at lines 222-258). A standard git rebase --rebase-merges that conflicts while replaying a merge leaves both .git/rebase-merge and .git/MERGE_HEAD. Because repository_operation() checks MERGE_HEAD first, status() reports Merge: recover(Rebase, Abort|Continue|Skip) is rejected as mismatched, while recover(Merge, Abort) is accepted and returns success after removing only MERGE_HEAD, leaving the rebase active. I reproduced this with Git 2.55.0; after git merge --abort, .git/rebase-merge remained and the repository was still detached in the rebase. This violates the abort/non-active-state contract and can discard the current merge resolution while falsely presenting the operation as recovered. Rebase state needs precedence (or explicit nested-state handling), with a temporary-repository regression test for a conflicted --rebase-merges merge step.

  2. [P2] Report successful recovery even when hooks emit non-UTF-8 output (crates/bitbygit-git/src/lib.rs:967-999). Recovery intentionally runs commit hooks, but run_args_with_editor decodes captured output with strict String::from_utf8 before considering the successful exit status. A successful merge --continue whose executable commit-msg hook does printf "\377" creates the merge commit and clears MERGE_HEAD, then this API returns GitError::Utf8. The caller is told recovery failed even though retrying is no longer possible, contrary to the requirement to report completion accurately. Preserve raw/lossy output for this execution path and add a hook-output regression test.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed both posted review findings in commit f44b3fc:

  • Rebase metadata now takes precedence over MERGE_HEAD, so a conflicted git rebase --rebase-merges merge step remains classified as Rebase. Merge recovery fails closed in that state, while rebase abort, continue, and skip use the correct Git operation.
  • Successful recovery commands decode captured hook output lossily, so non-UTF-8 bytes do not turn an already-completed recovery into GitError::Utf8; unrelated Git execution remains strict.
  • Added temporary-repository regressions that create a genuine conflicted --rebase-merges merge step and an executable commit-msg hook emitting \377.

Verification (all passed):

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (all workspace unit and doc tests passed; bitbygit-git: 63 passed)

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review found zero concerns.

Reviewed the complete PR diff and commit history against issue #54, relevant Git operation-state and command-execution code, temporary-repository tests, existing PR feedback, and CI status. I specifically checked correctness and data-loss risks around merge/rebase continue, abort, skip, rebase-merges nested merge state, mismatched/absent operation rejection, editor suppression, hook/signing preservation, non-UTF-8 hook output, and next-conflict reporting.

Local verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode

Copy link
Copy Markdown
Owner Author

Review finding

  1. [P2] Preserve the real Git failure when recovery output is non-UTF-8 (crates/bitbygit-git/src/lib.rs:975-992). run_args_with_editor switches to lossy decoding only when output.status.success(). Recovery commands deliberately run hooks and signing, so failure output can also contain arbitrary bytes. For example, the existing merge hook/signing test becomes a reproducer if the successful commit-msg hook also prints \377: signing still fails and the merge remains active, but recover returns GitError::Utf8 before constructing GitError::GitFailed, hiding the signing failure. A multi-step rebase can similarly advance after a resolved commit and then stop at the next conflict while invalid hook output masks that conflict, violating the requirement to report the next conflict accurately. Decode both streams lossily for every recovery result, regardless of exit status, and add a failed-recovery regression (ideally the next-conflict path).

Local verification on f44b3fc: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace all passed. CI is green.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the non-UTF-8 failed-recovery finding in commit efc0cba.

  • Recovery commands now decode stdout and stderr lossily on both success and failure, preserving the underlying GitError::GitFailed.
  • The hook/signing regression now emits \377, verifies the replacement character is retained in captured output, verifies signing still ran, and verifies the merge remains active after the real Git failure.

Verification (all passed):

  • cargo test --locked -p bitbygit-git merge_continue_runs_hooks_and_does_not_bypass_signing
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace (all workspace unit and doc tests passed; bitbygit-git: 63 passed)

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review of current head efc0cba1fd5ce9554c3e2d8f8b587c62a57e517a: zero concerns.

Reviewed the complete diff and commit history against issue #54, surrounding Git operation-state and process-execution code, temporary-repository tests, output/error handling, and current checks. Verified correctness, Git semantics, data-loss and race risks, security, hooks/signing, editor suppression, nested rebase-merge state, non-UTF-8 output, and missing-test risk.

Local verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

CI checks are green.

@cosentinode cosentinode merged commit ef2a9cb into develop Jul 14, 2026
2 checks passed
@cosentinode cosentinode deleted the issue-54-merge-rebase-recovery branch July 14, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Phase 12] Add typed merge and rebase recovery commands

1 participant