diff --git a/lore-revision/src/state.rs b/lore-revision/src/state.rs index 61f792d2..31fea631 100644 --- a/lore-revision/src/state.rs +++ b/lore-revision/src/state.rs @@ -4652,6 +4652,12 @@ pub async fn diff_filesystem_ex( /// drained — discarding mid-walk mutates `parent.child` / sibling chains /// under walks that are still reading them and races into /// `node_discard_patch`'s `"Discard hierarchy broken"`. +/// +/// Entries are applied in insertion order: subtree discards are queued +/// post-order (children before their directory), and a directory can only +/// be unlinked after its children are gone. Sorting by node id would break +/// that invariant, since block slot reuse makes ids non-monotonic with +/// tree depth. async fn apply_pending_discards( state: Arc, repository: Arc, @@ -4660,8 +4666,8 @@ async fn apply_pending_discards( if pending_discards.is_empty() { return Ok(()); } - pending_discards.sort_unstable(); - pending_discards.dedup(); + let mut seen = std::collections::HashSet::with_capacity(pending_discards.len()); + pending_discards.retain(|node_id| seen.insert(*node_id)); for discard_node_id in pending_discards { let Ok(discard_node) = state.node(repository.clone(), discard_node_id).await else { @@ -4843,6 +4849,7 @@ async fn diff_filesystem_subtree_impl( // Path doesn't exist on filesystem - everything in state is deleted diff_filesystem_missing( ctx.from, + ctx.current, ctx.filesystem_path, ctx.filter_mode, ctx.scan_dirty, @@ -5494,6 +5501,19 @@ async fn flush_pending_dir_deletes( Ok(()) } +/// Outcome of one node in the missing-subtree delete walk. +#[derive(Clone, Copy, PartialEq)] +enum SubtreeDeleteOutcome { + /// A delete was emitted for this node or a descendant (buffered ancestor + /// directories were flushed). + Materialized, + /// Nothing under this node materialized; the node was kept as-is. + Dropped, + /// The node was queued for discard as a reverted unstaged add (implies + /// nothing materialized and no delete was emitted). + Discarded, +} + /// Walk a revision subtree that is absent from the filesystem and emit `Delete` /// changes for only the portion that was actually materialized on disk under the /// active filter, returning whether anything materialized. @@ -5517,6 +5537,17 @@ async fn flush_pending_dir_deletes( /// the granularity it is reported. `node_mark_dirty` short-circuits on a node /// already carrying the base `Dirty` bit (which `DirtyDelete` includes), so a /// sibling's upward propagation never clobbers a directory's `DirtyDelete`. +/// +/// Deletes are only real for content that exists in the current revision. +/// `current_node` is this node's counterpart in `current_state` (the committed +/// revision), or `INVALID_NODE` when the path is not part of it. During a scan, +/// an unstaged node without a counterpart is a reverted add — the user created +/// it and removed it again without ever committing — so instead of reporting a +/// phantom `Delete` and persisting `DirtyDelete`, the node is queued into +/// `pending_discards` (post-order, children before their directory) and dropped +/// from the staged tree. A staged descendant keeps its ancestor chain alive; a +/// filter-excluded child is never visited, so its parent directory must also +/// survive or the excluded node would be orphaned. #[allow(clippy::too_many_arguments)] async fn emit_filesystem_subtree_deletes( state: Arc, @@ -5528,9 +5559,25 @@ async fn emit_filesystem_subtree_deletes( scan_dirty: bool, sink: &mut ChangeSink<'_>, pending: &mut Vec<(NodeID, RelativePath)>, -) -> Result { + current_state: &Arc, + current_repository: &Arc, + current_node: NodeID, + pending_discards: &mut Vec, +) -> Result { + let in_current = current_node.is_valid_node_id(); + let revertible = scan_dirty && !in_current && !node.is_staged(); + // Caller guarantees `node` is not filter-excluded. if node.is_file() || node.is_link() { + if revertible { + lore_trace!( + "Queueing reverted-add node {} for discard (no file at {}, not in current)", + node_id, + path + ); + pending_discards.push(node_id); + return Ok(SubtreeDeleteOutcome::Discarded); + } flush_pending_dir_deletes(&state, &repository, sink, pending, scan_dirty).await?; if scan_dirty { state @@ -5538,7 +5585,7 @@ async fn emit_filesystem_subtree_deletes( .await?; } emit_single_delete(state, repository, node_id, path, sink).await?; - return Ok(true); + return Ok(SubtreeDeleteOutcome::Materialized); } pending.push((node_id, path.clone())); @@ -5546,10 +5593,13 @@ async fn emit_filesystem_subtree_deletes( let mut children = StateNodeChildrenWithNameIterator::new(state.clone(), repository.clone(), node_id).await?; - let mut had_child = false; + // A surviving child is one that stays in the staged tree: filter-excluded + // (never visited) or visited but not discarded. Only a directory with no + // surviving children may itself be discarded or reported as an empty- + // directory deletion. + let mut any_surviving_child = false; let mut any_materialized = false; while let Some((child_id, child_node, child_name)) = children.next().await? { - had_child = true; let child_path = path.push_into_buf(&child_name).freeze(); // Release the block read lock before recursing (see NodeNameLock docs). drop(child_name); @@ -5557,9 +5607,22 @@ async fn emit_filesystem_subtree_deletes( .filter .excludes(&child_path, child_node.is_directory(), filter_mode) { + any_surviving_child = true; continue; } - if Box::pin(emit_filesystem_subtree_deletes( + let child_current_node = if in_current { + current_state + .find_subnode( + current_repository.clone(), + current_node, + child_node.name_hash, + ) + .await + .unwrap_or(INVALID_NODE) + } else { + INVALID_NODE + }; + let outcome = Box::pin(emit_filesystem_subtree_deletes( state.clone(), repository.clone(), child_id, @@ -5569,29 +5632,51 @@ async fn emit_filesystem_subtree_deletes( scan_dirty, sink, pending, + current_state, + current_repository, + child_current_node, + pending_discards, )) - .await? - { + .await?; + if outcome == SubtreeDeleteOutcome::Materialized { any_materialized = true; } + if outcome != SubtreeDeleteOutcome::Discarded { + any_surviving_child = true; + } } if any_materialized { // The first materializing descendant already flushed this directory. - return Ok(true); + return Ok(SubtreeDeleteOutcome::Materialized); } - if !had_child && !repository.filter.excludes(path, true, filter_mode) { - // Empty in-view directory: clone/checkout writes it, so its absence is a - // real deletion. It is the materializing leaf here, and its own buffered - // entry (pushed above) is flushed and marked along with its ancestors. - flush_pending_dir_deletes(&state, &repository, sink, pending, scan_dirty).await?; - return Ok(true); + if !any_surviving_child { + // No children at all, or every child was discarded as a reverted add. + if revertible { + // The directory itself was never committed either: discard it after + // its children instead of reporting it deleted. + pending.truncate(depth - 1); + lore_trace!( + "Queueing reverted-add directory {} for discard (no directory at {}, not in current)", + node_id, + path + ); + pending_discards.push(node_id); + return Ok(SubtreeDeleteOutcome::Discarded); + } + if !repository.filter.excludes(path, true, filter_mode) { + // Empty in-view directory: clone/checkout writes it, so its absence is a + // real deletion. It is the materializing leaf here, and its own buffered + // entry (pushed above) is flushed and marked along with its ancestors. + flush_pending_dir_deletes(&state, &repository, sink, pending, scan_dirty).await?; + return Ok(SubtreeDeleteOutcome::Materialized); + } } // Nothing under this directory materialized: drop its buffered entry. pending.truncate(depth - 1); - Ok(false) + Ok(SubtreeDeleteOutcome::Dropped) } #[allow(clippy::too_many_arguments)] @@ -5925,9 +6010,18 @@ async fn diff_filesystem_directory_walk( continue; }; + let current_counterpart = current_node_list + .children + .as_slice() + .binary_search_by(|child| child.name.cmp(&from_named_node.name)) + .map(|index| current_node_list.children[index].node) + .unwrap_or(INVALID_NODE); + let in_current = current_counterpart.is_valid_node_id(); + // 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). + // wrote to disk, and discarding reverted unstaged adds instead of + // reporting them deleted (see emit_filesystem_subtree_deletes). if from_node.node.is_directory() { let mut pending = Vec::new(); emit_filesystem_subtree_deletes( @@ -5940,6 +6034,10 @@ async fn diff_filesystem_directory_walk( ctx.scan_dirty, &mut ChangeSink::Vec(&mut *changes), &mut pending, + &ctx.current.state, + &ctx.current.repository, + current_counterpart, + pending_discards, ) .await?; continue; @@ -5950,11 +6048,6 @@ async fn diff_filesystem_directory_walk( // 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. - let in_current = current_node_list - .children - .as_slice() - .binary_search_by(|child| child.name.cmp(&from_named_node.name)) - .is_ok(); if ctx.scan_dirty && from_node.node.is_file() && !in_current { lore_trace!( "Queueing reverted-DirtyAdd node {} (no file at {}, not in current)", @@ -6318,6 +6411,7 @@ async fn diff_filesystem_single_file( /// Everything in state under this path is considered deleted. async fn diff_filesystem_missing( from: FilesystemTraversal, + current: FilesystemTraversal, node_path: RelativePath, filter_mode: FilterMode, scan_dirty: bool, @@ -6332,6 +6426,30 @@ async fn diff_filesystem_missing( .node(from.repository.clone(), from.root_node) .await?; + // A scanned path that exists neither on disk nor in the current + // revision is a reverted unstaged add: drop its whole subtree from the + // staged tree instead of reporting a phantom delete for content no + // revision ever had. A staged descendant blocks the discard and falls + // back to the regular delete report. + if scan_dirty + && !current.root_node.is_valid_node_id() + && !from_node.is_staged() + && let Some(discards) = collect_revertible_subtree( + from.state.clone(), + from.repository.clone(), + from.root_node, + ) + .await? + { + lore_trace!( + "Filesystem path {} does not exist and is not in the current revision, discarding {} reverted-add node(s)", + node_path, + discards.len() + ); + apply_pending_discards(from.state.clone(), from.repository.clone(), discards).await?; + return Ok((changes, stats)); + } + lore_trace!( "Filesystem path {} does not exist, marking state node {} as deleted", node_path, @@ -6377,6 +6495,39 @@ async fn diff_filesystem_missing( Ok((changes, stats)) } +/// Collect `node_id`'s subtree in post-order (children before their +/// directory) for a reverted-add discard. Returns `None` when the subtree +/// contains a staged node — staged content must survive the revert, so the +/// caller falls back to the regular delete report. +#[allow(clippy::type_complexity)] +fn collect_revertible_subtree( + state: Arc, + repository: Arc, + node_id: NodeID, +) -> Pin>, StateError>> + Send>> { + Box::pin(async move { + let node = state.node(repository.clone(), node_id).await?; + if node.is_staged() { + return Ok(None); + } + + let mut discards = Vec::new(); + if node.is_directory() { + let children = state.node_children(repository.clone(), node_id).await?; + for child_id in children { + match collect_revertible_subtree(state.clone(), repository.clone(), child_id) + .await? + { + Some(child_discards) => discards.extend(child_discards), + None => return Ok(None), + } + } + } + discards.push(node_id); + Ok(Some(discards)) + }) +} + #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] fn diff_filesystem_subtree_recurse( diff --git a/lore/tests/scan_reverted_add.rs b/lore/tests/scan_reverted_add.rs new file mode 100644 index 00000000..6cc963fb --- /dev/null +++ b/lore/tests/scan_reverted_add.rs @@ -0,0 +1,310 @@ +// SPDX-FileCopyrightText: 2026 Epic Games, Inc. +// SPDX-License-Identifier: MIT +//! Scan reconciliation for reverted unstaged adds. +//! +//! Deleting a never-committed file or folder from disk must remove its +//! dirty-add tracking entirely: a follow-up `status --scan` must not report +//! the vanished paths at all. Only content that exists in the current +//! revision may be reported as `Delete`. +mod test_util; + +#[cfg(test)] +mod tests { + use std::io::Write; + use std::str::FromStr; + use std::sync::Arc; + + use lore::file::LoreFileStageArgs; + use lore::repository::LoreRepositoryCreateArgs; + use lore::repository::LoreRepositoryStatusArgs; + use lore::revision::LoreRevisionCommitArgs; + use lore_revision::interface::LoreArray; + use lore_revision::interface::LoreEvent; + use lore_revision::interface::LoreFileAction; + use lore_revision::interface::LoreGlobalArgs; + use lore_revision::interface::LoreString; + use parking_lot::Mutex; + use rand::distr::Alphanumeric; + use rand::distr::SampleString; + + use super::test_util::TempDir; + + fn offline_globals(repository_path: &std::path::Path) -> LoreGlobalArgs { + LoreGlobalArgs { + repository_path: repository_path.into(), + offline: 1, + identity: "test-user".into(), + ..Default::default() + } + } + + async fn create_repository(globals: &LoreGlobalArgs) { + let name: String = Alphanumeric.sample_string(&mut rand::rng(), 16); + let mut url = String::from_str("lore://localhost/").unwrap_or_default(); + url.push_str(name.as_str()); + let args = LoreRepositoryCreateArgs { + repository_url: url.into(), + id: LoreString::default(), + description: LoreString::default(), + use_shared_store: 0, + shared_store_path: LoreString::default(), + }; + let result = lore::repository::create(globals.clone(), args, None).await; + assert_eq!(result, 0, "Failed to create repository"); + } + + fn write_file(path: &std::path::Path, content: &[u8]) { + std::fs::create_dir_all(path.parent().expect("file has a parent")) + .expect("Failed to create parent directory"); + let mut file = std::fs::File::options() + .create(true) + .truncate(true) + .write(true) + .open(path) + .expect("Failed to create test file"); + file.write_all(content).expect("Failed to write test file"); + } + + async fn stage_and_commit( + globals: &LoreGlobalArgs, + paths: Vec, + message: &str, + ) { + let args = LoreFileStageArgs { + paths: LoreArray::from_vec(paths), + case_change: 0, + scan: 1, + }; + let result = lore::file::stage(globals.clone(), args, None).await; + assert_eq!(result, 0, "Failed to stage files"); + + let args = LoreRevisionCommitArgs { + message: LoreString::from(message), + ..Default::default() + }; + let result = lore::revision::commit(globals.clone(), args, None).await; + assert_eq!(result, 0, "Failed to commit"); + } + + /// Run `status --scan` and collect the reported (path, action) pairs with + /// paths normalized to forward slashes. + async fn scan_status(globals: &LoreGlobalArgs) -> Vec<(String, LoreFileAction)> { + let entries: Arc>> = Arc::new(Mutex::new(Vec::new())); + let entries_ = Arc::clone(&entries); + let status_ok: Arc> = Arc::new(Mutex::new(false)); + let status_ok_ = Arc::clone(&status_ok); + + let callback = Some(Box::new(move |event: &LoreEvent| match event { + LoreEvent::RepositoryStatusFile(data) => { + entries_ + .lock() + .push((data.path.as_str().replace('\\', "/"), data.action)); + } + LoreEvent::Complete(data) => { + *status_ok_.lock() = data.status == 0; + } + LoreEvent::Error(data) => { + eprintln!("Error {}: {}", data.error_type, data.error_inner.as_str()); + } + _ => (), + }) as Box<_>); + + let args = LoreRepositoryStatusArgs { + staged: 1, + scan: 1, + check_dirty: 0, + reset: 0, + sync_point: 0, + revision_only: 0, + count: 0, + paths: LoreArray::default(), + }; + let result = lore::repository::status(globals.clone(), args, callback).await; + assert_eq!(result, 0, "Status call failed"); + assert!(*status_ok.lock(), "Status did not complete successfully"); + + let collected = entries.lock().clone(); + collected + } + + fn entries_under<'a>( + entries: &'a [(String, LoreFileAction)], + prefix: &str, + ) -> Vec<&'a (String, LoreFileAction)> { + entries + .iter() + .filter(|(path, _)| { + let trimmed = path.trim_end_matches('/'); + trimmed == prefix || path.starts_with(&format!("{prefix}/")) + }) + .collect() + } + + /// Deleting a never-committed folder must remove every trace of it from + /// the scan: no `Delete` entries for paths that were never part of a + /// revision, on this scan or any later one. + #[tokio::test] + async fn scan_forgets_deleted_untracked_folder() { + let tempdir = TempDir::new("lore-scan-revert-test-"); + let repository_path = tempdir.path().to_path_buf(); + let globals = offline_globals(&repository_path); + + create_repository(&globals).await; + + // A committed baseline so the repository has a non-empty revision. + let base = repository_path.join("base.txt"); + write_file(&base, b"base"); + stage_and_commit(&globals, vec![LoreString::from(&base)], "base").await; + + // Untracked folder with nested content, registered by a scan. + write_file(&repository_path.join("newfolder/one.png"), b"one"); + write_file(&repository_path.join("newfolder/two.png"), b"two"); + write_file(&repository_path.join("newfolder/sub/three.png"), b"three"); + + let entries = scan_status(&globals).await; + let added = entries_under(&entries, "newfolder"); + assert!( + added + .iter() + .any(|(path, action)| path.ends_with("one.png") && *action == LoreFileAction::Add), + "expected newfolder/one.png reported as Add, got: {entries:?}" + ); + + // Revert the add by deleting the folder from disk. + std::fs::remove_dir_all(repository_path.join("newfolder")) + .expect("Failed to remove newfolder"); + + let entries = scan_status(&globals).await; + assert!( + entries_under(&entries, "newfolder").is_empty(), + "vanished never-committed folder must not be reported, got: {entries:?}" + ); + + // The staged state must be clean of the folder too: a second scan must + // not resurrect phantom entries from leftover nodes or dirty flags. + let entries = scan_status(&globals).await; + assert!( + entries_under(&entries, "newfolder").is_empty(), + "phantom entries resurfaced on a later scan: {entries:?}" + ); + } + + /// Deleting a never-committed EMPTY folder must leave no trace either — + /// whether or not the scan registered a node for it, later scans must not + /// report the path. + #[tokio::test] + async fn scan_forgets_deleted_untracked_empty_folder() { + let tempdir = TempDir::new("lore-scan-revert-test-"); + let repository_path = tempdir.path().to_path_buf(); + let globals = offline_globals(&repository_path); + + create_repository(&globals).await; + + let base = repository_path.join("base.txt"); + write_file(&base, b"base"); + stage_and_commit(&globals, vec![LoreString::from(&base)], "base").await; + + std::fs::create_dir_all(repository_path.join("emptyfolder")) + .expect("Failed to create emptyfolder"); + scan_status(&globals).await; + + std::fs::remove_dir_all(repository_path.join("emptyfolder")) + .expect("Failed to remove emptyfolder"); + + let entries = scan_status(&globals).await; + assert!( + entries_under(&entries, "emptyfolder").is_empty(), + "vanished never-committed empty folder must not be reported, got: {entries:?}" + ); + let entries = scan_status(&globals).await; + assert!( + entries_under(&entries, "emptyfolder").is_empty(), + "phantom empty folder resurfaced on a later scan: {entries:?}" + ); + } + + /// Deleting a committed folder must still report `Delete` for its content. + #[tokio::test] + async fn scan_reports_deletes_for_committed_folder() { + let tempdir = TempDir::new("lore-scan-revert-test-"); + let repository_path = tempdir.path().to_path_buf(); + let globals = offline_globals(&repository_path); + + create_repository(&globals).await; + + let file_a = repository_path.join("folder/a.txt"); + let file_b = repository_path.join("folder/b.txt"); + write_file(&file_a, b"a"); + write_file(&file_b, b"b"); + stage_and_commit( + &globals, + vec![LoreString::from(&file_a), LoreString::from(&file_b)], + "add folder", + ) + .await; + + std::fs::remove_dir_all(repository_path.join("folder")).expect("Failed to remove folder"); + + let entries = scan_status(&globals).await; + for file in ["folder/a.txt", "folder/b.txt"] { + assert!( + entries + .iter() + .any(|(path, action)| path == file && *action == LoreFileAction::Delete), + "expected {file} reported as Delete, got: {entries:?}" + ); + } + } + + /// Deleting a committed folder that also contains never-committed files + /// must report `Delete` only for the committed content. + #[tokio::test] + async fn scan_mixed_folder_reports_only_committed_deletes() { + let tempdir = TempDir::new("lore-scan-revert-test-"); + let repository_path = tempdir.path().to_path_buf(); + let globals = offline_globals(&repository_path); + + create_repository(&globals).await; + + let committed = repository_path.join("folder/committed.txt"); + write_file(&committed, b"committed"); + stage_and_commit(&globals, vec![LoreString::from(&committed)], "add folder").await; + + // Drop an untracked file into the committed folder and register it. + write_file(&repository_path.join("folder/untracked.txt"), b"untracked"); + let entries = scan_status(&globals).await; + assert!( + entries + .iter() + .any(|(path, action)| path == "folder/untracked.txt" + && *action == LoreFileAction::Add), + "expected folder/untracked.txt reported as Add, got: {entries:?}" + ); + + std::fs::remove_dir_all(repository_path.join("folder")).expect("Failed to remove folder"); + + let entries = scan_status(&globals).await; + assert!( + entries + .iter() + .any(|(path, action)| path == "folder/committed.txt" + && *action == LoreFileAction::Delete), + "expected folder/committed.txt reported as Delete, got: {entries:?}" + ); + assert!( + !entries + .iter() + .any(|(path, _)| path == "folder/untracked.txt"), + "vanished never-committed file must not be reported, got: {entries:?}" + ); + + // Later scans stay clean as well. + let entries = scan_status(&globals).await; + assert!( + !entries + .iter() + .any(|(path, _)| path == "folder/untracked.txt"), + "phantom entry resurfaced on a later scan: {entries:?}" + ); + } +}