diff --git a/apps/staged/src-tauri/src/branches.rs b/apps/staged/src-tauri/src/branches.rs index 9330d8f96..c4d29ca3a 100644 --- a/apps/staged/src-tauri/src/branches.rs +++ b/apps/staged/src-tauri/src/branches.rs @@ -347,7 +347,7 @@ pub(crate) fn resolve_branch_clone_dir( } pub(crate) fn resolve_branch_workspace_subpath( - store: &Arc, + store: &Store, branch: &store::Branch, ) -> Result, String> { let Some(repo_id) = branch.project_repo_id.as_deref() else { diff --git a/apps/staged/src-tauri/src/commit_reassociation.rs b/apps/staged/src-tauri/src/commit_reassociation.rs new file mode 100644 index 000000000..8239480fc --- /dev/null +++ b/apps/staged/src-tauri/src/commit_reassociation.rs @@ -0,0 +1,489 @@ +//! Reattach commit metadata to rewritten SHAs after a rebase. +//! +//! Staged links each commit to the session that authored it by SHA. A rebase +//! gives every commit on the branch a new SHA, which orphans every one of +//! those rows: the timeline's SHA lookup misses, the commits lose their +//! session, and their reviews get hidden by `review_is_visible_in_timeline`. +//! +//! The mapping is recoverable from the DB plus git alone — no pre-rebase +//! capture, so this also survives an app restart mid-pipeline. `git rebase` +//! preserves author email, author date, and subject; only the SHA and the +//! committer fields change (conflict resolution doesn't touch author metadata, +//! and `--signoff` only appends a body trailer). The orphaned rows still hold +//! the old SHAs, and the old commit objects stay resolvable in the repo, so we +//! can read the old metadata back and match it against the rewritten commits. + +use std::collections::{HashMap, HashSet, VecDeque}; +use std::path::Path; + +use crate::store::Store; + +/// The commit metadata `git rebase` carries across a rewrite. Two commits with +/// the same identity are the same commit before and after a rebase. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct CommitIdentity { + pub author_email: String, + /// Author date as git's raw `%at` (unix seconds), compared as text. + pub author_timestamp: String, + pub subject: String, +} + +/// A `commits` row whose SHA is no longer on the branch. +#[derive(Debug, Clone)] +pub struct OrphanedRow { + pub row_id: String, + pub old_sha: String, + pub identity: CommitIdentity, +} + +/// A commit currently on the branch, as a candidate for an orphaned row. +#[derive(Debug, Clone)] +pub struct RewrittenCommit { + pub sha: String, + pub identity: CommitIdentity, + /// Whether a `commits` row already owns this SHA. Claimed commits are + /// never handed to an orphaned row — the existing row wins. + pub claimed: bool, +} + +/// A row to repoint, produced by [`match_rewritten_commits`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ShaRemap { + pub row_id: String, + pub old_sha: String, + pub new_sha: String, +} + +/// `%H`, `%ae`, `%at`, `%s`, separated by `%x1f` (the unit separator, as in +/// `BRANCH_COMMIT_LOG_FORMAT`) since git technically permits `|` in emails; +/// the subject still goes last as the remainder. Deliberately not +/// `CommitInfo`'s format, which carries `%ct` (committer time), the one +/// timestamp a rebase rewrites. +const REASSOCIATION_LOG_FORMAT: &str = "--format=%H%x1f%ae%x1f%at%x1f%s"; + +/// Pair orphaned rows with the commits that replaced them. +/// +/// Both inputs must be in branch order, oldest first: when several commits +/// share an identity (two `wip` commits authored in the same second, say), +/// they're paired oldest-to-oldest. Unmatched rows are simply left out — the +/// caller leaves them orphaned, which is what happens today anyway. +pub fn match_rewritten_commits( + orphans: &[OrphanedRow], + rewritten: &[RewrittenCommit], +) -> Vec { + let mut available: HashMap<&CommitIdentity, VecDeque<&str>> = HashMap::new(); + for commit in rewritten.iter().filter(|c| !c.claimed) { + available + .entry(&commit.identity) + .or_default() + .push_back(&commit.sha); + } + + let mut remaps = Vec::new(); + for orphan in orphans { + let Some(candidates) = available.get_mut(&orphan.identity) else { + continue; + }; + let Some(new_sha) = candidates.pop_front() else { + continue; + }; + remaps.push(ShaRemap { + row_id: orphan.row_id.clone(), + old_sha: orphan.old_sha.clone(), + new_sha: new_sha.to_string(), + }); + } + remaps +} + +/// Repoint a branch's orphaned commit rows (and their reviews) at the SHAs a +/// rebase rewrote them into. Returns how many rows were remapped. +/// +/// Safe to call when nothing was rewritten: with no orphaned rows it stops +/// after listing the branch and returns 0. Also safe to call while a rebase is +/// still in flight — it returns 0 without touching a row (see +/// [`head_is_on_branch`]). +pub fn reassociate_after_rebase( + store: &Store, + branch_id: &str, + working_dir: &Path, + workspace_name: Option<&str>, +) -> Result { + let (branch, git) = branch_git_runner(store, branch_id, working_dir, workspace_name)?; + + let branch_name = crate::git::branch_name_without_origin(&branch.branch_name); + if !head_is_on_branch(&git, branch_name) { + log::warn!( + "Skipping commit reassociation on branch {branch_id}: HEAD isn't on {branch_name} \ + (rebase still in progress?)" + ); + return Ok(0); + } + + let base_ref = crate::git::origin_ref_for_branch(&branch.base_branch); + let on_branch = list_branch_commits(&git, &base_ref)?; + + // `list_commits_for_branch` orders by `created_at`, which is the order the + // sessions authored them — i.e. branch order, as the matcher requires. + let rows = store + .list_commits_for_branch(branch_id) + .map_err(|e| e.to_string())?; + let owned: HashSet<&str> = rows.iter().filter_map(|row| row.sha.as_deref()).collect(); + + let rewritten: Vec = on_branch + .into_iter() + .map(|(sha, identity)| RewrittenCommit { + claimed: owned.contains(sha.as_str()), + sha, + identity, + }) + .collect(); + + let still_on_branch: HashSet<&str> = rewritten.iter().map(|c| c.sha.as_str()).collect(); + let orphan_shas: Vec = owned + .iter() + .filter(|sha| !still_on_branch.contains(*sha)) + .map(|sha| (*sha).to_string()) + .collect(); + if orphan_shas.is_empty() { + return Ok(0); + } + + let mut old_identities = lookup_commit_identities(&git, &orphan_shas)?; + let orphans: Vec = rows + .iter() + .filter_map(|row| { + let old_sha = row.sha.clone()?; + // GC-pruned objects drop out here, leaving their row orphaned. + let identity = old_identities.remove(&old_sha)?; + Some(OrphanedRow { + row_id: row.id.clone(), + old_sha, + identity, + }) + }) + .collect(); + + let remaps = match_rewritten_commits(&orphans, &rewritten); + if remaps.is_empty() { + return Ok(0); + } + + let pairs: Vec<(&str, &str, &str)> = remaps + .iter() + .map(|r| (r.row_id.as_str(), r.old_sha.as_str(), r.new_sha.as_str())) + .collect(); + store + .remap_commit_shas(branch_id, &pairs) + .map_err(|e| e.to_string()) +} + +/// The git runner [`branch_git_runner`] hands back. Boxed rather than an `impl +/// Fn`, since a named type keeps the return signature readable and every +/// consumer here is generic over `Fn` anyway. +type GitRunner<'a> = Box Result + 'a>; + +/// Resolve the branch row and build the git runner the entry points share: +/// local commands run in `working_dir`, remote ones through the branch's +/// workspace (and its repo subpath). +fn branch_git_runner<'a>( + store: &Store, + branch_id: &str, + working_dir: &'a Path, + workspace_name: Option<&'a str>, +) -> Result<(crate::store::Branch, GitRunner<'a>), String> { + let branch = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + + let repo_subpath = match workspace_name { + Some(_) => crate::branches::resolve_branch_workspace_subpath(store, &branch)?, + None => None, + }; + let git = move |args: &[&str]| -> Result { + match workspace_name { + Some(ws_name) => { + crate::branches::run_workspace_git(ws_name, repo_subpath.as_deref(), args) + .map_err(|e| e.to_string()) + } + None => crate::git::cli_run_smart(working_dir, args).map_err(|e| e.to_string()), + } + }; + Ok((branch, Box::new(git))) +} + +/// Whether HEAD is attached to this branch — i.e. no rebase is in flight +/// (see [`head_is_on_branch`] for why that's the same question). For callers +/// that need the answer *before* touching any rows, like the post-completion +/// commit detection. Errors read as "not attached", the safe direction. +pub fn head_is_attached_to_branch( + store: &Store, + branch_id: &str, + working_dir: &Path, + workspace_name: Option<&str>, +) -> bool { + match branch_git_runner(store, branch_id, working_dir, workspace_name) { + Ok((branch, git)) => head_is_on_branch( + &git, + crate::git::branch_name_without_origin(&branch.branch_name), + ), + Err(e) => { + log::warn!("Failed to check whether HEAD is attached to branch {branch_id}: {e}"); + false + } + } +} + +/// Whether HEAD is the branch we're about to reassociate, rather than a +/// detached commit. +/// +/// This is how we tell a finished rebase from one still in flight: a rebase +/// detaches HEAD for the whole rewrite and only moves the branch ref at the +/// end. If the agent stops with conflicts unresolved — or its turn simply ends +/// — HEAD sits on a partially applied commit, and repointing rows there would +/// be strictly worse than leaving them orphaned: a later `git rebase --abort` +/// restores the original SHAs, and the rows (plus their reviews) would then +/// name commits that are on no branch at all. Any other detached or +/// wrong-branch HEAD is skipped for the same reason — `merge-base..HEAD` isn't +/// the branch's history, so nothing it lists can be trusted as a rewrite of it. +fn head_is_on_branch(git: &F, branch_name: &str) -> bool +where + F: Fn(&[&str]) -> Result, +{ + // Exits non-zero on a detached HEAD; a failure for any other reason also + // reads as "don't touch anything", which is the safe direction. + match git(&["symbolic-ref", "--quiet", "--short", "HEAD"]) { + Ok(head) => head.trim() == branch_name, + Err(_) => false, + } +} + +/// List the branch's commits as `(sha, identity)` pairs, oldest first. +fn list_branch_commits(git: &F, base_ref: &str) -> Result, String> +where + F: Fn(&[&str]) -> Result, +{ + // Fall back to the bare base ref the way the timeline does, so a repo + // without a shared history with `origin/{base}` still reports something. + let range = match git(&["merge-base", base_ref, "HEAD"]) { + Ok(output) if !output.trim().is_empty() => format!("{}..HEAD", output.trim()), + _ => format!("{base_ref}..HEAD"), + }; + let output = git(&["log", REASSOCIATION_LOG_FORMAT, &range, "--"])?; + + // `git log` is newest-first; the matcher wants branch order, oldest first. + Ok(output + .lines() + .filter_map(parse_identity_line) + .rev() + .collect()) +} + +/// Batch-read metadata for commits that are no longer on any branch. The old +/// objects survive a rebase (the reflog keeps them alive), and +/// `--ignore-missing` silently drops any that have since been GC-pruned. +fn lookup_commit_identities( + git: &F, + shas: &[String], +) -> Result, String> +where + F: Fn(&[&str]) -> Result, +{ + // Guard the empty case: `git log --no-walk` with no revisions defaults to + // HEAD, which would hand back metadata for a commit nobody asked about. + if shas.is_empty() { + return Ok(HashMap::new()); + } + + let mut args = vec![ + "log", + "--no-walk=unsorted", + "--ignore-missing", + REASSOCIATION_LOG_FORMAT, + ]; + args.extend(shas.iter().map(String::as_str)); + args.push("--"); + let output = git(&args)?; + + Ok(output.lines().filter_map(parse_identity_line).collect()) +} + +/// Parse one [`REASSOCIATION_LOG_FORMAT`] line. The subject is the remainder, +/// so even a subject containing the separator byte survives intact. +fn parse_identity_line(line: &str) -> Option<(String, CommitIdentity)> { + let mut parts = line.splitn(4, '\x1f'); + let sha = parts.next()?; + let author_email = parts.next()?; + let author_timestamp = parts.next()?; + let subject = parts.next()?; + if sha.is_empty() { + return None; + } + Some(( + sha.to_string(), + CommitIdentity { + author_email: author_email.to_string(), + author_timestamp: author_timestamp.to_string(), + subject: subject.to_string(), + }, + )) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn identity(email: &str, timestamp: &str, subject: &str) -> CommitIdentity { + CommitIdentity { + author_email: email.to_string(), + author_timestamp: timestamp.to_string(), + subject: subject.to_string(), + } + } + + fn orphan(row_id: &str, old_sha: &str, identity: CommitIdentity) -> OrphanedRow { + OrphanedRow { + row_id: row_id.to_string(), + old_sha: old_sha.to_string(), + identity, + } + } + + fn rewritten(sha: &str, identity: CommitIdentity) -> RewrittenCommit { + RewrittenCommit { + sha: sha.to_string(), + identity, + claimed: false, + } + } + + #[test] + fn matches_rewritten_commits_by_author_identity() { + let parser = identity("a@example.com", "100", "feat: parser"); + let lexer = identity("b@example.com", "200", "fix: lexer"); + + let remaps = match_rewritten_commits( + &[ + orphan("row-1", "abc111", parser.clone()), + orphan("row-2", "abc222", lexer.clone()), + ], + &[rewritten("def444", parser), rewritten("def555", lexer)], + ); + + assert_eq!( + remaps, + vec![ + ShaRemap { + row_id: "row-1".into(), + old_sha: "abc111".into(), + new_sha: "def444".into() + }, + ShaRemap { + row_id: "row-2".into(), + old_sha: "abc222".into(), + new_sha: "def555".into() + }, + ] + ); + } + + /// A conflict-resolved commit has different content but the same author + /// metadata, so it still matches — that's the whole point of the key. + #[test] + fn matches_commit_whose_content_changed_during_conflict_resolution() { + let resolved = identity("a@example.com", "100", "feat: parser"); + let remaps = match_rewritten_commits( + &[orphan("row-1", "abc111", resolved.clone())], + &[rewritten("def444", resolved)], + ); + assert_eq!(remaps.len(), 1); + assert_eq!(remaps[0].new_sha, "def444"); + } + + /// Two commits authored in the same second with the same subject are + /// indistinguishable by key, so they pair up in branch order. + #[test] + fn matches_duplicate_identities_oldest_to_oldest() { + let wip = identity("a@example.com", "100", "wip"); + + let remaps = match_rewritten_commits( + &[ + orphan("row-old", "abc111", wip.clone()), + orphan("row-new", "abc222", wip.clone()), + ], + &[rewritten("def444", wip.clone()), rewritten("def555", wip)], + ); + + assert_eq!(remaps[0].row_id, "row-old"); + assert_eq!(remaps[0].new_sha, "def444"); + assert_eq!(remaps[1].row_id, "row-new"); + assert_eq!(remaps[1].new_sha, "def555"); + } + + /// A commit the rebase dropped (it became empty) has no counterpart; its + /// row stays orphaned rather than stealing a neighbour's SHA. + #[test] + fn leaves_dropped_commit_unmatched() { + let kept = identity("a@example.com", "100", "feat: parser"); + let dropped = identity("a@example.com", "200", "chore: already upstream"); + + let remaps = match_rewritten_commits( + &[ + orphan("row-kept", "abc111", kept.clone()), + orphan("row-dropped", "abc222", dropped), + ], + &[rewritten("def444", kept)], + ); + + assert_eq!(remaps.len(), 1); + assert_eq!(remaps[0].row_id, "row-kept"); + } + + /// A rewritten commit that already has a row of its own is off-limits — + /// e.g. the rebase session's own pending row once it has landed. + #[test] + fn skips_rewritten_commits_that_already_have_a_row() { + let parser = identity("a@example.com", "100", "feat: parser"); + let mut claimed = rewritten("def444", parser.clone()); + claimed.claimed = true; + + let remaps = match_rewritten_commits(&[orphan("row-1", "abc111", parser)], &[claimed]); + + assert!(remaps.is_empty()); + } + + /// A `|` is ordinary text in every field now that the separator is + /// `%x1f`; only a separator byte in the subject needs the remainder rule. + #[test] + fn parses_pipes_and_trailing_separators_intact() { + let (sha, identity) = + parse_identity_line("abc111\x1fa|b@example.com\x1f100\x1fchore: rename a\x1fb to c") + .unwrap(); + assert_eq!(sha, "abc111"); + assert_eq!(identity.author_email, "a|b@example.com"); + assert_eq!(identity.subject, "chore: rename a\x1fb to c"); + assert_eq!(identity.author_timestamp, "100"); + } + + #[test] + fn ignores_malformed_log_lines() { + assert!(parse_identity_line("").is_none()); + assert!(parse_identity_line("abc111\x1fa@example.com\x1f100").is_none()); + } + + /// Mid-rebase, `symbolic-ref` exits non-zero because HEAD is detached; a + /// checkout of some other branch answers with its name. Neither is the + /// branch we were asked to reassociate. + #[test] + fn head_is_on_branch_requires_an_attached_matching_head() { + let on_feature = |_: &[&str]| -> Result { Ok("feature\n".to_string()) }; + assert!(head_is_on_branch(&on_feature, "feature")); + assert!(!head_is_on_branch(&on_feature, "other")); + + let detached = |_: &[&str]| -> Result { + Err("fatal: ref HEAD is not a symbolic ref".to_string()) + }; + assert!(!head_is_on_branch(&detached, "feature")); + } +} diff --git a/apps/staged/src-tauri/src/git/mod.rs b/apps/staged/src-tauri/src/git/mod.rs index ab65d3be6..c8ea5d63c 100644 --- a/apps/staged/src-tauri/src/git/mod.rs +++ b/apps/staged/src-tauri/src/git/mod.rs @@ -40,11 +40,10 @@ pub use refs::{ origin_ref_for_branch, prune_remote, resolve_ref, BranchRef, }; pub use state::{ - complete_local_git_state, compute_branch_git_state, compute_branch_git_state_batched, - compute_fast_git_state_batched, compute_fast_local_git_state, compute_local_branch_git_state, - ensure_fast_forward_pullable, fast_forward_to_ref, local_git_state_cache_key, needs_fetch, - update_repo_fetch_cache, BaseGitState, BranchGitState, FastGitState, FetchGitState, FetchMode, - FetchStatus, UpstreamGitState, UpstreamRelation, WorktreeGitState, WorktreeStatusScope, + compute_branch_git_state, compute_branch_git_state_batched, compute_local_branch_git_state, + ensure_fast_forward_pullable, fast_forward_to_ref, update_repo_fetch_cache, BaseGitState, + BranchGitState, FetchGitState, FetchMode, FetchStatus, UpstreamGitState, UpstreamRelation, + WorktreeGitState, WorktreeStatusScope, }; pub use types::*; pub use worktree::{ @@ -52,8 +51,9 @@ pub use worktree::{ create_worktree_for_existing_branch_at_path, create_worktree_from_pr, create_worktree_from_pr_at_path, discard_worktree_changes, fetch_pr_head_sha, get_commits_since_base, get_full_commit_log, get_head_sha, get_parent_commit, - has_unpushed_commits, list_worktree_change_paths, list_worktrees, parse_worktree_status_paths, - project_worktree_path_for, project_worktree_root_for, remote_branch_exists, remove_worktree, - reset_to_commit, set_upstream_to_origin, switch_branch, update_branch_from_pr, - worktree_path_for, CommitInfo, UpdateFromPrResult, WorktreeChangePaths, + has_unpushed_commits, list_worktree_change_paths, list_worktrees, parse_branch_commit_line, + parse_worktree_status_paths, project_worktree_path_for, project_worktree_root_for, + remote_branch_exists, remove_worktree, reset_to_commit, set_upstream_to_origin, switch_branch, + update_branch_from_pr, worktree_path_for, BranchCommitFields, CommitInfo, UpdateFromPrResult, + WorktreeChangePaths, BRANCH_COMMIT_LOG_FORMAT, }; diff --git a/apps/staged/src-tauri/src/git/state.rs b/apps/staged/src-tauri/src/git/state.rs index 277398d2f..02ab00b78 100644 --- a/apps/staged/src-tauri/src/git/state.rs +++ b/apps/staged/src-tauri/src/git/state.rs @@ -128,58 +128,6 @@ pub enum FetchStatus { Failed, } -/// Fast (local-only) git state — no fetch, no ref comparisons. -/// Used for the fast stream of the two-stream timeline split. -#[derive(Debug, Clone, Serialize)] -#[serde(rename_all = "camelCase")] -pub struct FastGitState { - pub head_sha: Option, - pub current_branch: Option, - pub detached_head: bool, - pub expected_branch_matches: bool, - pub worktree: WorktreeGitState, -} - -impl FastGitState { - /// Convert to a full BranchGitState with placeholder upstream/base/fetch fields. - /// Used to build the partial timeline before the slow stream (fetch + refs) completes. - pub fn into_placeholder_git_state( - self, - branch_name: &str, - base_branch: &str, - ) -> BranchGitState { - let upstream_ref = origin_ref_for_branch(branch_name); - let base_ref = origin_ref_for_branch(base_branch); - BranchGitState { - head_sha: self.head_sha, - current_branch: self.current_branch, - detached_head: self.detached_head, - expected_branch_matches: self.expected_branch_matches, - worktree: self.worktree, - upstream: UpstreamGitState { - r#ref: upstream_ref, - exists: false, - sha: None, - relation: UpstreamRelation::Missing, - ahead: 0, - behind: 0, - merge_base_sha: None, - behind_base: 0, - }, - base: BaseGitState { - r#ref: base_ref, - sha: None, - commits_since_fork: 0, - }, - fetch: FetchGitState { - status: FetchStatus::Stale, - fetched_at: None, - error: None, - }, - } - } -} - #[derive(Debug, Clone)] struct FetchCacheEntry { fetched_at: i64, @@ -729,137 +677,6 @@ pub fn compute_local_branch_git_state( ) } -/// Check whether a fetch is needed for the given cache key and mode. -/// Used by timeline to decide whether to use the two-stream path. -/// -/// For local keys this consults the repo-level cache so the decision is -/// consistent with `refresh_refs_if_needed` — if another branch on the same -/// repo recently fetched, this returns `false`. -pub fn needs_fetch(cache_key: &str, fetch_mode: FetchMode) -> bool { - let now = now_ms(); - - // For local keys, check the repo-level cache first. - if let Some(repo_key) = repo_key_from_local_cache_key(cache_key) { - return match fetch_mode { - FetchMode::Never => false, - FetchMode::Force => true, - FetchMode::Ttl => { - let repo_fresh = repo_fetch_cache() - .lock() - .ok() - .and_then(|cache| cache.get(&repo_key).cloned()) - .map(|entry| now.saturating_sub(entry.fetched_at) <= FETCH_TTL_MS) - .unwrap_or(false); - // Even if the repo is fresh, we might still need a narrow - // fetch for uncovered refspecs — but that's fast enough that - // we don't need the two-stream split for it. - !repo_fresh - } - }; - } - - // Remote / non-local keys: fall back to per-branch cache. - let previous = fetch_cache() - .lock() - .ok() - .and_then(|cache| cache.get(cache_key).cloned()); - - match (fetch_mode, &previous) { - (FetchMode::Never, _) => false, - (FetchMode::Force, _) => true, - (FetchMode::Ttl, Some(entry)) => now.saturating_sub(entry.fetched_at) > FETCH_TTL_MS, - (FetchMode::Ttl, None) => true, - } -} - -/// Compute fast (local-only) git state for a local branch. -/// Returns HEAD, branch name, and worktree status without any fetch. -pub fn compute_fast_local_git_state( - repo: &Path, - branch_name: &str, - worktree_scope: WorktreeStatusScope, -) -> FastGitState { - let run_git = |args: &[&str]| -> Result { - cli::run(repo, args).map_err(|e| e.to_string()) - }; - let (head_sha, branch, worktree) = std::thread::scope(|s| { - let h = s.spawn(|| { - run_git(&["rev-parse", "HEAD"]) - .ok() - .and_then(trim_non_empty) - }); - let b = s.spawn(|| current_branch(&run_git)); - let w = s.spawn(|| compute_worktree_state(&run_git, worktree_scope)); - ( - h.join().expect("head thread panicked"), - b.join().expect("branch thread panicked"), - w.join().expect("worktree thread panicked"), - ) - }); - let expected = branch_name_without_origin(branch_name); - FastGitState { - detached_head: head_sha.is_some() && branch.is_none(), - expected_branch_matches: branch.as_deref().map(|c| c == expected).unwrap_or(false), - head_sha, - current_branch: branch, - worktree, - } -} - -/// Complete a local branch git state: runs fetch + ref comparisons, combining -/// with a pre-computed `FastGitState`. Used by the slow stream after the -/// partial timeline has been emitted. -pub fn complete_local_git_state( - repo: &Path, - fast: &FastGitState, - branch_name: &str, - base_branch: &str, - fetch_mode: FetchMode, -) -> BranchGitState { - let cache_key = format!("local:{}:{}:{}", repo.display(), branch_name, base_branch); - let run_git = |args: &[&str]| -> Result { - cli::run(repo, args).map_err(|e| e.to_string()) - }; - - let refresh = - refresh_refs_if_needed(&cache_key, &run_git, branch_name, base_branch, fetch_mode); - let upstream_ref = origin_ref_for_branch(branch_name); - let base_ref = origin_ref_for_branch(base_branch); - let base_ref_for_upstream = base_ref.clone(); - - let (upstream, base) = std::thread::scope(|s| { - let u = s.spawn(|| { - compute_upstream_state( - &run_git, - upstream_ref, - &base_ref_for_upstream, - fast.head_sha.as_deref(), - refresh.upstream_known_missing, - ) - }); - let b = s.spawn(|| compute_base_state(&run_git, base_ref, fast.head_sha.as_deref())); - ( - u.join().expect("upstream thread panicked"), - b.join().expect("base thread panicked"), - ) - }); - - BranchGitState { - head_sha: fast.head_sha.clone(), - current_branch: fast.current_branch.clone(), - detached_head: fast.detached_head, - expected_branch_matches: fast.expected_branch_matches, - worktree: fast.worktree.clone(), - fetch: refresh.fetch, - upstream, - base, - } -} - -pub fn local_git_state_cache_key(repo: &Path, branch_name: &str, base_branch: &str) -> String { - format!("local:{}:{}:{}", repo.display(), branch_name, base_branch) -} - // --------------------------------------------------------------------------- // Batched computation for remote projects // --------------------------------------------------------------------------- @@ -1107,150 +924,6 @@ fn parse_worktree_from_status(status_output: &str) -> WorktreeGitState { state } -// --------------------------------------------------------------------------- -// Fast script for remote two-stream split -// --------------------------------------------------------------------------- -// -// When a fetch is needed, the timeline uses two concurrent round-trips: -// 1. BATCH_FAST_SCRIPT — local state + commits (no fetch, returns immediately) -// 2. BATCH_GIT_STATE_SCRIPT — fetch + full ref comparisons (blocks on fetch) -// -// The fast script's output is used to emit a partial timeline event so -// commits + worktree rows appear before the slow stream completes. - -/// Fast local-only script for remote projects. -/// -/// Arguments: -/// $1 = repo_path -/// $2 = base_ref (e.g., "origin/main") — used for merge-base + git log -/// $3 = "uno" (no untracked enumeration) or "uall" (full enumeration) -const BATCH_FAST_SCRIPT: &str = concat!( - "cd \"$1\" || exit 1\n", - "head_sha=$(git rev-parse HEAD 2>/dev/null || true)\n", - "printf 'HEAD=%s\\n' \"$head_sha\"\n", - "printf 'BRANCH=%s\\n' \"$(git branch --show-current 2>/dev/null || true)\"\n", - "if [ \"$3\" = 'uno' ]; then ut_flag='--untracked-files=no'; else ut_flag='--untracked-files=all'; fi\n", - "echo STATUS_START\n", - "git status --porcelain=1 \"$ut_flag\" 2>/dev/null || true\n", - "echo STATUS_END\n", - // Commits using locally-cached refs - "mb=$(git merge-base \"$2\" HEAD 2>/dev/null || true)\n", - "if [ -n \"$mb\" ]; then\n", - " range=\"${mb}..HEAD\"\n", - "else\n", - " range=\"$2..HEAD\"\n", - "fi\n", - "echo COMMITS_START\n", - "git log --format='%H|%h|%s|%an|%ae|%ct' \"$range\" 2>/dev/null || true\n", - "echo COMMITS_END\n", - "exit 0\n", -); - -/// Parsed output from the fast local-only script. -pub struct BatchFastOutput { - pub head_sha: Option, - pub branch: Option, - pub status_lines: String, - pub commit_lines: Vec, -} - -pub fn parse_batch_fast_output(raw: &str) -> BatchFastOutput { - let mut head_sha = None; - let mut branch = None; - let mut status_lines = String::new(); - let mut commit_lines = Vec::new(); - let mut in_status = false; - let mut in_commits = false; - - for line in raw.lines() { - if line == "STATUS_START" { - in_status = true; - continue; - } - if line == "STATUS_END" { - in_status = false; - continue; - } - if line == "COMMITS_START" { - in_commits = true; - continue; - } - if line == "COMMITS_END" { - in_commits = false; - continue; - } - if in_status { - if !status_lines.is_empty() { - status_lines.push('\n'); - } - status_lines.push_str(line); - continue; - } - if in_commits { - if !line.is_empty() { - commit_lines.push(line.to_string()); - } - continue; - } - if let Some(val) = line.strip_prefix("HEAD=") { - let v = val.trim(); - if !v.is_empty() { - head_sha = Some(v.to_string()); - } - } else if let Some(val) = line.strip_prefix("BRANCH=") { - let v = val.trim(); - if !v.is_empty() { - branch = Some(v.to_string()); - } - } - } - - BatchFastOutput { - head_sha, - branch, - status_lines, - commit_lines, - } -} - -impl BatchFastOutput { - /// Convert to FastGitState. - pub fn into_fast_git_state(self, branch_name: &str) -> (FastGitState, Vec) { - let worktree = parse_worktree_from_status(&self.status_lines); - let expected = branch_name_without_origin(branch_name); - let fast = FastGitState { - detached_head: self.head_sha.is_some() && self.branch.is_none(), - expected_branch_matches: self - .branch - .as_deref() - .map(|c| c == expected) - .unwrap_or(false), - head_sha: self.head_sha, - current_branch: self.branch, - worktree, - }; - (fast, self.commit_lines) - } -} - -/// Run the fast local-only script on a remote workspace and return parsed output. -pub fn compute_fast_git_state_batched( - run_script: &F, - repo_path: &str, - base_branch: &str, - worktree_scope: WorktreeStatusScope, -) -> Result -where - F: Fn(&str, &[&str]) -> Result, -{ - let base_ref = origin_ref_for_branch(base_branch); - let raw = run_script( - BATCH_FAST_SCRIPT, - &[repo_path, &base_ref, worktree_scope.script_arg()], - )?; - Ok(parse_batch_fast_output(&raw)) -} - /// Compute branch git state using a single batched shell script. /// /// This is the remote-optimised counterpart of `compute_branch_git_state`. diff --git a/apps/staged/src-tauri/src/git/worktree.rs b/apps/staged/src-tauri/src/git/worktree.rs index 305ee6e64..a7cf975ad 100644 --- a/apps/staged/src-tauri/src/git/worktree.rs +++ b/apps/staged/src-tauri/src/git/worktree.rs @@ -299,6 +299,59 @@ pub fn get_head_sha(worktree: &Path) -> Result { Ok(output.trim().to_string()) } +/// `git log` format for every commit producer that feeds a +/// [`CommitTimelineItem`](crate::CommitTimelineItem). +/// +/// Carries both clocks: `%ct` (committer time) is what a rebase rewrites, so +/// it answers "did the branch change since?"; `%at` (author time) is what a +/// rebase preserves, so it answers "when was this commit written" and is what +/// the branch timeline sorts on. +/// +/// Fields are separated by `%x1f` (the unit separator) because no printable +/// delimiter is safe: git permits `|` in author names — and technically in +/// emails — and a delimiter inside a field shifts every field after it. The +/// subject still goes last so [`parse_branch_commit_line`] can take it as the +/// remainder — the same shape as `commit_reassociation`'s format. +pub const BRANCH_COMMIT_LOG_FORMAT: &str = "--format=%H%x1f%h%x1f%an%x1f%ae%x1f%ct%x1f%at%x1f%s"; + +/// One [`BRANCH_COMMIT_LOG_FORMAT`] line, borrowed from the log output. +#[derive(Debug, Clone)] +pub struct BranchCommitFields<'a> { + pub sha: &'a str, + pub short_sha: &'a str, + pub author: &'a str, + pub author_email: &'a str, + /// Committer time (`%ct`), in unix seconds. Rewritten by a rebase. + pub committer_timestamp: i64, + /// Author time (`%at`), in unix seconds. Preserved by a rebase. + pub author_timestamp: i64, + pub subject: &'a str, +} + +/// Parse one [`BRANCH_COMMIT_LOG_FORMAT`] line. The subject is the remainder, +/// so even a subject containing the separator byte survives intact. Returns +/// `None` for a line that doesn't carry every field — a blank line, or output +/// from some other format. +pub fn parse_branch_commit_line(line: &str) -> Option> { + let mut parts = line.splitn(7, '\x1f'); + let sha = parts.next().filter(|sha| !sha.is_empty())?; + let short_sha = parts.next()?; + let author = parts.next()?; + let author_email = parts.next()?; + let committer_timestamp = parts.next()?; + let author_timestamp = parts.next()?; + let subject = parts.next()?; + Some(BranchCommitFields { + sha, + short_sha, + author, + author_email, + committer_timestamp: committer_timestamp.parse().unwrap_or(0), + author_timestamp: author_timestamp.parse().unwrap_or(0), + subject, + }) +} + /// Get commits on a branch since it diverged from base. /// Returns commits in reverse chronological order (newest first). #[derive(Debug, Clone)] @@ -308,7 +361,10 @@ pub struct CommitInfo { pub subject: String, pub author: String, pub author_email: String, + /// Committer time (`%ct`), in unix seconds. Rewritten by a rebase. pub timestamp: i64, + /// Author time (`%at`), in unix seconds. Preserved by a rebase. + pub author_timestamp: i64, /// Position in git's topological order (0 = oldest on the branch). /// Used as a tiebreaker when multiple commits share the same second-level timestamp. pub order: i64, @@ -329,29 +385,30 @@ pub fn get_commits_since_base(worktree: &Path, base: &str) -> Result = line.splitn(6, '|').collect(); - if parts.len() >= 6 { - commits.push(CommitInfo { - sha: parts[0].to_string(), - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), - order: 0, // placeholder, assigned below - }); - } - } + let output = cli::run_smart(worktree, &["log", BRANCH_COMMIT_LOG_FORMAT, &range])?; + + Ok(parse_commit_info_lines(&output)) +} + +/// Parse [`BRANCH_COMMIT_LOG_FORMAT`] lines, newest first as `git log` emits +/// them, assigning `order` so that 0 is the oldest commit on the branch. +fn parse_commit_info_lines(output: &str) -> Vec { + let mut commits: Vec = output + .lines() + .filter_map(parse_branch_commit_line) + .map(|fields| CommitInfo { + sha: fields.sha.to_string(), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + timestamp: fields.committer_timestamp, + author_timestamp: fields.author_timestamp, + order: 0, // placeholder, assigned below + }) + .collect(); // git log returns newest-first; assign order so that 0 = oldest. let len = commits.len() as i64; @@ -359,7 +416,7 @@ pub fn get_commits_since_base(worktree: &Path, base: &str) -> Result Result Result = output .lines() - .filter(|l| !l.is_empty()) .enumerate() .filter_map(|(i, line)| { - let parts: Vec<&str> = line.splitn(6, '|').collect(); - if parts.len() >= 6 { - Some(CommitTimelineItem { - id: None, - sha: parts[0].to_string(), - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), - order: (max_count - 1 - i) as i64, - session_id: None, - session_status: None, - completion_reason: None, - is_own_commit: false, - }) - } else { - None - } + let fields = git::parse_branch_commit_line(line)?; + Some(CommitTimelineItem { + id: None, + sha: fields.sha.to_string(), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + // Committer time: this listing never interleaves with notes, + // so it has no reason to prefer the rebase-stable clock. + timestamp: fields.committer_timestamp, + sort_timestamp: fields.committer_timestamp, + order: (max_count - 1 - i) as i64, + session_id: None, + session_status: None, + completion_reason: None, + is_own_commit: false, + }) }) .collect(); diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 7a38670f9..0d5006c7c 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -3547,7 +3547,9 @@ fn latest_git_commit_ms(store: &Arc, branch_id: &str) -> i64 { Ok(c) => c, Err(_) => return 0, }; - // CommitInfo.timestamp is in seconds; convert to milliseconds. + // `timestamp` is committer time, not the author time the timeline sorts + // on: a rebase *should* read as new activity here. In seconds, so convert + // to milliseconds. commits.iter().map(|c| c.timestamp).max().unwrap_or(0) * 1000 } @@ -3719,7 +3721,10 @@ fn build_remote_branch_context( "git", "log", "--reverse", - "--format=%x00%ct%x01commit %H%nAuthor: %an%nDate: %ci%n%n%B", + // %at (author time) rather than %ct: the prefix only orders the + // interleave, and author time survives a rebase — see + // `git::get_full_commit_log`, the local counterpart. + "--format=%x00%at%x01commit %H%nAuthor: %an%nDate: %ci%n%n%B", &range, ], ) { @@ -4147,8 +4152,13 @@ fn render_timeline(mut timeline: Vec, error: Option) -> S /// Parse a timestamped git log into timeline entries. /// -/// Expects the format produced by `--format=%x00%ct%x01commit %H…`: +/// Expects the format produced by `--format=%x00%at%x01commit %H…`: /// `\0\x01` per commit. +/// +/// The prefix is author time, which a rebase preserves — but author dates +/// aren't monotonic along a branch (a cherry-pick keeps its old one), so the +/// entries are clamped the same way the branch card's are, keeping +/// "Branch History (oldest first)" in `git log` order. fn parse_timestamped_log(output: &str) -> Vec { let mut entries = Vec::new(); // The log is produced with --reverse (oldest-first), so index 0 = oldest. @@ -4169,6 +4179,7 @@ fn parse_timestamped_log(output: &str) -> Vec { } } } + crate::timeline::clamp_timestamps_monotonic(entries.iter_mut().map(|e| &mut e.timestamp)); entries } @@ -6758,6 +6769,29 @@ mod tests { assert!(entries[0].content.contains("user kept this review alive")); } + /// The log's prefix is author time, so a cherry-picked commit can carry a + /// date older than the commit it follows. "Branch History (oldest first)" + /// sorts on that prefix, so the entries have to be clamped into branch + /// order the same way the branch card's commits are. + #[test] + fn parse_timestamped_log_clamps_out_of_order_author_dates() { + // --reverse output: oldest first, with a cherry-pick in the middle. + let log = "\u{0}200\u{1}commit aaa\nfirst\ + \u{0}100\u{1}commit bbb\ncherry-picked\ + \u{0}300\u{1}commit ccc\nthird"; + + let entries = parse_timestamped_log(log); + + assert_eq!( + entries.iter().map(|e| e.timestamp).collect::>(), + vec![200, 200, 300] + ); + assert_eq!( + entries.iter().map(|e| e.order).collect::>(), + vec![0, 1, 2] + ); + } + #[test] fn parse_commit_shas_extracts_full_shas() { let log = "\u{0}1700000000\u{1}commit abc123def456\nAuthor: A\nDate: d\n\nfirst\ diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 264705d57..83d9528c2 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -35,7 +35,7 @@ use std::collections::HashMap; use std::io; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::{Command, Output, Stdio}; use std::sync::{Arc, OnceLock}; use std::time::Duration; @@ -1430,6 +1430,44 @@ fn current_pipeline_head(config: &PipelineConfig) -> Result { } } +/// Whether a session's persisted pipeline is a rebase. Read from the session +/// row rather than an in-memory config so it still answers correctly for an AI +/// handoff that outlived the pipeline that started it. +fn session_is_rebase_pipeline(store: &Store, session_id: &str) -> bool { + store + .get_session(session_id) + .ok() + .flatten() + .and_then(|session| session.pipeline) + .and_then(|pipeline| pipeline.kind) + .is_some_and(|kind| kind == PipelineKind::Rebase) +} + +/// Reattach a branch's commit metadata (and reviews) to the SHAs a rebase +/// rewrote them into. Best-effort: a failure here only leaves the rows +/// orphaned, which is what happened before reassociation existed. +fn reassociate_rebased_commits( + store: &Store, + branch_id: &str, + working_dir: &Path, + workspace_name: Option<&str>, +) { + match crate::commit_reassociation::reassociate_after_rebase( + store, + branch_id, + working_dir, + workspace_name, + ) { + Ok(0) => {} + Ok(count) => { + log::info!("Reassociated {count} rebased commit(s) on branch {branch_id}") + } + Err(e) => { + log::warn!("Failed to reassociate rebased commits on branch {branch_id}: {e}") + } + } +} + fn resolve_pipeline_artifacts_without_ai(config: &PipelineConfig, store: &Store, completed: bool) { match config.pipeline.kind.as_ref() { Some(PipelineKind::Rebase) if completed => { @@ -1486,6 +1524,20 @@ fn finalize_rebase_pipeline_without_ai(config: &PipelineConfig, store: &Store) { return; } + // The rebase rewrote every SHA on the branch, orphaning the metadata rows + // that pointed at the old ones. Reattach them *before* claiming the new + // HEAD: once the pre-existing head row owns it, `complete_pending_commit_sha` + // takes its "target SHA already owned" branch and drops this pipeline's + // pending row — so the top commit keeps its authoring session instead of + // the mechanical "Rebase branch" one. A head commit authored outside + // Staged has no prior row, so the rebase session keeps it, as before. + reassociate_rebased_commits( + store, + &commit.branch_id, + &config.working_dir, + config.workspace_name.as_deref(), + ); + match store.complete_pending_commit_sha(&commit.id, &commit.branch_id, ¤t_head) { Ok(true) => log::info!( "Rebase pipeline session {} updated pending commit to {}", @@ -2319,65 +2371,110 @@ fn run_post_completion_hooks( match current_head_result { Ok(current_head) if current_head != pre_sha => { - log::info!( - "Session {session_id}: new commit detected ({} → {})", - &pre_sha[..7.min(pre_sha.len())], - ¤t_head[..7.min(current_head.len())] - ); - let recorded = if commit.sha.is_none() { - match store.complete_pending_commit_sha( - &commit.id, + // A rebase pipeline that handed off to AI (conflicts, a + // failed fetch) lands here instead of + // `finalize_rebase_pipeline_without_ai`. If its turn ended + // with the rebase still stopped on a conflict, HEAD is + // detached on a partially applied commit — a SHA `git + // rebase --abort` erases — so nothing may claim it: not + // the pending row, not an amend, and no auto-review via + // `committed_branch_id`. Skip the whole arm; the rows + // self-resolve on a later turn, because resumed sessions + // re-capture `pre_head_sha` and land back here once HEAD + // is attached again (after `--continue` finishes or + // `--abort` restores, reassociation plus the duplicate-SHA + // branch of `complete_pending_commit_sha` settle every + // row), while a turn that never comes leaves the pending + // row `sha IS NULL` — an ordinary failed commit attempt. + let rebase_pipeline = session_is_rebase_pipeline(store, session_id); + if rebase_pipeline + && !crate::commit_reassociation::head_is_attached_to_branch( + store, &commit.branch_id, - ¤t_head, - ) { - Ok(recorded) => recorded, - Err(e) => { - log::error!("Failed to update pending commit SHA: {e}"); - false - } - } + working_dir, + workspace_name, + ) + { + log::info!( + "Session {session_id}: rebase still in flight (HEAD detached), \ + leaving commit detection for a later turn" + ); } else { - match store.get_commit_by_sha(&commit.branch_id, ¤t_head) { - Ok(Some(existing)) if existing.id != commit.id => { - log::warn!( - "Session {session_id}: target commit SHA already has metadata row {}, skipping update", - existing.id - ); - false - } - Ok(_) => { - if let Err(e) = store.update_commit_sha(&commit.id, ¤t_head) { - log::error!("Failed to update commit SHA: {e}"); + log::info!( + "Session {session_id}: new commit detected ({} → {})", + &pre_sha[..7.min(pre_sha.len())], + ¤t_head[..7.min(current_head.len())] + ); + // The rebase rewrote SHAs just the same as the no-AI + // path. Reattach the orphaned rows before the pending + // row below claims the new HEAD — see + // `finalize_rebase_pipeline_without_ai` for why the + // ordering matters. + if rebase_pipeline { + reassociate_rebased_commits( + store, + &commit.branch_id, + working_dir, + workspace_name, + ); + } + let recorded = if commit.sha.is_none() { + match store.complete_pending_commit_sha( + &commit.id, + &commit.branch_id, + ¤t_head, + ) { + Ok(recorded) => recorded, + Err(e) => { + log::error!("Failed to update pending commit SHA: {e}"); false - } else { - true } } - Err(e) => { - log::error!("Failed to check existing commit SHA: {e}"); - false + } else { + match store.get_commit_by_sha(&commit.branch_id, ¤t_head) { + Ok(Some(existing)) if existing.id != commit.id => { + log::warn!( + "Session {session_id}: target commit SHA already has metadata row {}, skipping update", + existing.id + ); + false + } + Ok(_) => { + if let Err(e) = + store.update_commit_sha(&commit.id, ¤t_head) + { + log::error!("Failed to update commit SHA: {e}"); + false + } else { + true + } + } + Err(e) => { + log::error!("Failed to check existing commit SHA: {e}"); + false + } } - } - }; + }; - if recorded { - committed_branch_id = Some(commit.branch_id.clone()); - - // Spawn background diff caching for remote branches. - if let Some(ws_name) = workspace_name { - let commit_shas: Vec = store - .list_commits_for_branch(&commit.branch_id) - .unwrap_or_default() - .into_iter() - .filter_map(|c| c.sha) - .collect(); - crate::diff_cache::spawn_cache_branch_diff( - Arc::clone(store), - commit.branch_id.clone(), - ws_name.to_string(), - current_head.clone(), - commit_shas, - ); + if recorded { + committed_branch_id = Some(commit.branch_id.clone()); + + // Spawn background diff caching for remote branches. + if let Some(ws_name) = workspace_name { + let commit_shas: Vec = store + .list_commits_for_branch(&commit.branch_id) + .unwrap_or_default() + .into_iter() + .filter_map(|c| c.sha) + .collect(); + crate::diff_cache::spawn_cache_branch_diff( + Arc::clone(store), + commit.branch_id.clone(), + ws_name.to_string(), + current_head.clone(), + commit_shas, + ); + } } } } @@ -3655,6 +3752,385 @@ mod tests { let _ = std::fs::remove_dir_all(repo); } + /// A branch whose two commits were authored by Staged sessions (with a + /// review on the head one) and have just been rebased onto a moved base by + /// a rebase-pipeline session, leaving every row orphaned. + struct RebasedBranch { + repo: crate::test_utils::TempGitRepo, + store: Arc, + /// `(session_id, commit_row_id)` for the authoring sessions, oldest first. + authored: Vec<(String, String)>, + pending_id: String, + review_id: String, + rebase_session_id: String, + old_head: String, + new_head: String, + new_first: String, + } + + fn rebased_branch() -> RebasedBranch { + use crate::store::{Commit, Review, ReviewScope, Session}; + + let repo = crate::test_utils::TempGitRepo::new(); + repo.write_file("base.txt", "base\n"); + let base_sha = repo.commit("chore: base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &base_sha]); + + repo.run_git(&["checkout", "-b", "feature"]); + repo.write_file("parser.txt", "parser\n"); + let old_first = repo.commit("feat: parser"); + repo.write_file("lexer.txt", "lexer\n"); + let old_head = repo.commit("fix: lexer"); + + let store = Arc::new(Store::in_memory().unwrap()); + let project = crate::store::Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = crate::store::Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + + // The two sessions that authored the branch, oldest first. + let mut authored = Vec::new(); + for (index, (prompt, sha)) in [("Add parser", &old_first), ("Fix lexer", &old_head)] + .into_iter() + .enumerate() + { + let session = Session::new_running(prompt, repo.path()); + store.create_session(&session).unwrap(); + let mut row = Commit::new_with_sha(&branch.id, sha).with_session(&session.id); + row.created_at = 1_000 + index as i64; + row.updated_at = row.created_at; + store.create_commit(&row).unwrap(); + authored.push((session.id, row.id)); + } + let review = Review::new(&branch.id, &old_head, ReviewScope::Commit); + store.create_review(&review).unwrap(); + + let mut rebase_session = Session::new_running("Rebase branch", repo.path()); + rebase_session.pipeline = + Some(PipelineExecution::from_steps(&[]).with_kind(PipelineKind::Rebase)); + store.create_session(&rebase_session).unwrap(); + let mut pending = Commit::new_pending(&branch.id).with_session(&rebase_session.id); + pending.created_at = 2_000; + pending.updated_at = pending.created_at; + store.create_commit(&pending).unwrap(); + + // Move the base out from under the branch, then really rebase onto it. + repo.run_git(&["checkout", "main"]); + repo.write_file("moved.txt", "moved\n"); + let moved_base = repo.commit("chore: move base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &moved_base]); + repo.run_git(&["checkout", "feature"]); + repo.run_git(&["rebase", "--signoff", "origin/main"]); + let new_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + let new_first = repo.run_git(&["rev-parse", "HEAD~1"]).trim().to_string(); + assert_ne!(new_head, old_head, "the rebase must rewrite the SHAs"); + + RebasedBranch { + repo, + store, + authored, + pending_id: pending.id, + review_id: review.id, + rebase_session_id: rebase_session.id, + old_head, + new_head, + new_first, + } + } + + fn assert_reassociated(fixture: &RebasedBranch) { + let store = &fixture.store; + + let first = store.get_commit(&fixture.authored[0].1).unwrap().unwrap(); + assert_eq!(first.sha.as_deref(), Some(fixture.new_first.as_str())); + let head = store.get_commit(&fixture.authored[1].1).unwrap().unwrap(); + assert_eq!(head.sha.as_deref(), Some(fixture.new_head.as_str())); + assert_eq!( + head.session_id.as_deref(), + Some(fixture.authored[1].0.as_str()), + "the head commit must keep its authoring session, not the rebase one" + ); + + // The rebase session's pending row loses the race for the new HEAD and + // is dropped, exactly as it is for a no-op rebase today. + assert!(store.get_commit(&fixture.pending_id).unwrap().is_none()); + + // The review followed its commit, so it stays visible in the timeline. + let review = store.get_review(&fixture.review_id).unwrap().unwrap(); + assert_eq!(review.commit_sha, fixture.new_head); + } + + /// A rebase rewrites every SHA on the branch. Finalizing the pipeline must + /// move the pre-existing commit rows (and their reviews) onto the new SHAs. + #[test] + fn rebase_pipeline_completion_reassociates_pre_existing_commits() { + let fixture = rebased_branch(); + + finalize_rebase_pipeline_without_ai( + &rebase_pipeline_config( + &fixture.rebase_session_id, + fixture.repo.path(), + &fixture.old_head, + ), + &fixture.store, + ); + + assert_reassociated(&fixture); + } + + /// When the pipeline hands off to AI (conflicts, a failed fetch), the agent + /// finishes the rebase and the post-completion hooks run instead — they + /// have to reassociate too. + #[test] + fn rebase_handoff_post_completion_reassociates_pre_existing_commits() { + let fixture = rebased_branch(); + + run_post_completion_hooks( + &fixture.rebase_session_id, + fixture.repo.path(), + Some(&fixture.old_head), + None, + &fixture.store, + ); + + assert_reassociated(&fixture); + } + + /// A branch like [`RebasedBranch`], except the rebase is still in flight: + /// the first commit rebased cleanly, the second stopped on a conflict, + /// leaving HEAD detached on the partially applied rewrite. + struct ConflictedRebase { + repo: crate::test_utils::TempGitRepo, + store: Arc, + /// `(session_id, commit_row_id)` for the authoring sessions, oldest first. + authored: Vec<(String, String)>, + pending_id: String, + review_id: String, + rebase_session_id: String, + old_first: String, + old_head: String, + } + + fn conflicted_rebase() -> ConflictedRebase { + use crate::store::{Commit, Review, ReviewScope, Session}; + + let repo = crate::test_utils::TempGitRepo::new(); + repo.write_file("shared.txt", "base\n"); + let base_sha = repo.commit("chore: base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &base_sha]); + + // Two commits: the first rebases cleanly, the second conflicts — so the + // stopped rebase leaves a rewritten commit on a detached HEAD. + repo.run_git(&["checkout", "-b", "feature"]); + repo.write_file("parser.txt", "parser\n"); + let old_first = repo.commit("feat: parser"); + repo.write_file("shared.txt", "feature\n"); + let old_head = repo.commit("fix: lexer"); + + let store = Arc::new(Store::in_memory().unwrap()); + let project = crate::store::Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = crate::store::Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + + let mut authored = Vec::new(); + for (index, sha) in [&old_first, &old_head].into_iter().enumerate() { + let session = Session::new_running("Author a commit", repo.path()); + store.create_session(&session).unwrap(); + let mut row = Commit::new_with_sha(&branch.id, sha).with_session(&session.id); + row.created_at = 1_000 + index as i64; + row.updated_at = row.created_at; + store.create_commit(&row).unwrap(); + authored.push((session.id, row.id)); + } + let review = Review::new(&branch.id, &old_head, ReviewScope::Commit); + store.create_review(&review).unwrap(); + + let mut rebase_session = Session::new_running("Rebase branch", repo.path()); + rebase_session.pipeline = + Some(PipelineExecution::from_steps(&[]).with_kind(PipelineKind::Rebase)); + store.create_session(&rebase_session).unwrap(); + let pending = Commit::new_pending(&branch.id).with_session(&rebase_session.id); + store.create_commit(&pending).unwrap(); + + // Move the base with a conflicting change, then rebase into the conflict. + repo.run_git(&["checkout", "main"]); + repo.write_file("shared.txt", "moved\n"); + let moved_base = repo.commit("chore: move base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &moved_base]); + repo.run_git(&["checkout", "feature"]); + assert!( + repo.try_run_git(&["rebase", "origin/main"]).is_err(), + "the rebase must stop on the conflict" + ); + assert_ne!( + repo.run_git(&["rev-parse", "HEAD"]).trim(), + old_head, + "the stopped rebase must have moved HEAD off the branch" + ); + + ConflictedRebase { + repo, + store, + authored, + pending_id: pending.id, + review_id: review.id, + rebase_session_id: rebase_session.id, + old_first, + old_head, + } + } + + /// End the handoff turn with the rebase still stopped on the conflict. + /// Returns the hooks' `committed_branch_id`, which must be `None` — a + /// mid-rebase state must not trigger the auto-review follow-up. + fn end_turn_mid_rebase(fixture: &ConflictedRebase) -> Option { + run_post_completion_hooks( + &fixture.rebase_session_id, + fixture.repo.path(), + Some(&fixture.old_head), + None, + &fixture.store, + ) + } + + fn assert_untouched(fixture: &ConflictedRebase) { + let store = &fixture.store; + assert_eq!( + store + .get_commit(&fixture.authored[0].1) + .unwrap() + .unwrap() + .sha + .as_deref(), + Some(fixture.old_first.as_str()), + "the first commit's row must keep the SHA an abort would restore" + ); + assert_eq!( + store + .get_commit(&fixture.authored[1].1) + .unwrap() + .unwrap() + .sha + .as_deref(), + Some(fixture.old_head.as_str()) + ); + assert_eq!( + store + .get_review(&fixture.review_id) + .unwrap() + .unwrap() + .commit_sha, + fixture.old_head + ); + } + + /// The handoff also runs when the agent's turn ends with the rebase still + /// stopped on a conflict. HEAD is detached on a partially applied rewrite + /// there, and those SHAs only survive until someone runs `git rebase + /// --abort` — which restores the originals — so neither the authored rows + /// nor the rebase session's pending row may take one. The rows have to + /// stay put until the rebase finishes. + #[test] + fn rebase_stopped_on_a_conflict_leaves_the_rows_alone() { + let fixture = conflicted_rebase(); + + assert!(end_turn_mid_rebase(&fixture).is_none()); + + assert_untouched(&fixture); + let pending = fixture + .store + .get_commit(&fixture.pending_id) + .unwrap() + .unwrap(); + assert!( + pending.sha.is_none(), + "the pending row must not claim the detached mid-rebase SHA" + ); + } + + /// The deferred pending row resolves on the next turn: the resumed session + /// re-captures HEAD (now the detached mid-rebase commit), the conflict is + /// resolved, and `--continue` finishes the rebase. The authored rows claim + /// the rewritten SHAs first, so the pending row drops as a duplicate — + /// the same end state as a rebase that never conflicted. + #[test] + fn rebase_resumed_and_finished_resolves_the_deferred_pending_row() { + let fixture = conflicted_rebase(); + assert!(end_turn_mid_rebase(&fixture).is_none()); + + let repo = &fixture.repo; + let detached_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + repo.write_file("shared.txt", "resolved\n"); + repo.run_git(&["add", "shared.txt"]); + repo.run_git(&["-c", "core.editor=true", "rebase", "--continue"]); + let new_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + let new_first = repo.run_git(&["rev-parse", "HEAD~1"]).trim().to_string(); + + run_post_completion_hooks( + &fixture.rebase_session_id, + repo.path(), + Some(&detached_head), + None, + &fixture.store, + ); + + let store = &fixture.store; + let first = store.get_commit(&fixture.authored[0].1).unwrap().unwrap(); + assert_eq!(first.sha.as_deref(), Some(new_first.as_str())); + let head = store.get_commit(&fixture.authored[1].1).unwrap().unwrap(); + assert_eq!(head.sha.as_deref(), Some(new_head.as_str())); + assert_eq!( + head.session_id.as_deref(), + Some(fixture.authored[1].0.as_str()), + "the head commit must keep its authoring session, not the rebase one" + ); + assert_eq!( + store + .get_review(&fixture.review_id) + .unwrap() + .unwrap() + .commit_sha, + new_head + ); + assert!( + store.get_commit(&fixture.pending_id).unwrap().is_none(), + "the deferred pending row must drop as a duplicate of the reclaimed head" + ); + } + + /// The other way out of the conflict: `--abort` restores the original + /// SHAs. The next turn sees HEAD attached again, reassociation finds no + /// orphans, and the pending row's claim on the old head hits the same + /// duplicate-resolution branch — dropped cleanly, rows untouched. + #[test] + fn rebase_resumed_and_aborted_drops_the_deferred_pending_row() { + let fixture = conflicted_rebase(); + assert!(end_turn_mid_rebase(&fixture).is_none()); + + let repo = &fixture.repo; + let detached_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + repo.run_git(&["rebase", "--abort"]); + + run_post_completion_hooks( + &fixture.rebase_session_id, + repo.path(), + Some(&detached_head), + None, + &fixture.store, + ); + + assert_untouched(&fixture); + assert!( + fixture + .store + .get_commit(&fixture.pending_id) + .unwrap() + .is_none(), + "the deferred pending row must drop as a duplicate of the restored head" + ); + } + // ── find_closing_fence ────────────────────────────────────────────── #[test] diff --git a/apps/staged/src-tauri/src/store/commits.rs b/apps/staged/src-tauri/src/store/commits.rs index adc592b57..117376c06 100644 --- a/apps/staged/src-tauri/src/store/commits.rs +++ b/apps/staged/src-tauri/src/store/commits.rs @@ -137,6 +137,58 @@ impl Store { Ok(rows > 0) } + /// Repoint commit rows at the SHAs a history rewrite (e.g. a rebase) gave + /// them, carrying each row's reviews along so they don't drop out of the + /// timeline with their old commit. + /// + /// `remaps` is a list of `(row_id, old_sha, new_sha)`, applied in one + /// transaction. Each pair re-checks the `(branch_id, sha)` unique index + /// inside the transaction and is skipped on collision, so a row another + /// writer already attached to `new_sha` keeps it. Returns how many rows + /// were repointed. + pub fn remap_commit_shas( + &self, + branch_id: &str, + remaps: &[(&str, &str, &str)], + ) -> Result { + let mut conn = self.conn.lock().unwrap(); + let tx = conn.transaction()?; + let now = now_timestamp(); + let mut remapped = 0; + + for (row_id, old_sha, new_sha) in remaps { + let owner = tx + .query_row( + "SELECT id FROM commits WHERE branch_id = ?1 AND sha = ?2", + params![branch_id, new_sha], + |row| row.get::<_, String>(0), + ) + .optional()?; + if owner.is_some_and(|owner| owner != *row_id) { + continue; + } + + let rows = tx.execute( + "UPDATE commits SET sha = ?1, updated_at = ?2 + WHERE id = ?3 AND branch_id = ?4 AND sha = ?5", + params![new_sha, now, row_id, branch_id, old_sha], + )?; + if rows == 0 { + continue; + } + + tx.execute( + "UPDATE reviews SET commit_sha = ?1, updated_at = ?2 + WHERE branch_id = ?3 AND commit_sha = ?4", + params![new_sha, now, branch_id, old_sha], + )?; + remapped += 1; + } + + tx.commit()?; + Ok(remapped) + } + /// Delete a linked pending commit row if it has not landed. pub fn delete_pending_commit_for_session(&self, session_id: &str) -> Result { let conn = self.conn.lock().unwrap(); diff --git a/apps/staged/src-tauri/src/store/tests.rs b/apps/staged/src-tauri/src/store/tests.rs index 0a973ccfe..4a301f818 100644 --- a/apps/staged/src-tauri/src/store/tests.rs +++ b/apps/staged/src-tauri/src/store/tests.rs @@ -1694,6 +1694,75 @@ fn test_complete_pending_commit_sha_updates_pending_row() { assert_eq!(commit.sha.as_deref(), Some("bbb222")); } +#[test] +fn test_remap_commit_shas_moves_rows_and_their_reviews() { + let store = Store::in_memory().unwrap(); + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + + let first = Commit::new_with_sha(&branch.id, "old111"); + let second = Commit::new_with_sha(&branch.id, "old222"); + store.create_commit(&first).unwrap(); + store.create_commit(&second).unwrap(); + let review = Review::new(&branch.id, "old222", ReviewScope::Commit); + store.create_review(&review).unwrap(); + + let remapped = store + .remap_commit_shas( + &branch.id, + &[ + (first.id.as_str(), "old111", "new111"), + (second.id.as_str(), "old222", "new222"), + ], + ) + .unwrap(); + + assert_eq!(remapped, 2); + let first = store.get_commit(&first.id).unwrap().unwrap(); + assert_eq!(first.sha.as_deref(), Some("new111")); + let second = store.get_commit(&second.id).unwrap().unwrap(); + assert_eq!(second.sha.as_deref(), Some("new222")); + let review = store.get_review(&review.id).unwrap().unwrap(); + assert_eq!(review.commit_sha, "new222"); +} + +/// The `(branch_id, sha)` unique index is re-checked inside the transaction, +/// so a target SHA another row already owns is skipped rather than blowing up +/// the whole remap. +#[test] +fn test_remap_commit_shas_skips_target_owned_by_another_row() { + let store = Store::in_memory().unwrap(); + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + + let orphan = Commit::new_with_sha(&branch.id, "old111"); + let owner = Commit::new_with_sha(&branch.id, "new111"); + let movable = Commit::new_with_sha(&branch.id, "old222"); + store.create_commit(&orphan).unwrap(); + store.create_commit(&owner).unwrap(); + store.create_commit(&movable).unwrap(); + + let remapped = store + .remap_commit_shas( + &branch.id, + &[ + (orphan.id.as_str(), "old111", "new111"), + (movable.id.as_str(), "old222", "new222"), + ], + ) + .unwrap(); + + assert_eq!(remapped, 1); + let orphan = store.get_commit(&orphan.id).unwrap().unwrap(); + assert_eq!(orphan.sha.as_deref(), Some("old111")); + let movable = store.get_commit(&movable.id).unwrap().unwrap(); + assert_eq!(movable.sha.as_deref(), Some("new222")); +} + #[test] fn test_delete_branch_cascades_commits() { let store = Store::in_memory().unwrap(); diff --git a/apps/staged/src-tauri/src/test_utils.rs b/apps/staged/src-tauri/src/test_utils.rs index 4fe6a9fe8..37fdd6f04 100644 --- a/apps/staged/src-tauri/src/test_utils.rs +++ b/apps/staged/src-tauri/src/test_utils.rs @@ -39,6 +39,14 @@ impl TempGitRepo { } pub fn run_git(&self, args: &[&str]) -> String { + self.try_run_git(args) + .unwrap_or_else(|stderr| panic!("git {args:?} failed: {stderr}")) + } + + /// Run git and report the exit status instead of asserting on it, for + /// commands whose failure is the point — a `git rebase` that stops on a + /// conflict, say. `Err` carries stderr. + pub fn try_run_git(&self, args: &[&str]) -> Result { let mut command = Command::new("git"); command .arg("-c") @@ -50,14 +58,11 @@ impl TempGitRepo { let output = command.output().unwrap(); - assert!( - output.status.success(), - "git {:?} failed: {}", - args, - String::from_utf8_lossy(&output.stderr) - ); + if !output.status.success() { + return Err(String::from_utf8_lossy(&output.stderr).into_owned()); + } - String::from_utf8(output.stdout).unwrap() + Ok(String::from_utf8(output.stdout).unwrap()) } } diff --git a/apps/staged/src-tauri/src/timeline.rs b/apps/staged/src-tauri/src/timeline.rs index bfa5aeb7a..8ce0993ca 100644 --- a/apps/staged/src-tauri/src/timeline.rs +++ b/apps/staged/src-tauri/src/timeline.rs @@ -159,24 +159,24 @@ pub struct ParentBranchCommit { } fn parse_parent_commit_lines(lines: &[String]) -> Vec { - let mut commits = Vec::new(); - for line in lines { - let parts: Vec<&str> = line.splitn(6, '|').collect(); - if parts.len() >= 6 { - commits.push(ParentBranchCommit { - sha: parts[0].to_string(), - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), - }); - } - } - commits + lines + .iter() + .filter_map(|line| git::parse_branch_commit_line(line)) + .map(|fields| ParentBranchCommit { + sha: fields.sha.to_string(), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + // Committer time: these commits are listed on their own, never + // interleaved with notes, so there's nothing for a rebase-stable + // clock to line up with. + timestamp: fields.committer_timestamp, + }) + .collect() } -/// Parse `%H|%h|%s|%an|%ae|%ct` formatted commit lines into timeline items, +/// Parse [`git::BRANCH_COMMIT_LOG_FORMAT`] commit lines into timeline items, /// looking up DB metadata for session linkage. fn parse_commit_lines( store: &Arc, @@ -185,20 +185,20 @@ fn parse_commit_lines( ) -> Vec { let mut commits = Vec::new(); for line in lines { - let parts: Vec<&str> = line.splitn(6, '|').collect(); - if parts.len() >= 6 { - let sha = parts[0].to_string(); + if let Some(fields) = git::parse_branch_commit_line(line) { + let sha = fields.sha.to_string(); let our_commit = store.get_commit_by_sha(branch_id, &sha).unwrap_or(None); let resolved = store .resolve_session_status(our_commit.as_ref().and_then(|c| c.session_id.as_deref())); commits.push(CommitTimelineItem { id: our_commit.as_ref().map(|c| c.id.clone()), sha, - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + timestamp: fields.author_timestamp, + sort_timestamp: fields.author_timestamp, order: 0, session_id: resolved.session_id, session_status: resolved.status, @@ -230,9 +230,12 @@ fn fetch_remote_commits( } else { format!("{base_ref}..HEAD") }; - let format_arg = "--format=%H|%h|%s|%an|%ae|%ct"; - let output = branches::run_workspace_git(ws_name, repo_subpath, &["log", format_arg, &range]) - .map_err(|e| format!("Failed to load commits from workspace: {e}"))?; + let output = branches::run_workspace_git( + ws_name, + repo_subpath, + &["log", git::BRANCH_COMMIT_LOG_FORMAT, &range], + ) + .map_err(|e| format!("Failed to load commits from workspace: {e}"))?; let lines: Vec = output .lines() .filter(|l| !l.is_empty()) @@ -260,7 +263,8 @@ fn map_local_commits( subject: gc.subject.clone(), author: gc.author.clone(), author_email: gc.author_email.clone(), - timestamp: gc.timestamp, + timestamp: gc.author_timestamp, + sort_timestamp: gc.author_timestamp, order: gc.order, session_id: resolved.session_id, session_status: resolved.status, @@ -285,6 +289,38 @@ fn non_empty_acp_title(resolved: &ResolvedSession) -> Option { .map(str::to_string) } +/// Clamp commit sort keys so they never decrease in branch order. +/// +/// Committer dates are naturally non-decreasing along a branch; the author +/// dates the timeline sorts on aren't — a cherry-pick keeps its original +/// author date, and an interactive rebase can reorder commits — so a commit +/// could otherwise sort above one that precedes it in `git log`. Walking +/// oldest-first with a running max pins each commit to at least its +/// predecessor's effective time, keeping the rendered order the same as git's. +/// +/// Only `sort_timestamp` moves; `timestamp` keeps the real author date, which +/// is what the UI renders. +/// +/// `commits` arrives in `git log` order (newest first), as both producers emit +/// it, so the walk runs in reverse. +fn clamp_commit_sort_timestamps(commits: &mut [CommitTimelineItem]) { + clamp_timestamps_monotonic(commits.iter_mut().rev().map(|c| &mut c.sort_timestamp)); +} + +/// Raise each timestamp to at least its predecessor's, in iteration order. +/// +/// Shared by the two timelines that interleave git commits with DB-timed items +/// and so have to sort on author date: this module's branch timeline and the +/// agent-facing branch history in `session_commands`. Callers pass their +/// timestamps in branch order, oldest first. +pub(crate) fn clamp_timestamps_monotonic<'a>(timestamps: impl Iterator) { + let mut floor = i64::MIN; + for timestamp in timestamps { + floor = (*timestamp).max(floor); + *timestamp = floor; + } +} + /// Public wrapper for `build_branch_timeline` for use by the web server. pub fn build_branch_timeline_public( store: &Arc, @@ -413,6 +449,8 @@ fn build_branch_timeline(store: &Arc, branch_id: &str) -> Result, branch_id: &str) -> Result CommitTimelineItem { + let store = Arc::new(Store::in_memory().unwrap()); + let commits = parse_commit_lines(&store, "branch-1", &[line.to_string()]); + commits.into_iter().next().unwrap() + } + + #[test] + fn parse_commit_lines_takes_its_timestamp_from_author_time() { + let commit = parsed_commit( + "abc123\x1fabc123a\x1fTest\x1ftest@example.com\x1f9100\x1f1100\x1ffeat: parser", + ); + + assert_eq!(commit.subject, "feat: parser"); + assert_eq!(commit.timestamp, 1100); + assert_eq!(commit.sort_timestamp, 1100); + } + + fn commit_at(subject: &str, timestamp: i64, order: i64) -> CommitTimelineItem { + CommitTimelineItem { + id: None, + sha: format!("sha-{order}"), + short_sha: format!("sha-{order}"), + subject: subject.to_string(), + author: "Test".to_string(), + author_email: "test@example.com".to_string(), + timestamp, + sort_timestamp: timestamp, + order, + session_id: None, + session_status: None, + completion_reason: None, + is_own_commit: false, + } + } + + /// A cherry-picked commit keeps its original author date, which would sort + /// it above the commit it actually follows. The clamp pins it down instead. + #[test] + fn clamp_keeps_out_of_order_author_dates_in_branch_order() { + // Newest-first, as `git log` emits. + let mut commits = vec![ + commit_at("fix: lexer", 300, 2), + commit_at("chore: cherry-picked", 100, 1), + commit_at("feat: parser", 200, 0), + ]; + + clamp_commit_sort_timestamps(&mut commits); + + assert_eq!( + commits.iter().map(|c| c.sort_timestamp).collect::>(), + vec![300, 200, 200] + ); + assert_eq!( + commits.iter().map(|c| c.timestamp).collect::>(), + vec![300, 100, 200], + "the rendered author dates stay untouched" + ); + } + + #[test] + fn clamp_leaves_already_increasing_author_dates_alone() { + let mut commits = vec![commit_at("fix: lexer", 300, 1), commit_at("feat", 200, 0)]; + + clamp_commit_sort_timestamps(&mut commits); + + assert_eq!( + commits.iter().map(|c| c.sort_timestamp).collect::>(), + vec![300, 200] + ); + } + + /// Commit the working tree with a fixed author date, leaving the committer + /// date at "now" — the same split a rebase creates. + fn commit_authored_at(repo: &TempGitRepo, message: &str, author_epoch: i64) -> String { + repo.run_git(&["add", "."]); + repo.run_git(&[ + "commit", + "--date", + &format!("@{author_epoch} +0000"), + "-m", + message, + ]); + repo.run_git(&["rev-parse", "HEAD"]).trim().to_string() + } + + fn committer_time(repo: &TempGitRepo, sha: &str) -> i64 { + repo.run_git(&["show", "-s", "--format=%ct", sha]) + .trim() + .parse() + .unwrap() + } + + /// The timeline interleaves commits with notes by timestamp. A rebase + /// rewrites every committer date to "now" while notes keep their DB times, + /// so sorting on committer time would sink every commit below every note. + /// Author dates survive the rewrite, so the interleaving does too. + #[test] + fn build_branch_timeline_keeps_notes_interleaved_across_a_rebase() { + const FIRST_AUTHORED_AT: i64 = 1_700_000_000; + const NOTE_WRITTEN_AT: i64 = 1_700_000_100; + const SECOND_AUTHORED_AT: i64 = 1_700_000_200; + + let repo = TempGitRepo::new(); + repo.write_file("base.txt", "base\n"); + let base_sha = repo.commit("chore: base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &base_sha]); + + repo.run_git(&["checkout", "-b", "feature"]); + repo.write_file("parser.txt", "parser\n"); + commit_authored_at(&repo, "feat: parser", FIRST_AUTHORED_AT); + repo.write_file("lexer.txt", "lexer\n"); + let old_head = commit_authored_at(&repo, "fix: lexer", SECOND_AUTHORED_AT); + + let (store, branch) = store_with_branch(&repo); + let mut note = Note::new(&branch.id, "Plan", "the plan"); + note.created_at = NOTE_WRITTEN_AT * 1000; + note.updated_at = note.created_at; + note.completed_at = Some(note.created_at); + store.create_note(¬e).unwrap(); + + let before = build_branch_timeline(&store, &branch.id).unwrap(); + assert_eq!( + timeline_order(&before), + vec!["feat: parser", "Plan", "fix: lexer"], + "the note was written between the two commits" + ); + + // Move the base out from under the branch, then really rebase onto it. + repo.run_git(&["checkout", "main"]); + repo.write_file("moved.txt", "moved\n"); + let moved_base = repo.commit("chore: move base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &moved_base]); + repo.run_git(&["checkout", "feature"]); + repo.run_git(&["rebase", "--signoff", "origin/main"]); + let new_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + assert_ne!(new_head, old_head, "the rebase must rewrite the SHAs"); + + let after = build_branch_timeline(&store, &branch.id).unwrap(); + + assert_eq!( + timeline_order(&after), + vec!["feat: parser", "Plan", "fix: lexer"], + "the note must stay between the two commits after the rebase" + ); + assert_eq!( + after + .commits + .iter() + .map(|c| c.timestamp) + .collect::>(), + vec![SECOND_AUTHORED_AT, FIRST_AUTHORED_AT], + "the rewritten commits keep their original author dates" + ); + // The committer dates really are elsewhere — a `%ct` sort would put + // both commits after the note rather than around it. + assert!( + committer_time(&repo, &new_head) > NOTE_WRITTEN_AT, + "committer time is 'now', long after the note was written" + ); + } + + /// Commit subjects and note titles, merged and sorted the way the frontend + /// does it (`BranchTimeline.svelte`): ascending sort timestamp, `order` + /// breaking ties between commits. + fn timeline_order(timeline: &BranchTimeline) -> Vec<&str> { + let mut items: Vec<(i64, i64, &str)> = timeline + .commits + .iter() + .map(|c| (c.sort_timestamp, c.order, c.subject.as_str())) + .chain(timeline.notes.iter().map(|n| { + ( + n.completed_at.unwrap_or(n.created_at) / 1000, + 0, + n.title.as_str(), + ) + })) + .collect(); + items.sort_by_key(|(timestamp, order, _)| (*timestamp, *order)); + items.into_iter().map(|(_, _, label)| label).collect() + } } diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 283136046..22009fd17 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -995,34 +995,37 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result = output .lines() - .filter(|l| !l.is_empty()) .enumerate() .filter_map(|(i, line)| { - let parts: Vec<&str> = line.splitn(6, '|').collect(); - if parts.len() >= 6 { - Some(crate::CommitTimelineItem { - id: None, - sha: parts[0].to_string(), - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), - order: (max_count - 1 - i) as i64, - session_id: None, - session_status: None, - completion_reason: None, - is_own_commit: false, - }) - } else { - None - } + let fields = crate::git::parse_branch_commit_line(line)?; + Some(crate::CommitTimelineItem { + id: None, + sha: fields.sha.to_string(), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + // Committer time, as in the Tauri command this mirrors. + timestamp: fields.committer_timestamp, + sort_timestamp: fields.committer_timestamp, + order: (max_count - 1 - i) as i64, + session_id: None, + session_status: None, + completion_reason: None, + is_own_commit: false, + }) }) .collect(); Ok(crate::RepoDefaultBranchTimeline { diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index eba9597af..1b288fba1 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -410,7 +410,7 @@ const all: AnyCandidate[] = [...candidates]; for (const commit of timeline.commits) { if (!commit.sha) continue; // skip pending - all.push({ kind: 'commit', timestamp: commit.timestamp }); + all.push({ kind: 'commit', timestamp: commit.sortTimestamp }); } if (all.length === 0) return empty; diff --git a/apps/staged/src/lib/features/sessions/hashtagItems.test.ts b/apps/staged/src/lib/features/sessions/hashtagItems.test.ts index 20d0e9a5c..0b51cd07c 100644 --- a/apps/staged/src/lib/features/sessions/hashtagItems.test.ts +++ b/apps/staged/src/lib/features/sessions/hashtagItems.test.ts @@ -92,6 +92,7 @@ describe('timelineToHashtagItems', () => { authorEmail: 'test@example.com', isOwnCommit: true, timestamp: 1, + sortTimestamp: 1, order: 0, sessionId: null, sessionStatus: null, @@ -149,6 +150,7 @@ describe('timelineToHashtagItems', () => { authorEmail: 'test@example.com', isOwnCommit: true, timestamp: 2000, + sortTimestamp: 2000, order: 0, sessionId: null, sessionStatus: null, @@ -163,6 +165,7 @@ describe('timelineToHashtagItems', () => { authorEmail: 'test@example.com', isOwnCommit: true, timestamp: 6000, + sortTimestamp: 6000, order: 1, sessionId: null, sessionStatus: null, diff --git a/apps/staged/src/lib/features/sessions/hashtagItems.ts b/apps/staged/src/lib/features/sessions/hashtagItems.ts index bb683ba92..2c70a5c9f 100644 --- a/apps/staged/src/lib/features/sessions/hashtagItems.ts +++ b/apps/staged/src/lib/features/sessions/hashtagItems.ts @@ -225,7 +225,7 @@ function timelineToSortableHashtagItems( repoSubpath, branchId, projectId, - sortTimestamp: commit.timestamp, + sortTimestamp: commit.sortTimestamp, sortOrder: commit.order, }); } diff --git a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte index 23f649792..211a9d159 100644 --- a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte +++ b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte @@ -217,6 +217,7 @@ secondaryMeta?: string; tertiaryMeta?: string; deleting?: boolean; + /** Sort key in unix seconds. Commits pass their clamped `sortTimestamp`; displayed times come from `meta`. */ timestamp: number; /** Position in git's topological order (0 = oldest). Tiebreaker for same-second timestamps. */ order: number; @@ -552,7 +553,7 @@ secondaryMeta: isDeleting || isRunning ? undefined : commit.shortSha || undefined, tertiaryMeta: showAuthor ? commit.author : undefined, deleting: isDeleting, - timestamp: commit.timestamp, + timestamp: commit.sortTimestamp, order: commit.order, sessionId: commit.sessionId ?? undefined, commitSha: commit.sha || undefined, @@ -568,7 +569,7 @@ if (type === 'commit' && commit.sha) { commitAnchors.set(commit.sha, { - timestamp: commit.timestamp, + timestamp: commit.sortTimestamp, order: commit.order, shortSha: commit.shortSha || undefined, }); diff --git a/apps/staged/src/lib/types.ts b/apps/staged/src/lib/types.ts index ac8d71022..14f9f4164 100644 --- a/apps/staged/src/lib/types.ts +++ b/apps/staged/src/lib/types.ts @@ -126,8 +126,13 @@ export interface CommitTimelineItem { subject: string; author: string; authorEmail: string; - /** Unix timestamp in seconds */ + /** Unix timestamp in seconds — author time for branch commits, so it survives a rebase. */ timestamp: number; + /** + * Unix timestamp in seconds to sort on, clamped so it can't decrease in + * branch order. Order only — render `timestamp`. + */ + sortTimestamp: number; /** Position in git's topological order (0 = oldest). Tiebreaker for same-second timestamps. */ order: number; sessionId: string | null;