From 2c33584d81d065e97ee3a57ae9bb1173994b6709 Mon Sep 17 00:00:00 2001 From: congziqi Date: Mon, 1 Jun 2026 15:33:00 +0800 Subject: [PATCH] fix(rebase): consume empty mapping segments Persist skipped rebase events when a completed rebase segment produces no commit mapping, and treat those skipped events as processed so stale no-op segments cannot be selected by later rebases to the same target. Prefer semantic rebase heads before target-only segment matching to reduce stale segment selection when multiple rebases share the same target hint. Fixes #1476 --- src/daemon.rs | 50 ++++++++++-- src/git/repo_state.rs | 28 +++++++ src/git/rewrite_log.rs | 33 ++++++++ tests/integration/main.rs | 1 + tests/integration/rebase_empty_mapping.rs | 92 +++++++++++++++++++++++ 5 files changed, 199 insertions(+), 5 deletions(-) create mode 100644 tests/integration/rebase_empty_mapping.rs diff --git a/src/daemon.rs b/src/daemon.rs index 88c91fdea1..90c810bd75 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -18,7 +18,8 @@ use crate::git::repo_state::{ use crate::git::repository::{Repository, discover_repository_in_path_no_git_exec, exec_git}; use crate::git::rewrite_log::{ CherryPickAbortEvent, CherryPickCompleteEvent, MergeSquashEvent, RebaseAbortEvent, - RebaseCompleteEvent, ResetEvent, ResetKind, RewriteLogEvent, StashEvent, StashOperation, + RebaseCompleteEvent, RebaseSkippedEvent, ResetEvent, ResetKind, RewriteLogEvent, StashEvent, + StashOperation, }; use crate::git::sync_authorship::{fetch_authorship_notes, fetch_remote_from_args}; use crate::utils::LockFile; @@ -952,6 +953,7 @@ fn stable_carryover_heads_for_command( input.worktree, input.argv, rebase_start_target_hint.as_deref(), + None, )? .map(|(old_head, new_head, _onto_head)| (old_head, new_head)) .or_else(|| { @@ -2981,8 +2983,14 @@ type RebaseCommitMappings = (Vec, Vec); fn processed_rebase_new_heads(repository: &Repository) -> Result, GitAiError> { let mut out = HashSet::new(); for event in repository.storage.read_rewrite_events()? { - if let RewriteLogEvent::RebaseComplete { rebase_complete } = event { - out.insert(rebase_complete.new_head); + match event { + RewriteLogEvent::RebaseComplete { rebase_complete } => { + out.insert(rebase_complete.new_head); + } + RewriteLogEvent::RebaseSkipped { rebase_skipped } => { + out.insert(rebase_skipped.new_head); + } + _ => {} } } Ok(out) @@ -6313,10 +6321,15 @@ impl ActorDaemonCoordinator { worktree: &Path, argv: &[String], start_target_hint: Option<&str>, + semantic_heads: Option<(&str, &str)>, ) -> Result, GitAiError> { let processed_new_heads = processed_rebase_new_heads(repository)?; - let mut segment = - resolve_rebase_segment_for_worktree(worktree, start_target_hint, &processed_new_heads)?; + let mut segment = resolve_rebase_segment_for_worktree( + worktree, + start_target_hint, + &processed_new_heads, + semantic_heads, + )?; let Some(mut segment) = segment.take() else { return Ok(None); }; @@ -6555,12 +6568,25 @@ impl ActorDaemonCoordinator { })?; let repository = repository_for_rewrite_context(cmd, "rebase_complete")?; let start_target_hint = rebase_start_target_hint_from_command(cmd); + let semantic_heads = if !old_head.is_empty() + && !new_head.is_empty() + && old_head != new_head + && is_valid_oid(old_head) + && !is_zero_oid(old_head) + && is_valid_oid(new_head) + && !is_zero_oid(new_head) + { + Some((old_head.as_str(), new_head.as_str())) + } else { + None + }; let (mapping_old_head, stable_new_head, onto_head) = if let Some(heads) = Self::stable_rebase_heads_from_worktree( &repository, worktree, &cmd.raw_argv, start_target_hint.as_deref(), + semantic_heads, )? { heads } else if !old_head.is_empty() && !new_head.is_empty() && old_head != new_head { @@ -6627,6 +6653,12 @@ impl ActorDaemonCoordinator { sid = %cmd.root_sid, "rebase complete: commit mapping produced no commits; authorship notes will NOT be rewritten for this rebase" ); + out.push(RewriteLogEvent::rebase_skipped(RebaseSkippedEvent::new( + mapping_old_head, + stable_new_head, + *interactive, + "empty_commit_mapping".to_string(), + ))); } if let Some(worktree) = cmd.worktree.as_ref() { self.clear_pending_rebase_original_head_for_worktree(worktree)?; @@ -6809,6 +6841,7 @@ impl ActorDaemonCoordinator { worktree, &cmd.raw_argv, None, + None, )? else { tracing::debug!( @@ -6836,6 +6869,13 @@ impl ActorDaemonCoordinator { original_commits, new_commits, ))); + } else { + out.push(RewriteLogEvent::rebase_skipped(RebaseSkippedEvent::new( + mapping_old_head, + new_head, + false, + "empty_commit_mapping".to_string(), + ))); } if let Some(worktree) = cmd.worktree.as_ref() { self.clear_pending_rebase_original_head_for_worktree(worktree)?; diff --git a/src/git/repo_state.rs b/src/git/repo_state.rs index 37c6c4762d..28af512896 100644 --- a/src/git/repo_state.rs +++ b/src/git/repo_state.rs @@ -462,12 +462,40 @@ pub fn resolve_rebase_segment_for_worktree( worktree: &Path, start_target_hint: Option<&str>, already_processed_new_heads: &std::collections::HashSet, + semantic_heads: Option<(&str, &str)>, ) -> Result, GitAiError> { let candidates = read_complete_rebase_segments_for_worktree(worktree)? .into_iter() .filter(|segment| !already_processed_new_heads.contains(&segment.new_head)) .collect::>(); + if let Some((semantic_old, semantic_new)) = semantic_heads { + if is_valid_git_oid(semantic_old) + && is_valid_git_oid(semantic_new) + && let Some(segment) = candidates.iter().find(|segment| { + segment.original_head == semantic_old && segment.new_head == semantic_new + }) + { + return Ok(Some(segment.clone())); + } + + if is_valid_git_oid(semantic_new) + && let Some(segment) = candidates + .iter() + .find(|segment| segment.new_head == semantic_new) + { + return Ok(Some(segment.clone())); + } + + if is_valid_git_oid(semantic_old) + && let Some(segment) = candidates + .iter() + .find(|segment| segment.original_head == semantic_old) + { + return Ok(Some(segment.clone())); + } + } + if let Some(start_target_hint) = start_target_hint && let Some(segment) = candidates .iter() diff --git a/src/git/rewrite_log.rs b/src/git/rewrite_log.rs index e544e01e9a..3887c22093 100644 --- a/src/git/rewrite_log.rs +++ b/src/git/rewrite_log.rs @@ -18,6 +18,9 @@ pub enum RewriteLogEvent { RebaseComplete { rebase_complete: RebaseCompleteEvent, }, + RebaseSkipped { + rebase_skipped: RebaseSkippedEvent, + }, RebaseAbort { rebase_abort: RebaseAbortEvent, }, @@ -88,6 +91,12 @@ impl RewriteLogEvent { } } + pub fn rebase_skipped(event: RebaseSkippedEvent) -> Self { + Self::RebaseSkipped { + rebase_skipped: event, + } + } + pub fn rebase_abort(event: RebaseAbortEvent) -> Self { Self::RebaseAbort { rebase_abort: event, @@ -264,6 +273,30 @@ impl RebaseCompleteEvent { } } +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct RebaseSkippedEvent { + pub original_head: String, + pub new_head: String, + pub is_interactive: bool, + pub reason: String, +} + +impl RebaseSkippedEvent { + pub fn new( + original_head: String, + new_head: String, + is_interactive: bool, + reason: String, + ) -> Self { + Self { + original_head, + new_head, + is_interactive, + reason, + } + } +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RebaseAbortEvent { pub original_head: String, diff --git a/tests/integration/main.rs b/tests/integration/main.rs index 3763f17691..1f469530ac 100644 --- a/tests/integration/main.rs +++ b/tests/integration/main.rs @@ -99,6 +99,7 @@ mod rebase; mod rebase_attribution_remaining; mod rebase_authorship_unit; mod rebase_benchmark; +mod rebase_empty_mapping; mod rebase_hooks_unit; mod rebase_merge_commit_note_leak; mod rebase_note_integrity; diff --git a/tests/integration/rebase_empty_mapping.rs b/tests/integration/rebase_empty_mapping.rs new file mode 100644 index 0000000000..698f60cf12 --- /dev/null +++ b/tests/integration/rebase_empty_mapping.rs @@ -0,0 +1,92 @@ +use crate::repos::test_file::ExpectedLineExt; +use crate::repos::test_repo::TestRepo; +use std::fs; + +/// Regression for #1476. +/// +/// A rebase whose only replayed commit is already present upstream produces a +/// complete reflog segment, but no commit mapping. If that no-op segment is left +/// unprocessed, a later real rebase to the same target can select the stale +/// segment and skip authorship note rewriting for the new commit. +#[test] +fn test_noop_rebase_segment_does_not_steal_later_rebase_mapping() { + let repo = TestRepo::new(); + + let shared_path = repo.path().join("shared.txt"); + fs::write(&shared_path, "base\n").unwrap(); + repo.git_ai(&["checkpoint", "mock_known_human", "shared.txt"]) + .unwrap(); + repo.stage_all_and_commit("base").unwrap(); + let default_branch = repo.current_branch(); + + let mut shared = repo.filename("shared.txt"); + shared.assert_committed_lines(crate::lines!["base".human()]); + + repo.git(&["checkout", "-b", "upstream-noop"]).unwrap(); + fs::write(&shared_path, "base\nequivalent\n").unwrap(); + repo.git_ai(&["checkpoint", "mock_known_human", "shared.txt"]) + .unwrap(); + repo.stage_all_and_commit("upstream equivalent patch") + .unwrap(); + shared.assert_committed_lines(crate::lines!["base".human(), "equivalent".human()]); + + repo.git(&["checkout", &default_branch]).unwrap(); + repo.git(&["checkout", "-b", "feature-noop"]).unwrap(); + fs::write(&shared_path, "base\nequivalent\n").unwrap(); + repo.git_ai(&["checkpoint", "mock_known_human", "shared.txt"]) + .unwrap(); + repo.stage_all_and_commit("feature equivalent patch") + .unwrap(); + shared.assert_committed_lines(crate::lines!["base".human(), "equivalent".human()]); + + repo.git(&["rebase", "upstream-noop"]).unwrap(); + shared.assert_committed_lines(crate::lines!["base".human(), "equivalent".human()]); + + let ai_path = repo.path().join("ai.txt"); + fs::write(&ai_path, "AI line 1\n").unwrap(); + repo.git_ai(&["checkpoint", "mock_ai", "ai.txt"]).unwrap(); + let ai_commit = repo.stage_all_and_commit("add ai work").unwrap(); + let mut ai_file = repo.filename("ai.txt"); + ai_file.assert_committed_lines(crate::lines!["AI line 1".ai()]); + assert!( + repo.read_authorship_note(&ai_commit.commit_sha).is_some(), + "AI commit should have an authorship note before amend/rebase" + ); + + fs::write(&ai_path, "AI line 1\nAI line 2\n").unwrap(); + repo.git_ai(&["checkpoint", "mock_ai", "ai.txt"]).unwrap(); + repo.git(&["add", "ai.txt"]).unwrap(); + repo.git(&["commit", "--amend", "-m", "add amended ai work"]) + .unwrap(); + let amended_sha = repo.git(&["rev-parse", "HEAD"]).unwrap().trim().to_string(); + ai_file.assert_committed_lines(crate::lines!["AI line 1".ai(), "AI line 2".ai()]); + assert!( + repo.read_authorship_note(&amended_sha).is_some(), + "Amended AI commit should have an authorship note before the second rebase" + ); + + repo.git(&["checkout", "upstream-noop"]).unwrap(); + let upstream_path = repo.path().join("upstream.txt"); + fs::write(&upstream_path, "new upstream line\n").unwrap(); + repo.git_ai(&["checkpoint", "mock_known_human", "upstream.txt"]) + .unwrap(); + repo.stage_all_and_commit("advance upstream").unwrap(); + let mut upstream_file = repo.filename("upstream.txt"); + upstream_file.assert_committed_lines(crate::lines!["new upstream line".human()]); + + repo.git(&["checkout", "feature-noop"]).unwrap(); + repo.git(&["rebase", "upstream-noop"]).unwrap(); + + let rebased_sha = repo.git(&["rev-parse", "HEAD"]).unwrap().trim().to_string(); + let note = repo.read_authorship_note(&rebased_sha); + assert!( + note.is_some(), + "Rebased AI commit should keep its authorship note" + ); + ai_file.assert_committed_lines(crate::lines!["AI line 1".ai(), "AI line 2".ai()]); + + let stats = repo.stats().unwrap(); + assert_eq!(stats.git_diff_added_lines, 2); + assert_eq!(stats.ai_additions, 2); + assert_eq!(stats.unknown_additions, 0); +}