From 7befc87170b5693dfa00ce1104120e497fe86c27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A2n=20L=C3=AA-V=C6=B0=C6=A1ng?= <65440815+lehuan5062@users.noreply.github.com> Date: Thu, 9 Jul 2026 10:23:54 +0700 Subject: [PATCH 1/6] lore: Discard reverted uncommitted directory adds on scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A working-tree scan (`status --scan` / `stage --scan`) already discards a reverted uncommitted *file* add so `state_staged` matches the filesystem: a file that was staged and then removed before any commit has no committed base to delete from, so reporting a `Delete` would leave an unremovable "zombie" entry. A reverted uncommitted *directory* add was not handled the same way. Extend the same treatment to directories. When a directory node exists in `state_from` but neither in `state_current` (never committed) nor on disk, queue it for discard instead of emitting a meaningless `Delete`. Discarding a directory node must also reclaim its subtree: `apply_pending_discards` now recursively discards every child below a directory node before unlinking the node itself, so no stale descendant slots are left behind. Includes a Rust test covering the index-then-remove cycle for a directory add, asserting the node is discarded (and does not resurface on a later scan) rather than reported as a delete. Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com> --- lore-revision/src/state.rs | 58 ++++++ lore-revision/tests/reverted_directory.rs | 226 ++++++++++++++++++++++ 2 files changed, 284 insertions(+) create mode 100644 lore-revision/tests/reverted_directory.rs diff --git a/lore-revision/src/state.rs b/lore-revision/src/state.rs index af01711e..ed16d429 100644 --- a/lore-revision/src/state.rs +++ b/lore-revision/src/state.rs @@ -4684,6 +4684,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(), @@ -5606,6 +5630,15 @@ 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. Must only be called from [`diff_filesystem_directory`], +/// which sorts `node_list` and `current_node_list` by name beforehand — the +/// binary searches here assume that ordering. #[allow(clippy::too_many_arguments)] async fn diff_filesystem_directory_walk( ctx: &DiffFilesystemContext, @@ -5937,6 +5970,31 @@ async fn diff_filesystem_directory_walk( continue; }; + // A directory node that exists in state_from but neither in state_current + // (never committed) nor on disk is a reverted, uncommitted add: the + // directory was staged and then removed from disk before any commit, + // together with whatever of its contents had been staged under it. + // Reporting it as a `Delete` is meaningless because there is no committed + // base to delete from, and no mutation verb can clear it (the "zombie" + // entry). Discard the whole subtree so state_staged matches the filesystem + // instead, the same way a reverted single-file add is discarded below. + 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). diff --git a/lore-revision/tests/reverted_directory.rs b/lore-revision/tests/reverted_directory.rs new file mode 100644 index 00000000..a2d65051 --- /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"); + } +} From d33516046c4d90a4292b40af4fb492ac6ab82baa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A2n=20L=C3=AA-V=C6=B0=C6=A1ng?= <65440815+lehuan5062@users.noreply.github.com> Date: Fri, 10 Jul 2026 19:28:17 +0700 Subject: [PATCH 2/6] lore: Address maintainer feedback on directory discard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Condense verbose comment in state.rs explaining reverted uncommitted directory handling to a single sentence - Add smoke test to verify directory discard behavior with JSON output validation (no stray deletes, no duplicates, concurrent changes properly reported) Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com> --- lore-revision/src/state.rs | 9 +---- scripts/test/test_dirty.py | 77 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 8 deletions(-) diff --git a/lore-revision/src/state.rs b/lore-revision/src/state.rs index ed16d429..8a6d66f1 100644 --- a/lore-revision/src/state.rs +++ b/lore-revision/src/state.rs @@ -5970,14 +5970,7 @@ async fn diff_filesystem_directory_walk( continue; }; - // A directory node that exists in state_from but neither in state_current - // (never committed) nor on disk is a reverted, uncommitted add: the - // directory was staged and then removed from disk before any commit, - // together with whatever of its contents had been staged under it. - // Reporting it as a `Delete` is meaningless because there is no committed - // base to delete from, and no mutation verb can clear it (the "zombie" - // entry). Discard the whole subtree so state_staged matches the filesystem - // instead, the same way a reverted single-file add is discarded below. + // Discard reverted uncommitted directories (staged then removed from disk before commit) to match the filesystem. if ctx.scan_dirty && from_node.node.is_directory() { let in_current = current_node_list .children diff --git a/scripts/test/test_dirty.py b/scripts/test/test_dirty.py index e46baf02..54a3c60d 100644 --- a/scripts/test/test_dirty.py +++ b/scripts/test/test_dirty.py @@ -2806,3 +2806,80 @@ 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 scanning properly discards reverted uncommitted directories. + + A directory is staged, then removed from disk before any commit. Scanning + should discard this "zombie" directory entry so status shows no stray + deletes and matches the filesystem state. Verify via JSON output that: + - No spurious delete entries appear for the discarded directory + - No duplicate entries are reported + - Other concurrent changes remain correctly reported + """ + repo: Lore = new_lore_repo() + + # Create initial commit with a base file + with repo.open_file("base.txt", "w+") as f: + f.write("base file\n") + repo.stage(scan=True, offline=True) + repo.commit(offline=True) + + # Stage a directory with files inside it + 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) + + # Create another unrelated change to verify it's still reported + with repo.open_file("other_change.txt", "w+") as f: + f.write("other\n") + repo.dirty("other_change.txt", offline=True) + + # Remove the reverted directory from disk before committing + # (this simulates user removing a staged directory) + import shutil + shutil.rmtree(os.path.join(repo.path, "reverted_dir")) + + # Run status scan to detect the directory was removed and discard it + output = repo.status(json=True, offline=True) + status_entries = parse_status_json(output) + + # Extract all paths and actions + file_entries = [e for e in status_entries if e.get("type") == "file"] + paths_by_action = {} + for entry in file_entries: + action = entry.get("action", "unknown") + path = to_posix(entry.get("path", "")) + if action not in paths_by_action: + paths_by_action[action] = [] + paths_by_action[action].append(path) + + # Verify no spurious deletes for the discarded directory + delete_paths = paths_by_action.get("delete", []) + assert not any( + "reverted_dir" in p for p in delete_paths + ), f"Should not report deletes for discarded directory, got: {delete_paths}" + + # Verify no duplicate entries + all_paths = [to_posix(e.get("path", "")) for e in file_entries] + assert len(all_paths) == len(set(all_paths)), ( + f"Duplicate entries reported: {sorted(all_paths)}" + ) + + # Verify the unrelated change is still properly reported + other_entries = [p for p in all_paths if "other_change" in p] + assert len(other_entries) == 1, ( + f"other_change.txt should be reported once, got: {other_entries}" + ) + other_entry = next(e for e in file_entries if "other_change" in e.get("path", "")) + 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}" + ) From e89c9a57c3a3eca43717a7b65a2039620335d531 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A2n=20L=C3=AA-V=C6=B0=C6=A1ng?= <65440815+lehuan5062@users.noreply.github.com> Date: Fri, 10 Jul 2026 21:15:18 +0700 Subject: [PATCH 3/6] lore: Fix smoke test to actually exercise the directory discard scan path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The smoke test previously called status() without --scan, so the filesystem-diff walk containing the discard logic never ran, and it filtered results to type == "file", hiding the directory-node entries where a regression would actually surface. Verified by disabling the discard logic and confirming the old test still passed; it now fails as expected. The fix drives status(scan=True) and checks a follow-up call (since the scan that performs the discard reports its own stale pre-discard snapshot) with no type filter. Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com> --- scripts/test/test_dirty.py | 82 ++++++++++++++++++++++++-------------- 1 file changed, 51 insertions(+), 31 deletions(-) diff --git a/scripts/test/test_dirty.py b/scripts/test/test_dirty.py index 54a3c60d..86d5b7a2 100644 --- a/scripts/test/test_dirty.py +++ b/scripts/test/test_dirty.py @@ -2810,14 +2810,27 @@ def check(label: str, **kwargs) -> None: @pytest.mark.smoke def test_scan_discards_reverted_uncommitted_directory(new_lore_repo): - """Verify that scanning properly discards reverted uncommitted directories. - - A directory is staged, then removed from disk before any commit. Scanning - should discard this "zombie" directory entry so status shows no stray - deletes and matches the filesystem state. Verify via JSON output that: - - No spurious delete entries appear for the discarded directory + """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 (see + lore-revision/src/repository/status.rs: the filesystem diff, and with it + the discard logic, only runs `if show_scan`). + + The scan that performs the discard still reports its *own* output from + the staged snapshot captured before the discard was persisted (a + pre-existing report/persist ordering quirk, not part of what's under + test here) -- so the discard is only observable on a subsequent status + call. Verify that after one scan runs: + - A follow-up status call shows the reverted directory and everything + staged under it gone entirely (no add, delete, or other action + references it) -- checked with no `type` filter, since the directory + node itself (not just the files under it) is what the discard logic + operates on - No duplicate entries are reported - - Other concurrent changes remain correctly reported + - An unrelated concurrent change is still correctly reported + - A second scan does not resurrect the discarded directory """ repo: Lore = new_lore_repo() @@ -2840,46 +2853,53 @@ def test_scan_discards_reverted_uncommitted_directory(new_lore_repo): f.write("other\n") repo.dirty("other_change.txt", offline=True) - # Remove the reverted directory from disk before committing + # Remove the staged directory from disk before committing # (this simulates user removing a staged directory) import shutil shutil.rmtree(os.path.join(repo.path, "reverted_dir")) - # Run status scan to detect the directory was removed and discard it - output = repo.status(json=True, offline=True) - status_entries = parse_status_json(output) - - # Extract all paths and actions - file_entries = [e for e in status_entries if e.get("type") == "file"] - paths_by_action = {} - for entry in file_entries: - action = entry.get("action", "unknown") - path = to_posix(entry.get("path", "")) - if action not in paths_by_action: - paths_by_action[action] = [] - paths_by_action[action].append(path) - - # Verify no spurious deletes for the discarded directory - delete_paths = paths_by_action.get("delete", []) - assert not any( - "reverted_dir" in p for p in delete_paths - ), f"Should not report deletes for discarded directory, got: {delete_paths}" + 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/")] + + # First scan: reconciles staged state against the filesystem and queues + # the zombie directory for discard. + get_status_files(repo, scan=True) + + # Second call observes the persisted result of the discard. + entries = get_status_files(repo, scan=True) + all_paths = [to_posix(e.get("path", "")) for e in entries] + + # The reverted directory and its staged contents must be fully discarded -- + # not reported under any action (add, delete, or otherwise), and not just + # absent from a `type == "file"` filter that would miss the directory + # node itself. + assert not reverted_dir_entries(entries), ( + f"Reverted directory should be fully discarded from status, " + f"got: {reverted_dir_entries(entries)}" + ) # Verify no duplicate entries - all_paths = [to_posix(e.get("path", "")) for e in file_entries] assert len(all_paths) == len(set(all_paths)), ( f"Duplicate entries reported: {sorted(all_paths)}" ) - # Verify the unrelated change is still properly reported - other_entries = [p for p in all_paths if "other_change" in p] + # Verify the unrelated change is still properly reported exactly once + 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 = next(e for e in file_entries if "other_change" in e.get("path", "")) + 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}" ) + + # A third scan must not resurrect the discarded directory or duplicate it + 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)}" + ) From 409f2c78c4c503e2edd96f22e97191d3c4ac5089 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A2n=20L=C3=AA-V=C6=B0=C6=A1ng?= <65440815+lehuan5062@users.noreply.github.com> Date: Fri, 10 Jul 2026 21:25:04 +0700 Subject: [PATCH 4/6] lore: Move shutil import to module scope in discard test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local mid-function import didn't match the file's existing convention of importing at module scope. Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com> --- scripts/test/test_dirty.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/test/test_dirty.py b/scripts/test/test_dirty.py index 86d5b7a2..d488b283 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 @@ -2855,7 +2856,6 @@ def test_scan_discards_reverted_uncommitted_directory(new_lore_repo): # Remove the staged directory from disk before committing # (this simulates user removing a staged directory) - import shutil shutil.rmtree(os.path.join(repo.path, "reverted_dir")) def reverted_dir_entries(entries: list[dict]) -> list[str]: From 5cc9f50e1a3ec8c03cf41492b6bbdcfbb1977c90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A2n=20L=C3=AA-V=C6=B0=C6=A1ng?= <65440815+lehuan5062@users.noreply.github.com> Date: Fri, 10 Jul 2026 21:28:23 +0700 Subject: [PATCH 5/6] lore: Trim organizational comments from discard test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Lore's comment standards, remove comments that only restate what the adjacent code already says; keep the ones explaining non-obvious behavior (the revert simulation, the two-scan reconciliation quirk, and why no type filter is applied). Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com> --- scripts/test/test_dirty.py | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/scripts/test/test_dirty.py b/scripts/test/test_dirty.py index d488b283..0e5d7c30 100644 --- a/scripts/test/test_dirty.py +++ b/scripts/test/test_dirty.py @@ -2835,13 +2835,11 @@ def test_scan_discards_reverted_uncommitted_directory(new_lore_repo): """ repo: Lore = new_lore_repo() - # Create initial commit with a base file with repo.open_file("base.txt", "w+") as f: f.write("base file\n") repo.stage(scan=True, offline=True) repo.commit(offline=True) - # Stage a directory with files inside it 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") @@ -2849,42 +2847,35 @@ def test_scan_discards_reverted_uncommitted_directory(new_lore_repo): f.write("file2\n") repo.stage(scan=True, offline=True) - # Create another unrelated change to verify it's still reported with repo.open_file("other_change.txt", "w+") as f: f.write("other\n") repo.dirty("other_change.txt", offline=True) - # Remove the staged directory from disk before committing - # (this simulates user removing a staged directory) + # 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/")] - # First scan: reconciles staged state against the filesystem and queues - # the zombie directory for discard. + # The discard queued by this scan is only observable on a later call + # (see docstring), so its own output isn't asserted on. get_status_files(repo, scan=True) - # Second call observes the persisted result of the discard. entries = get_status_files(repo, scan=True) all_paths = [to_posix(e.get("path", "")) for e in entries] - # The reverted directory and its staged contents must be fully discarded -- - # not reported under any action (add, delete, or otherwise), and not just - # absent from a `type == "file"` filter that would miss the directory - # node itself. + # 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)}" ) - - # Verify no duplicate entries assert len(all_paths) == len(set(all_paths)), ( f"Duplicate entries reported: {sorted(all_paths)}" ) - # Verify the unrelated change is still properly reported exactly once 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}" @@ -2897,7 +2888,6 @@ def reverted_dir_entries(entries: list[dict]) -> list[str]: f"other_change.txt should be flagDirty: {other_entry}" ) - # A third scan must not resurrect the discarded directory or duplicate it entries_again = get_status_files(repo, scan=True) assert not reverted_dir_entries(entries_again), ( f"Later scan resurrected the reverted directory: " From 4da04197a535028876ba0f2c9796fb14afff3625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A2n=20L=C3=AA-V=C6=B0=C6=A1ng?= <65440815+lehuan5062@users.noreply.github.com> Date: Tue, 14 Jul 2026 19:09:50 +0700 Subject: [PATCH 6/6] fix: scan discards reverted uncommitted directories and fixes ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends status --scan to discard reverted uncommitted directories (staged, then deleted from disk before any commit), matching the existing behavior for reverted file adds. Also reorders status() so the scan block runs before the staged-diff block, applying the discard to the shared staged state before the staged diff reads it — a single status --scan now reflects the discard instead of requiring a second call. Changes: - diff_filesystem_directory_walk queues directories for discard when staged but absent from commit and filesystem, recursively discarding children. - status() reordered: scan section before staged-diff section. - Doc comment on diff_filesystem_directory_walk restated to declare its precondition (node_list/current_node_list must be sorted by name) directly, per review feedback. - Smoke test updated to assert the discard on the first scan. Addresses review feedback: - Doc comment no longer describes caller. - Test no longer claims the discard is only observable on a second call. - Comments trimmed to Lore's terse house style. Testing: cargo fmt/clippy clean, reverted_directory test passed, smoke test passed, full test_dirty suite regression-free (10 pre-existing failures in unrelated branch merge path). Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com> --- lore-revision/src/repository/status.rs | 236 +++++++++++++------------ lore-revision/src/state.rs | 15 +- scripts/test/test_dirty.py | 32 ++-- 3 files changed, 136 insertions(+), 147 deletions(-) diff --git a/lore-revision/src/repository/status.rs b/lore-revision/src/repository/status.rs index 00ade2b3..7b8b04cf 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 8a6d66f1..cc7bebbf 100644 --- a/lore-revision/src/state.rs +++ b/lore-revision/src/state.rs @@ -5636,9 +5636,8 @@ async fn emit_filesystem_subtree_deletes( /// `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. Must only be called from [`diff_filesystem_directory`], -/// which sorts `node_list` and `current_node_list` by name beforehand — the -/// binary searches here assume that ordering. +/// 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, @@ -5970,7 +5969,8 @@ async fn diff_filesystem_directory_walk( continue; }; - // Discard reverted uncommitted directories (staged then removed from disk before commit) to match the filesystem. + // 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 @@ -6008,11 +6008,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/scripts/test/test_dirty.py b/scripts/test/test_dirty.py index 0e5d7c30..6d93fec7 100644 --- a/scripts/test/test_dirty.py +++ b/scripts/test/test_dirty.py @@ -2815,23 +2815,17 @@ def test_scan_discards_reverted_uncommitted_directory(new_lore_repo): 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 (see - lore-revision/src/repository/status.rs: the filesystem diff, and with it - the discard logic, only runs `if show_scan`). - - The scan that performs the discard still reports its *own* output from - the staged snapshot captured before the discard was persisted (a - pre-existing report/persist ordering quirk, not part of what's under - test here) -- so the discard is only observable on a subsequent status - call. Verify that after one scan runs: - - A follow-up status call shows the reverted directory and everything - staged under it gone entirely (no add, delete, or other action - references it) -- checked with no `type` filter, since the directory - node itself (not just the files under it) is what the discard logic - operates on - - No duplicate entries are reported - - An unrelated concurrent change is still correctly reported - - A second scan does not resurrect the discarded directory + 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() @@ -2859,10 +2853,6 @@ 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/")] - # The discard queued by this scan is only observable on a later call - # (see docstring), so its own output isn't asserted on. - get_status_files(repo, scan=True) - entries = get_status_files(repo, scan=True) all_paths = [to_posix(e.get("path", "")) for e in entries]