diff --git a/lore-revision/src/repository/status.rs b/lore-revision/src/repository/status.rs index 00ade2b..7b8b04c 100644 --- a/lore-revision/src/repository/status.rs +++ b/lore-revision/src/repository/status.rs @@ -1165,6 +1165,125 @@ pub async fn status( return Ok(()); } + // Scan before the staged diff: the scan discards reverted uncommitted + // adds from the shared staged state, which the staged diff must not + // report as stale adds. + if show_scan { + lore_debug!( + "Calculating deltas against filesystem for {} paths", + paths.len() + ); + + let mut tasks = JoinSet::new(); + for path in paths.iter() { + let repository = repository.clone(); + let state_current = state_current.clone(); + let state_staged = state_staged.clone(); + let path = path.clone(); + let layer_mounts = layer_mounts.clone(); + let summary = summary.clone(); + let exists = if let Some(path) = path.as_ref() { + let mut exists_in_state = false; + let mut exists_in_filesystem = false; + + let state = if has_staged { + state_staged.clone() + } else { + state_current.clone() + }; + + let node_link = state + .find_node_link(repository.clone(), path.as_str()) + .await + .unwrap_or_default(); + if node_link.is_valid() { + exists_in_state = true; + } else { + let absolute_path = path.to_absolute_path(repository.require_path()?); + exists_in_filesystem = std::fs::exists(absolute_path).unwrap_or_default(); + } + + if !exists_in_state && !exists_in_filesystem { + emit_path_ignore(path.as_str()).await; + lore_trace!("Ignoring invalid path: {path}"); + } + + exists_in_state || exists_in_filesystem + } else { + true + }; + + if exists { + lore_spawn!(tasks, { + async move { + if let Some(path) = path.as_ref() { + lore_debug!( + "Calculating deltas against filesystem path: {}", + path.as_str() + ); + } else { + lore_debug!( + "Calculating deltas against filesystem for full repository" + ); + } + + let start = Instant::now(); + + // Scan uses staged state as diff base with scan_dirty=true. + // Content hashes in staged state are either zero (add nodes) + // or equal to current revision hashes, so the comparison is + // effectively filesystem vs committed content. + // The current revision is passed as the second pair so the + // walk can distinguish "node exists in staged but not in + // committed" — i.e. unstaged adds — from regular tracked + // files. Dirty flags are set/cleared inline during the walk. + let (changes, _stats) = state::diff_filesystem_ex( + repository.clone(), + state_staged.clone(), + repository.clone(), + state_current.clone(), + path, + FilterMode::Full, + true, // scan_dirty + layer_mounts.clone(), + ) + .await + .forward::("computing diff against filesystem")?; + + lore_debug!( + "Scan found {} file system changes in {:.3}s", + changes.len(), + start.elapsed().as_secs_f64(), + ); + + for change in changes.iter() { + let size = + file_size_from_node_change_path(repository.require_path()?, change) + .await?; + + // Emit event for display (dirty set/clear handled inline by diff) + if !change.flags.is_stage() { + summary.classify(change); + event::LoreEvent::RepositoryStatusFile( + LoreRepositoryStatusFileEventData::from_node_change( + change, size, + ), + ) + .send(); + } else { + lore_debug!("Ignore staged file {}", change.path); + } + } + + Ok(()) + } + }); + } + + lore_drain_tasks!(tasks, StatusError::internal("Recursion task failed"))?; + } + } + // Compare current state against staged state if show_staged && has_staged { lore_debug!("Calculating deltas against staged revision"); @@ -1295,123 +1414,6 @@ pub async fn status( lore_drain_tasks!(tasks, StatusError::internal("Recursion task failed"))?; } - // Compare current/staged state against filesystem - if show_scan { - lore_debug!( - "Calculating deltas against filesystem for {} paths", - paths.len() - ); - - let mut tasks = JoinSet::new(); - for path in paths.iter() { - let repository = repository.clone(); - let state_current = state_current.clone(); - let state_staged = state_staged.clone(); - let path = path.clone(); - let layer_mounts = layer_mounts.clone(); - let summary = summary.clone(); - let exists = if let Some(path) = path.as_ref() { - let mut exists_in_state = false; - let mut exists_in_filesystem = false; - - let state = if has_staged { - state_staged.clone() - } else { - state_current.clone() - }; - - let node_link = state - .find_node_link(repository.clone(), path.as_str()) - .await - .unwrap_or_default(); - if node_link.is_valid() { - exists_in_state = true; - } else { - let absolute_path = path.to_absolute_path(repository.require_path()?); - exists_in_filesystem = std::fs::exists(absolute_path).unwrap_or_default(); - } - - if !exists_in_state && !exists_in_filesystem { - emit_path_ignore(path.as_str()).await; - lore_trace!("Ignoring invalid path: {path}"); - } - - exists_in_state || exists_in_filesystem - } else { - true - }; - - if exists { - lore_spawn!(tasks, { - async move { - if let Some(path) = path.as_ref() { - lore_debug!( - "Calculating deltas against filesystem path: {}", - path.as_str() - ); - } else { - lore_debug!( - "Calculating deltas against filesystem for full repository" - ); - } - - let start = Instant::now(); - - // Scan uses staged state as diff base with scan_dirty=true. - // Content hashes in staged state are either zero (add nodes) - // or equal to current revision hashes, so the comparison is - // effectively filesystem vs committed content. - // The current revision is passed as the second pair so the - // walk can distinguish "node exists in staged but not in - // committed" — i.e. unstaged adds — from regular tracked - // files. Dirty flags are set/cleared inline during the walk. - let (changes, _stats) = state::diff_filesystem_ex( - repository.clone(), - state_staged.clone(), - repository.clone(), - state_current.clone(), - path, - FilterMode::Full, - true, // scan_dirty - layer_mounts.clone(), - ) - .await - .forward::("computing diff against filesystem")?; - - lore_debug!( - "Scan found {} file system changes in {:.3}s", - changes.len(), - start.elapsed().as_secs_f64(), - ); - - for change in changes.iter() { - let size = - file_size_from_node_change_path(repository.require_path()?, change) - .await?; - - // Emit event for display (dirty set/clear handled inline by diff) - if !change.flags.is_stage() { - summary.classify(change); - event::LoreEvent::RepositoryStatusFile( - LoreRepositoryStatusFileEventData::from_node_change( - change, size, - ), - ) - .send(); - } else { - lore_debug!("Ignore staged file {}", change.path); - } - } - - Ok(()) - } - }); - } - - lore_drain_tasks!(tasks, StatusError::internal("Recursion task failed"))?; - } - } - // Emit the aggregate dirty-node summary for reconciling status runs. For // --scan these are the changes detected against the filesystem; for // --check-dirty they are the nodes that stayed dirty after verification. diff --git a/lore-revision/src/state.rs b/lore-revision/src/state.rs index 8ee26ea..f0d9fa8 100644 --- a/lore-revision/src/state.rs +++ b/lore-revision/src/state.rs @@ -4722,6 +4722,30 @@ async fn apply_pending_discards( } let initial_ancestor = discard_node.parent; + + // For a directory, discard the whole subtree below it first so its node + // slots are reclaimed; the node itself is unlinked from its parent and + // discarded by node_discard_patch below. Each child's sibling pointer is + // captured before discarding it, since discard_node repurposes that + // pointer for the block's free list. + if discard_node.is_directory() { + let mut child_ref = discard_node.child(); + while let Some(child_id) = child_ref { + let child_node = state.node(repository.clone(), child_id).await?; + let next_sibling = child_node.sibling(); + node_discard_recurse( + state.clone(), + repository.clone(), + child_id, + true, /* recurse */ + true, /* discard */ + |_, _| {}, + ) + .await?; + child_ref = next_sibling; + } + } + node_discard_patch( state.clone(), repository.clone(), @@ -5644,6 +5668,14 @@ async fn emit_filesystem_subtree_deletes( Ok(false) } +/// Match each filesystem item from `file_receiver` against `node_list` (the +/// `from` state's children) and `current_node_list` (the `current` state's +/// children), emitting changes into `changes`, marking matched entries in +/// `node_list_found`, spawning subtree-recursion tasks into `tasks`, and +/// queueing stale directory nodes into `pending_discards`. Items with no +/// match in `node_list` are buffered and processed as new adds once the +/// receiver is drained. `node_list` and `current_node_list` must be sorted by +/// name; the binary searches here rely on that ordering. #[allow(clippy::too_many_arguments)] async fn diff_filesystem_directory_walk( ctx: &DiffFilesystemContext, @@ -5975,6 +6007,25 @@ async fn diff_filesystem_directory_walk( continue; }; + // Directory staged then removed from disk before any commit: discard it + // rather than emit a Delete, since nothing committed backs it. + if ctx.scan_dirty && from_node.node.is_directory() { + let in_current = current_node_list + .children + .as_slice() + .binary_search_by(|child| child.name.cmp(&from_named_node.name)) + .is_ok(); + if !in_current { + lore_trace!( + "Queueing reverted uncommitted directory node {} (no entry at {}, not in current)", + from_named_node.node, + from_node.path + ); + pending_discards.push(from_named_node.node); + continue; + } + } + // Emit deletes only for the materialized portion of the subtree, // suppressing directories the filter merely descended through but never // wrote to disk (see emit_filesystem_subtree_deletes). @@ -5995,11 +6046,8 @@ async fn diff_filesystem_directory_walk( continue; } - // A leaf node present in state_from but not in state_current, with - // no file on disk, is an unstaged add that the user reverted by - // removing the file. Discard the node so state_staged matches the - // filesystem rather than emitting a Delete change for a node that - // shouldn't exist. + // Leaf staged but absent from both the commit and disk: a reverted + // unstaged add. Discard it rather than emit a Delete. let in_current = current_node_list .children .as_slice() diff --git a/lore-revision/tests/reverted_directory.rs b/lore-revision/tests/reverted_directory.rs new file mode 100644 index 0000000..a2d6505 --- /dev/null +++ b/lore-revision/tests/reverted_directory.rs @@ -0,0 +1,226 @@ +// SPDX-FileCopyrightText: 2026 Epic Games, Inc. +// SPDX-License-Identifier: MIT + +//! Working-tree scan handling of a reverted uncommitted directory add. +//! +//! When a directory (and its contents) is indexed as an uncommitted add and +//! then removed from disk before any commit, the next scan must discard the +//! stale node rather than report a delete. The parent has no committed base the +//! directory could be a deletion of, so a delete entry would be an unremovable +//! "zombie" — the same treatment already given to a reverted single-file add. + +#[cfg(test)] +mod tests { + #![allow(clippy::disallowed_methods)] // Test fixture writes; not subject to repository write-token discipline. + + use std::fs::File; + use std::io::Write; + use std::path::Path; + use std::sync::Arc; + + use lore_base::error::NoRemote; + use lore_base::runtime::LORE_CONTEXT; + use lore_base::runtime::runtime; + use lore_base::types::Context; + use lore_revision::branch; + use lore_revision::change::FileAction; + use lore_revision::filter::FilterMode; + use lore_revision::lore::RepositoryId; + use lore_revision::repository; + use lore_revision::repository::RepositoryContext; + use lore_revision::repository::RepositoryFormat; + use lore_revision::repository::load_filter; + use lore_revision::state; + use lore_transport::ProtocolError; + + include!("helper.rs"); + + /// Create (or truncate) a read/write file at `path` and write `contents` to + /// it, returning the open handle. Panics if the file cannot be created or + /// written, since a failed fixture setup invalidates the test. + fn create_file(path: &Path, contents: &[u8]) -> File { + let mut file = File::options() + .create(true) + .truncate(true) + .read(true) + .write(true) + .open(path) + .unwrap_or_else(|_| panic!("Failed to create test file at {}", path.display())); + file.write_all(contents) + .unwrap_or_else(|_| panic!("Failed to write test file at {}", path.display())); + file + } + + /// Build a fresh on-disk repository at `path` with no commits (revision 0) + /// and return a write-capable [`RepositoryContext`] for it. + async fn create_repository( + path: &Path, + repository_id: RepositoryId, + immutable_store: Arc, + mutable_store: Arc, + ) -> Arc { + std::fs::create_dir_all(path).expect("Create repository directory failed"); + let default_branch = Context::from(uuid::Uuid::now_v7()); + let write_token = repository::RepositoryWriteToken::acquire(path).await; + let created_repo = repository::create_local( + path, + &write_token, + repository_id, + default_branch, + branch::DEFAULT_DEFAULT_NAME.to_string(), + repository::RepositoryConfig::default(), + false, + ) + .await + .expect("Failed to create repository"); + + let repository = Arc::new( + RepositoryContext::new( + Some(path.to_path_buf()), + immutable_store, + mutable_store, + repository_id, + created_repo.instance_id, + Err(ProtocolError::from(NoRemote)), + load_filter(path).expect("Failed to load filter"), + RepositoryFormat::Lore, + ) + .with_write_token(write_token.share()), + ); + lore_revision::instance::store_current_anchor_branch(&repository, default_branch) + .await + .expect("Failed to store anchor branch"); + repository + } + + /// Reconcile the working tree against the staged state, mutating `state_staged` + /// in place exactly as `lore status --scan` does, and return the detected + /// changes. + async fn scan( + repository: Arc, + state_staged: Arc, + state_current: Arc, + ) -> Vec { + let (changes, _stats) = state::diff_filesystem_ex( + repository.clone(), + state_staged, + repository, + state_current, + None, /* full tree */ + FilterMode::Full, + true, /* scan_dirty */ + Arc::new(Vec::new()), + ) + .await + .expect("Failed to diff filesystem"); + changes + } + + /// A directory indexed as an uncommitted add (along with its contents) and + /// then removed from disk must be discarded on the next scan rather than + /// reported as a delete: with no committed base there is nothing to delete, + /// and a delete entry would be an unremovable "zombie". + #[tokio::test] + async fn removed_uncommitted_directory_is_discarded_not_deleted() { + let (immutable_store, mutable_store, execution) = + test_store_create().await.expect("Failed to create stores"); + let repository_id = RepositoryId::from(uuid::Uuid::now_v7()); + + runtime() + .spawn(LORE_CONTEXT.scope(execution.clone(), async move { + let tempdir = generate_tempdir(); + let path = tempdir.to_path_buf(); + let repository = create_repository( + path.as_path(), + repository_id, + immutable_store.clone(), + mutable_store.clone(), + ) + .await; + + // A directory with content that gets indexed as an uncommitted + // add (the directory node plus its child file). + std::fs::create_dir(path.join("ghost").as_path()) + .expect("Create ghost directory failed"); + let _ = create_file(path.join("ghost").join("inner.txt").as_path(), &[7, 7, 7]); + + let (current_revision, _branch) = + lore_revision::instance::load_current_anchor(&repository) + .await + .expect("Failed to load current anchor"); + let state_current = state::State::deserialize(repository.clone(), current_revision) + .await + .expect("Failed to deserialize current state"); + let state_staged = state::State::deserialize(repository.clone(), current_revision) + .await + .expect("Failed to deserialize staged state"); + + // First scan indexes the directory as an add. + let changes = scan( + repository.clone(), + state_staged.clone(), + state_current.clone(), + ) + .await; + assert!( + changes + .iter() + .any(|c| c.path.as_str() == "ghost" && c.action == FileAction::Add), + "expected the new directory to be indexed as an add, found: {:?}", + changes + .iter() + .map(|c| (c.path.as_str().to_string(), c.action)) + .collect::>() + ); + assert!( + changes.iter().any(|c| c.path.as_str() == "ghost/inner.txt"), + "expected the directory's contents to be indexed too, found: {:?}", + changes + .iter() + .map(|c| (c.path.as_str().to_string(), c.action)) + .collect::>() + ); + + // Remove it from disk and rescan against the same staged state. + std::fs::remove_dir_all(path.join("ghost")) + .expect("Failed to remove ghost directory"); + let changes = scan( + repository.clone(), + state_staged.clone(), + state_current.clone(), + ) + .await; + assert!( + changes + .iter() + .all(|c| !c.path.as_str().starts_with("ghost")), + "removed uncommitted directory must be discarded, not reported, found: {:?}", + changes + .iter() + .map(|c| (c.path.as_str().to_string(), c.action)) + .collect::>() + ); + + // A further scan stays clean — the node was discarded, not merely + // hidden, so it cannot resurface. + let changes = scan( + repository.clone(), + state_staged.clone(), + state_current.clone(), + ) + .await; + assert!( + changes + .iter() + .all(|c| !c.path.as_str().starts_with("ghost")), + "discarded directory must not resurface on a later scan, found: {:?}", + changes + .iter() + .map(|c| (c.path.as_str().to_string(), c.action)) + .collect::>() + ); + })) + .await + .expect("Test task panicked"); + } +} diff --git a/scripts/test/test_dirty.py b/scripts/test/test_dirty.py index e46baf0..6d93fec 100644 --- a/scripts/test/test_dirty.py +++ b/scripts/test/test_dirty.py @@ -10,6 +10,7 @@ import json import logging import os +import shutil import pytest from lore_parsers import parse_jsonl, parse_status_json, parse_status_summary_json @@ -2806,3 +2807,79 @@ def check(label: str, **kwargs) -> None: repo.dirty(added, offline=True) check("second dirty, plain status") check("second dirty, --check-dirty", check_dirty=True) + + +@pytest.mark.smoke +def test_scan_discards_reverted_uncommitted_directory(new_lore_repo): + """Verify that `status --scan` discards reverted uncommitted directories. + + A directory is staged, then removed from disk before any commit -- a + "zombie" entry that is in staged state but neither committed nor on disk. + Only `--scan` walks the filesystem and can discard it (the filesystem + diff, and with it the discard logic, runs only `if show_scan`). + + The scan applies the discard to the shared staged state before reporting, + so a single `status --scan` reflects it. Verify that the first scan: + - Reports neither the reverted directory nor anything staged under it (no + `type` filter, since the directory node itself is what the discard logic + operates on) + - Reports no duplicate entries + - Still reports an unrelated concurrent change + - Does not resurrect the directory on a second scan + """ + repo: Lore = new_lore_repo() + + with repo.open_file("base.txt", "w+") as f: + f.write("base file\n") + repo.stage(scan=True, offline=True) + repo.commit(offline=True) + + os.makedirs(os.path.join(repo.path, "reverted_dir", "subdir"), exist_ok=True) + with repo.open_file("reverted_dir/file1.txt", "w+") as f: + f.write("file1\n") + with repo.open_file("reverted_dir/subdir/file2.txt", "w+") as f: + f.write("file2\n") + repo.stage(scan=True, offline=True) + + with repo.open_file("other_change.txt", "w+") as f: + f.write("other\n") + repo.dirty("other_change.txt", offline=True) + + # Reverts the staged add: the directory is now staged but neither + # committed nor present on disk. + shutil.rmtree(os.path.join(repo.path, "reverted_dir")) + + def reverted_dir_entries(entries: list[dict]) -> list[str]: + paths = [to_posix(e.get("path", "")) for e in entries] + return [p for p in paths if p == "reverted_dir" or p.startswith("reverted_dir/")] + + entries = get_status_files(repo, scan=True) + all_paths = [to_posix(e.get("path", "")) for e in entries] + + # No `type` filter here: the directory node itself, not just the files + # under it, is what the discard logic operates on. + assert not reverted_dir_entries(entries), ( + f"Reverted directory should be fully discarded from status, " + f"got: {reverted_dir_entries(entries)}" + ) + assert len(all_paths) == len(set(all_paths)), ( + f"Duplicate entries reported: {sorted(all_paths)}" + ) + + other_entries = [e for e in entries if to_posix(e.get("path", "")) == "other_change.txt"] + assert len(other_entries) == 1, ( + f"other_change.txt should be reported once, got: {other_entries}" + ) + other_entry = other_entries[0] + assert other_entry.get("action") == "add", ( + f"other_change.txt should be marked as add: {other_entry}" + ) + assert other_entry.get("flagDirty") is True, ( + f"other_change.txt should be flagDirty: {other_entry}" + ) + + entries_again = get_status_files(repo, scan=True) + assert not reverted_dir_entries(entries_again), ( + f"Later scan resurrected the reverted directory: " + f"{reverted_dir_entries(entries_again)}" + )