Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 45 additions & 5 deletions src/daemon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(|| {
Expand Down Expand Up @@ -2981,8 +2983,14 @@ type RebaseCommitMappings = (Vec<String>, Vec<String>);
fn processed_rebase_new_heads(repository: &Repository) -> Result<HashSet<String>, 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)
Expand Down Expand Up @@ -6313,10 +6321,15 @@ impl ActorDaemonCoordinator {
worktree: &Path,
argv: &[String],
start_target_hint: Option<&str>,
semantic_heads: Option<(&str, &str)>,
) -> Result<Option<(String, String, String)>, 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);
};
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -6809,6 +6841,7 @@ impl ActorDaemonCoordinator {
worktree,
&cmd.raw_argv,
None,
None,
)?
else {
tracing::debug!(
Expand Down Expand Up @@ -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)?;
Expand Down
28 changes: 28 additions & 0 deletions src/git/repo_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
semantic_heads: Option<(&str, &str)>,
) -> Result<Option<RebaseReflogSegment>, GitAiError> {
let candidates = read_complete_rebase_segments_for_worktree(worktree)?
.into_iter()
.filter(|segment| !already_processed_new_heads.contains(&segment.new_head))
.collect::<Vec<_>>();

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()
Expand Down
33 changes: 33 additions & 0 deletions src/git/rewrite_log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ pub enum RewriteLogEvent {
RebaseComplete {
rebase_complete: RebaseCompleteEvent,
},
RebaseSkipped {
rebase_skipped: RebaseSkippedEvent,
},
RebaseAbort {
rebase_abort: RebaseAbortEvent,
},
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions tests/integration/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
92 changes: 92 additions & 0 deletions tests/integration/rebase_empty_mapping.rs
Original file line number Diff line number Diff line change
@@ -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);
}