diff --git a/Makefile b/Makefile index e29cda33..1c642a24 100644 --- a/Makefile +++ b/Makefile @@ -296,8 +296,13 @@ format: format-rust format-lua format-ts lint-rust: cargo clippy --workspace --no-default-features --features zlob -- -D warnings +# Prefer luacheck from PATH (what CI uses). The luarocks shim is a fallback: +# it hardcodes the lua binary it was generated against and breaks whenever +# the interpreter is upgraded. +LUACHECK ?= $(shell command -v luacheck 2>/dev/null || echo ~/.luarocks/bin/luacheck) + lint-lua: - ~/.luarocks/bin/luacheck . + $(LUACHECK) . lint-ts: bun lint diff --git a/crates/fff-c/include/fff.h b/crates/fff-c/include/fff.h index 83df6c38..4df778cd 100644 --- a/crates/fff-c/include/fff.h +++ b/crates/fff-c/include/fff.h @@ -414,7 +414,12 @@ typedef struct FffMixedSearchResult { /** * A single watch event. `kind`: 0 = created, 1 = modified, 2 = removed, - * 3 = rescan (events were lost; re-stat what you care about). + * 3 = rescan (events were lost; re-stat what you care about), + * 4 = renamed (`path` is the destination; read the source with + * `fff_watch_events_get_from_path`). + * + * Layout is frozen: this is an array element, so growing it would change the + * stride every existing binding compiled against. */ typedef struct FffWatchEvent { /** @@ -426,10 +431,17 @@ typedef struct FffWatchEvent { /** * A batch of watch events. Free with `fff_free_watch_events`. + * Versioned by append only: existing field offsets never move. */ typedef struct FffWatchEventBatch { struct FffWatchEvent *events; uint32_t count; + /** + * Parallel to `events`, `count` long: the pre-rename path for entries + * with `kind == 4`, null for every other entry. Null when the batch holds + * no renames at all. + */ + char **rename_sources; } FffWatchEventBatch; /** @@ -1321,7 +1333,7 @@ struct FffResult *fff_watch_args(void *fff_handle, struct FffResult *fff_unwatch(void *fff_handle, uint64_t watch_id); /** - * Number of events in a batch; 0 if `batch` is null. + * Number of events in a batch, 0 if `batch` is null. * * ## Safety * `batch` must be a valid `FffWatchEventBatch` pointer or null. @@ -1337,10 +1349,22 @@ uint32_t fff_watch_events_count(const struct FffWatchEventBatch *batch); const char *fff_watch_events_get_path(const struct FffWatchEventBatch *batch, uint32_t index); /** - * Kind of event `index` (0 = created, 1 = modified, 2 = removed, 3 = rescan) + * Pre-rename path of event `index`, null unless its kind is 4 (renamed). + * Owned by the batch; do not free separately. + * + * ## Safety + * `batch` must be a valid `FffWatchEventBatch` pointer or null. + */ +const char *fff_watch_events_get_from_path(const struct FffWatchEventBatch *batch, uint32_t index); + +/** + * Kind of event `index` (0 = created, 1 = modified, 2 = removed, 3 = rescan, + * 4 = renamed). * 3 (rescan aka "re-stat something" kind) returned when OS based buffer * has been overflown and some events might be loss. Paths will contain a list of * directories that needs to be rescanned to ensure consistency. + * 4 reports a move: `path` is the destination and + * `fff_watch_events_get_from_path` yields the source. * * ## Safety * `batch` must be a valid `FffWatchEventBatch` pointer or null. diff --git a/crates/fff-c/src/watch.rs b/crates/fff-c/src/watch.rs index e4ec23e4..8c30e1bc 100644 --- a/crates/fff-c/src/watch.rs +++ b/crates/fff-c/src/watch.rs @@ -25,7 +25,12 @@ pub struct FffWatchOptions { } /// A single watch event. `kind`: 0 = created, 1 = modified, 2 = removed, -/// 3 = rescan (events were lost; re-stat what you care about). +/// 3 = rescan (events were lost; re-stat what you care about), +/// 4 = renamed (`path` is the destination; read the source with +/// `fff_watch_events_get_from_path`). +/// +/// Layout is frozen: this is an array element, so growing it would change the +/// stride every existing binding compiled against. #[repr(C)] pub struct FffWatchEvent { /// Absolute path (heap C string owned by the parent batch). @@ -34,10 +39,15 @@ pub struct FffWatchEvent { } /// A batch of watch events. Free with `fff_free_watch_events`. +/// Versioned by append only: existing field offsets never move. #[repr(C)] pub struct FffWatchEventBatch { pub events: *mut FffWatchEvent, pub count: u32, + /// Parallel to `events`, `count` long: the pre-rename path for entries + /// with `kind == 4`, null for every other entry. Null when the batch holds + /// no renames at all. + pub rename_sources: *mut *mut c_char, } /// Instance-wide callback invoked with `(watch_id, batch)` for every `fff_watch` @@ -49,29 +59,51 @@ fn batch_into_raw(events: &[WatchEvent]) -> *mut FffWatchEventBatch { let items: Vec = events .iter() .map(|ev| FffWatchEvent { - path: CString::new(ev.path.to_string_lossy().as_bytes()) - .unwrap_or_default() - .into_raw(), + path: path_into_raw(&ev.path), kind: ev.kind as u8, }) .collect(); let count = items.len() as u32; - let events_ptr = if items.is_empty() { - ptr::null_mut() + let events_ptr = leak_slice(items); + + let rename_sources = if events.iter().any(|ev| ev.from.is_some()) { + let sources: Vec<*mut c_char> = events + .iter() + .map(|ev| { + ev.from + .as_ref() + .map_or(ptr::null_mut(), |p| path_into_raw(p)) + }) + .collect(); + leak_slice(sources) } else { - let mut boxed = items.into_boxed_slice(); - let p = boxed.as_mut_ptr(); - std::mem::forget(boxed); - p + ptr::null_mut() }; Box::into_raw(Box::new(FffWatchEventBatch { events: events_ptr, count, + rename_sources, })) } +fn path_into_raw(path: &std::path::Path) -> *mut c_char { + CString::new(path.to_string_lossy().as_bytes()) + .unwrap_or_default() + .into_raw() +} + +fn leak_slice(items: Vec) -> *mut T { + if items.is_empty() { + return ptr::null_mut(); + } + let mut boxed = items.into_boxed_slice(); + let p = boxed.as_mut_ptr(); + std::mem::forget(boxed); + p +} + unsafe fn watch_options_from_ffi( opts: *const FffWatchOptions, ) -> Result { @@ -270,10 +302,33 @@ pub unsafe extern "C" fn fff_watch_events_get_path( } } -/// Kind of event `index` (0 = created, 1 = modified, 2 = removed, 3 = rescan) +/// Pre-rename path of event `index`, null unless its kind is 4 (renamed). +/// Owned by the batch; do not free separately. +/// +/// ## Safety +/// `batch` must be a valid `FffWatchEventBatch` pointer or null. +#[unsafe(no_mangle)] +pub unsafe extern "C" fn fff_watch_events_get_from_path( + batch: *const FffWatchEventBatch, + index: u32, +) -> *const c_char { + if batch.is_null() { + return ptr::null(); + } + let batch = unsafe { &*batch }; + if batch.rename_sources.is_null() || index >= batch.count { + return ptr::null(); + } + unsafe { *batch.rename_sources.add(index as usize) } +} + +/// Kind of event `index` (0 = created, 1 = modified, 2 = removed, 3 = rescan, +/// 4 = renamed). /// 3 (rescan aka "re-stat something" kind) returned when OS based buffer /// has been overflown and some events might be loss. Paths will contain a list of /// directories that needs to be rescanned to ensure consistency. +/// 4 reports a move: `path` is the destination and +/// `fff_watch_events_get_from_path` yields the source. /// /// ## Safety /// `batch` must be a valid `FffWatchEventBatch` pointer or null. @@ -313,15 +368,23 @@ pub unsafe extern "C" fn fff_free_watch_events(batch: *mut FffWatchEventBatch) { } unsafe { let batch = Box::from_raw(batch); + let count = batch.count as usize; if !batch.events.is_null() { - let events = - Vec::from_raw_parts(batch.events, batch.count as usize, batch.count as usize); + let events = Vec::from_raw_parts(batch.events, count, count); for ev in events { if !ev.path.is_null() { drop(CString::from_raw(ev.path)); } } } + if !batch.rename_sources.is_null() { + let sources = Vec::from_raw_parts(batch.rename_sources, count, count); + for source in sources { + if !source.is_null() { + drop(CString::from_raw(source)); + } + } + } } } @@ -344,8 +407,12 @@ mod layout_tests { assert_eq!(offset_of!(FffWatchEvent, path), 0); assert_eq!(offset_of!(FffWatchEvent, kind), 8); - assert_eq!(size_of::(), 16); assert_eq!(offset_of!(FffWatchEventBatch, events), 0); assert_eq!(offset_of!(FffWatchEventBatch, count), 8); + // Appended in the renamed-event release. The batch is a single + // library-allocated struct reached only through a pointer, so growing + // its tail leaves every previously published offset valid. + assert_eq!(size_of::(), 24); + assert_eq!(offset_of!(FffWatchEventBatch, rename_sources), 16); } } diff --git a/crates/fff-c/tests/smoke.c b/crates/fff-c/tests/smoke.c index 68eaa9a2..7124c182 100644 --- a/crates/fff-c/tests/smoke.c +++ b/crates/fff-c/tests/smoke.c @@ -27,6 +27,8 @@ static int watch_glob_hits = 0; static int watch_dir_hits = 0; static int watch_all_hits = 0; static int watch_ignored_leaks = 0; +static int watch_rename_hits = 0; +static int watch_rename_bad_source = 0; static uint64_t watch_glob_id = 0; static uint64_t watch_dir_id = 0; static uint64_t watch_all_id = 0; @@ -47,6 +49,16 @@ static void on_watch_batch(uint64_t watch_id, struct FffWatchEventBatch *batch, if (watch_id == watch_all_id && strstr(path, "hello.txt")) { watch_all_hits++; } + /* kind 4 = renamed: the source rides in the parallel array */ + if (watch_id == watch_all_id && fff_watch_events_get_kind(batch, i) == 4 && + strstr(path, "renamed.txt")) { + const char *from = fff_watch_events_get_from_path(batch, i); + if (from && strstr(from, "to_rename.txt")) { + watch_rename_hits++; + } else { + watch_rename_bad_source++; + } + } } fff_free_watch_events(batch); // need to clean dynamic array of events @@ -156,6 +168,26 @@ static int watch_smoke(void) { usleep(100 * 1000); } + /* a rename must arrive as one kind-4 event carrying both paths */ + char rename_src[512]; + char rename_dst[512]; + snprintf(rename_src, sizeof(rename_src), "%s/to_rename.txt", dir); + snprintf(rename_dst, sizeof(rename_dst), "%s/renamed.txt", dir); + FILE *rf = fopen(rename_src, "w"); + if (rf) { + fputs("move me\n", rf); + fclose(rf); + } + usleep(500 * 1000); /* let the create land before the move */ + if (rename(rename_src, rename_dst) != 0) { + fprintf(stderr, "watch_smoke: rename failed\n"); + fff_destroy(picker); + return 1; + } + for (int attempt = 0; attempt < 100 && watch_rename_hits == 0; attempt++) { + usleep(100 * 1000); + } + r = fff_unwatch(picker, watch_glob_id); fff_free_result(r); r = fff_unwatch(picker, watch_dir_id); @@ -191,9 +223,18 @@ static int watch_smoke(void) { fprintf(stderr, "watch_smoke FAIL: repeated unwatch was not a no-op\n"); return 1; } + if (watch_rename_hits == 0) { + fprintf(stderr, "watch_smoke FAIL: no renamed event with a source path\n"); + return 1; + } + if (watch_rename_bad_source > 0) { + fprintf(stderr, "watch_smoke FAIL: %d renamed events had a wrong source\n", + watch_rename_bad_source); + return 1; + } - fprintf(stderr, "watch_smoke PASS (glob=%d dir=%d all=%d)\n", watch_glob_hits, watch_dir_hits, - watch_all_hits); + fprintf(stderr, "watch_smoke PASS (glob=%d dir=%d all=%d rename=%d)\n", watch_glob_hits, + watch_dir_hits, watch_all_hits, watch_rename_hits); return 0; } diff --git a/crates/fff-core/src/dbs/frecency.rs b/crates/fff-core/src/dbs/frecency.rs index 7413396a..8eaec320 100644 --- a/crates/fff-core/src/dbs/frecency.rs +++ b/crates/fff-core/src/dbs/frecency.rs @@ -291,6 +291,7 @@ impl FrecencyTracker { ); return Ok(()); } + return Err(Error::DbWrite { db: Self::LABEL, source: e, @@ -313,6 +314,67 @@ impl FrecencyTracker { }) } + pub fn copy_history(&self, from: &Path, to: &Path) -> Result { + if from == to { + return Ok(false); + } + + let Some(source) = self.get_accesses(from)?.filter(|a| !a.is_empty()) else { + return Ok(false); + }; + + let target_key = Self::path_to_hash_bytes(to)?; + let target = self.get_accesses(to)?.unwrap_or_default(); + + let mut timestamps: Vec = source.iter().chain(target.iter()).copied().collect(); + timestamps.sort_unstable(); + let overflow = timestamps.len().saturating_sub(MAX_TIMESTAMPS_PER_FILE); + let merged: VecDeque = timestamps.drain(overflow..).collect(); + + tracing::debug!( + ?from, + ?to, + accesses = merged.len(), + "Copying frecency history" + ); + + let mut wtxn = self + .env + .write_txn() + .map_err(|source| Error::DbStartWriteTxn { + db: Self::LABEL, + source, + })?; + if let Err(e) = self.db.put(&mut wtxn, &target_key, &merged) { + if is_map_full(&e) { + self.health.mark_unhealthy("MDB_MAP_FULL on put"); + tracing::error!(?to, "Frecency DB hit MDB_MAP_FULL; dropping history copy"); + return Ok(false); + } + return Err(Error::DbWrite { + db: Self::LABEL, + source: e, + }); + } + + if let Err(e) = wtxn.commit() { + if is_map_full(&e) { + self.health.mark_unhealthy("MDB_MAP_FULL on commit"); + tracing::error!( + ?to, + "Frecency DB hit MDB_MAP_FULL on commit; dropping history copy" + ); + return Ok(false); + } + return Err(Error::DbCommit { + db: Self::LABEL, + source: e, + }); + } + + Ok(true) + } + pub fn get_access_score(&self, file_path: &Path, mode: FFFMode) -> i64 { let accesses = self .get_accesses(file_path) diff --git a/crates/fff-core/src/shared.rs b/crates/fff-core/src/shared.rs index 06883ea8..7880411d 100644 --- a/crates/fff-core/src/shared.rs +++ b/crates/fff-core/src/shared.rs @@ -286,7 +286,7 @@ impl SharedFilePicker { /// Patterns may be base-relative globs (./ works), exact paths inside the indexed /// tree, or existing directories. An empty pattern watches the whole tree. /// - /// Events are debounced and submitted in batches per 100-ms window at most 128 events. + /// Events are debounced over a 50-ms window and submitted in batches of at most 128 events. /// Gitignored and other ignored files are never triggering watcher. pub fn watch( &self, diff --git a/crates/fff-core/src/watcher/background_watcher.rs b/crates/fff-core/src/watcher/background_watcher.rs index b6d6d714..0ef3bbfc 100644 --- a/crates/fff-core/src/watcher/background_watcher.rs +++ b/crates/fff-core/src/watcher/background_watcher.rs @@ -7,7 +7,7 @@ use crate::shared::{SharedFilePicker, SharedFrecency}; use crate::sort_buffer::sort_with_buffer; use crate::watch::{RawWatchEvent, WatchEventKind}; use git2::Repository; -use notify::event::{AccessKind, AccessMode}; +use notify::event::{AccessKind, AccessMode, ModifyKind, RenameMode}; use notify::{Config, EventKind, EventKindMask, RecursiveMode}; use notify_debouncer_full::{DebounceEventResult, DebouncedEvent, NoCache, new_debouncer_opt}; use parking_lot::Mutex; @@ -350,6 +350,7 @@ pub(crate) fn handle_debounced_events( let mut paths_to_add_or_modify = Vec::new(); let mut new_dirs_to_watch = Vec::new(); let mut affected_paths_count = 0usize; + let mut explicit_renames: Vec<(PathBuf, PathBuf)> = Vec::new(); let watch_registry = shared_picker.watch_registry(); let need_events_propagation = watch_registry.is_active(); @@ -402,6 +403,15 @@ pub(crate) fn handle_debounced_events( } tracing::debug!(event = ?debounced_event.event, "Processing FS event"); + + // Backends that pair the halves for us hand over [from, to] directly. + // The per-path pass below still classifies both, we only note the link. + if let EventKind::Modify(ModifyKind::Name(RenameMode::Both)) = debounced_event.event.kind + && let [from, to] = debounced_event.event.paths.as_slice() + { + explicit_renames.push((from.clone(), to.clone())); + } + for path in &debounced_event.event.paths { if matches!( path.file_name().and_then(|f| f.to_str()), @@ -527,7 +537,12 @@ pub(crate) fn handle_debounced_events( let mut index_update_rejected = false; let mut overflow_count = 0; let mut removed_from_dirs = Vec::new(); - let mut watch_events = ahash::AHashMap::new(); + // Destination path -> (kind, rename source). One entry per path, so a + // remove+add pair recognised as a rename collapses into a single event. + let mut watch_events: ahash::AHashMap)> = + ahash::AHashMap::new(); + // Destination -> source for every rename recognised in this batch. + let mut renames: ahash::AHashMap = ahash::AHashMap::new(); if !paths_to_remove.is_empty() || !dirs_to_remove.is_empty() @@ -549,12 +564,22 @@ pub(crate) fn handle_debounced_events( return new_dirs_to_watch; }; + // Must run before the removals below, while the vanished files still + // carry their indexed size/mtime. + renames.extend(detect_renames( + picker, + base_path, + &paths_to_remove, + &paths_to_add_or_modify, + &explicit_renames, + )); + for (path, may_be_dir) in &paths_to_remove { let removed = picker.remove_file_by_path(path); if removed { if need_events_propagation { - watch_events.insert(path.to_path_buf(), WatchEventKind::Removed); + watch_events.insert(path.to_path_buf(), (WatchEventKind::Removed, None)); } } else if *may_be_dir { // Not an indexed file: likely a dir renamed out of the tree @@ -579,7 +604,7 @@ pub(crate) fn handle_debounced_events( if need_events_propagation { for path in removed_from_dirs.drain(..) { - watch_events.insert(path, WatchEventKind::Removed); + watch_events.insert(path, (WatchEventKind::Removed, None)); } } @@ -597,19 +622,31 @@ pub(crate) fn handle_debounced_events( if picker.handle_create_or_modify(path).is_some() { files_to_update_git_status.push(path.to_path_buf()); if need_events_propagation { - let kind = if existed { - WatchEventKind::Modified - } else { - WatchEventKind::Created + let event = match renames.get(*path) { + Some(from) => (WatchEventKind::Renamed, Some(from.clone())), + None if existed => (WatchEventKind::Modified, None), + None => (WatchEventKind::Created, None), }; - watch_events.insert(path.to_path_buf(), kind); + watch_events.insert(path.to_path_buf(), event); } } else { index_update_rejected = true; } } + // A recognised move is one change, not a removal plus a creation. Drop + // the source's own event only where the destination really reported the + // pair — a destination that never made it into the index still needs + // its source announced as removed. + if need_events_propagation { + for (to, from) in &renames { + if matches!(watch_events.get(to), Some((WatchEventKind::Renamed, _))) { + watch_events.remove(from); + } + } + } + overflow_count = picker.get_overflow_files().len(); } @@ -638,15 +675,51 @@ pub(crate) fn handle_debounced_events( base_path, watch_events .into_iter() - .map(|(path, kind)| RawWatchEvent { + .map(|(path, (kind, from))| RawWatchEvent { path, kind, is_ignored: false, + from, }) .collect(), ); } + // Carry the access history onto the new path. Copied, not moved: checking + // out a revision where the old path exists again must still rank it. + if !renames.is_empty() { + let mut carried = Vec::new(); + if let Ok(frecency_guard) = shared_frecency.read() + && let Some(ref frecency) = *frecency_guard + { + for (to, from) in &renames { + match frecency.copy_history(from, to) { + Ok(true) => carried.push(to.clone()), + Ok(false) => {} + Err(e) => error!( + ?from, + ?to, + "Failed to carry frecency over a rename: {:?}", + e + ), + } + } + } + + if !carried.is_empty() { + info!(count = carried.len(), "Carried frecency across renames"); + if let Ok(mut picker_guard) = shared_picker.write() + && let Some(ref mut picker) = *picker_guard + && let Ok(frecency_guard) = shared_frecency.read() + && let Some(ref frecency) = *frecency_guard + { + for path in &carried { + let _ = picker.update_single_file_frecency(path, frecency); + } + } + } + } + // AI mode: auto-track frecency for all modified/created files. // Uses a 5-minute cooldown per file to prevent score inflation from rapid // burst edits (AI agents often edit the same file many times in minutes). @@ -791,6 +864,7 @@ fn index_new_directory( path: path.clone(), kind: WatchEventKind::Created, is_ignored: false, + from: None, }) .collect(); @@ -954,6 +1028,129 @@ fn watch_git_status_paths(debouncer: &mut Debouncer, git_workdir: Option<&PathBu } } +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +struct FileIdentity { + size: u64, + mtime: u64, +} + +// Returns destination -> source. Explicit pairs from the OS win outright; the +// rest is inferred from files that kept their exact size and mtime. Call while +// the removed paths are still indexed — that is the only source of their +// metadata once they are gone from disk. +fn detect_renames( + picker: &crate::file_picker::FilePicker, + base_path: &Path, + paths_to_remove: &[(&Path, bool)], + paths_to_add_or_modify: &[&Path], + explicit: &[(PathBuf, PathBuf)], +) -> ahash::AHashMap { + // Subscribers only ever see paths relative to the base, so a move that + // crosses the boundary stays a plain removal or creation. + let mut renames: ahash::AHashMap = explicit + .iter() + .filter(|(from, to)| from != to && from.starts_with(base_path) && to.starts_with(base_path)) + .map(|(from, to)| (to.clone(), from.clone())) + .collect(); + + if paths_to_remove.is_empty() || paths_to_add_or_modify.is_empty() { + return renames; + } + + let removed: Vec<(PathBuf, FileIdentity)> = paths_to_remove + .iter() + .filter(|(path, _)| !renames.values().any(|from| from == path)) + .filter_map(|(path, _)| { + let file = picker.get_file_by_path(path)?; + Some(( + path.to_path_buf(), + FileIdentity { + size: file.size, + mtime: file.modified, + }, + )) + }) + .collect(); + + let added: Vec<(PathBuf, FileIdentity)> = paths_to_add_or_modify + .iter() + .filter(|path| !renames.contains_key(**path)) + // A path already live in the index was edited, not moved into place. + .filter(|path| { + picker + .get_file_by_path(path) + .is_none_or(|file| file.is_deleted()) + }) + .filter_map(|path| { + let meta = std::fs::metadata(path).ok()?; + let mtime = meta + .modified() + .ok()? + .duration_since(std::time::UNIX_EPOCH) + .ok()? + .as_secs(); + Some(( + path.to_path_buf(), + FileIdentity { + size: meta.len(), + mtime, + }, + )) + }) + .collect(); + + for (from, to) in pair_renames_by_identity(&removed, &added) { + renames.insert(to, from); + } + + renames +} + +// Pair a removed with an added path when they are the only two files in the +// batch sharing a (size, mtime) — a rename preserves both exactly. Uniqueness +// on both sides is what stops bulk operations from cross-wiring histories. +fn pair_renames_by_identity( + removed: &[(PathBuf, FileIdentity)], + added: &[(PathBuf, FileIdentity)], +) -> Vec<(PathBuf, PathBuf)> { + if removed.is_empty() || added.is_empty() { + return Vec::new(); + } + + // `None` marks an identity claimed by more than one distinct path. The OS + // repeats a path within a batch (FSEvents does routinely), and a repeat is + // still one candidate, not a collision. + fn index_by_identity( + entries: &[(PathBuf, FileIdentity)], + ) -> ahash::AHashMap> { + let mut by_identity: ahash::AHashMap> = + ahash::AHashMap::default(); + for (path, identity) in entries { + by_identity + .entry(*identity) + .and_modify(|slot| { + if *slot != Some(path.as_path()) { + *slot = None; + } + }) + .or_insert(Some(path.as_path())); + } + by_identity + } + + let sources = index_by_identity(removed); + let targets = index_by_identity(added); + + targets + .into_iter() + .filter_map(|(identity, target)| { + let target = target?; + let source = (*sources.get(&identity)?)?; + Some((source.to_path_buf(), target.to_path_buf())) + }) + .collect() +} + #[cfg(test)] mod tests { use super::*; @@ -1028,6 +1225,112 @@ mod tests { assert_eq!(received[0].kind, WatchEventKind::Modified); } + fn identity(size: u64, mtime: u64) -> FileIdentity { + FileIdentity { size, mtime } + } + + fn entry(path: &str, size: u64, mtime: u64) -> (PathBuf, FileIdentity) { + (PathBuf::from(path), identity(size, mtime)) + } + + #[test] + fn unique_size_and_mtime_pairs_as_a_rename() { + let pairs = pair_renames_by_identity( + &[entry("/w/old.rs", 120, 1_700_000_000)], + &[entry("/w/new.rs", 120, 1_700_000_000)], + ); + + assert_eq!( + pairs, + vec![(PathBuf::from("/w/old.rs"), PathBuf::from("/w/new.rs"))] + ); + } + + #[test] + fn ambiguous_identities_are_never_paired() { + let two_removed = pair_renames_by_identity( + &[entry("/w/a.rs", 10, 5), entry("/w/b.rs", 10, 5)], + &[entry("/w/c.rs", 10, 5)], + ); + assert!( + two_removed.is_empty(), + "two removed files sharing an identity are ambiguous: {two_removed:?}" + ); + + let two_added = pair_renames_by_identity( + &[entry("/w/a.rs", 10, 5)], + &[entry("/w/b.rs", 10, 5), entry("/w/c.rs", 10, 5)], + ); + assert!( + two_added.is_empty(), + "two added files sharing an identity are ambiguous: {two_added:?}" + ); + } + + #[test] + fn a_repeated_path_is_one_candidate_not_a_collision() { + // FSEvents routinely reports the same path several times per batch. + let pairs = pair_renames_by_identity( + &[entry("/w/old.rs", 15, 99), entry("/w/old.rs", 15, 99)], + &[entry("/w/new.rs", 15, 99), entry("/w/new.rs", 15, 99)], + ); + + assert_eq!( + pairs, + vec![(PathBuf::from("/w/old.rs"), PathBuf::from("/w/new.rs"))] + ); + } + + #[test] + fn empty_files_are_paired_only_when_unambiguous() { + let unique = pair_renames_by_identity(&[entry("/w/a", 0, 7)], &[entry("/w/b", 0, 7)]); + assert_eq!( + unique, + vec![(PathBuf::from("/w/a"), PathBuf::from("/w/b"))], + "a lone empty file still has a usable identity" + ); + + let colliding = pair_renames_by_identity( + &[entry("/w/a", 0, 7), entry("/w/b", 0, 7)], + &[entry("/w/c", 0, 7), entry("/w/d", 0, 7)], + ); + assert!(colliding.is_empty(), "{colliding:?}"); + } + + #[test] + fn differing_metadata_is_not_a_rename() { + // git checkout rewrites mtime, so branch switches must never pair. + let fresh_mtime = pair_renames_by_identity( + &[entry("/w/old.rs", 120, 1_700_000_000)], + &[entry("/w/new.rs", 120, 1_700_000_900)], + ); + assert!(fresh_mtime.is_empty(), "{fresh_mtime:?}"); + + let other_size = pair_renames_by_identity( + &[entry("/w/old.rs", 120, 1_700_000_000)], + &[entry("/w/new.rs", 121, 1_700_000_000)], + ); + assert!(other_size.is_empty(), "{other_size:?}"); + } + + #[test] + fn independent_identities_pair_side_by_side() { + let pairs = pair_renames_by_identity( + &[entry("/w/a.rs", 10, 1), entry("/w/b.rs", 20, 2)], + &[entry("/w/y.rs", 20, 2), entry("/w/x.rs", 10, 1)], + ); + + let mut pairs = pairs; + pairs.sort(); + assert_eq!( + pairs, + vec![ + (PathBuf::from("/w/a.rs"), PathBuf::from("/w/x.rs")), + (PathBuf::from("/w/b.rs"), PathBuf::from("/w/y.rs")), + ] + ); + } + #[test] fn dotgit_status_filter_matches_worktree_state_changes() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/fff-core/src/watcher/rescan_tests.rs b/crates/fff-core/src/watcher/rescan_tests.rs index 1a6c4ef6..47b4e37e 100644 --- a/crates/fff-core/src/watcher/rescan_tests.rs +++ b/crates/fff-core/src/watcher/rescan_tests.rs @@ -14,9 +14,12 @@ use tempfile::TempDir; use super::handle_debounced_events; use crate::constants::MAX_OVERFLOW_FILES; use crate::file_picker::{FFFMode, FilePicker, FilePickerOptions}; +use crate::frecency::FrecencyTracker; use crate::git_status_worker::GitStatusWorker; use crate::rescan_stats::{RescanReason, RescanStats}; use crate::shared::{SharedFilePicker, SharedFrecency}; +use crate::watch::{WatchEvent, WatchEventKind, WatchOptions}; +use std::sync::mpsc; #[test] fn saving_an_indexed_file_stays_incremental() { @@ -483,6 +486,178 @@ fn a_throttled_ignore_file_event_is_still_applied_incrementally() { ); } +#[test] +fn renaming_a_file_carries_frecency_and_keeps_the_old_entry() { + let f = Fixture::with_frecency(); + f.write("src/old.rs", "fn old() {}"); + f.index(); + + for _ in 0..3 { + f.track_access("src/old.rs"); + } + let before = f.access_score("src/old.rs"); + assert!(before > 0, "precondition: source must be ranked"); + + // A real rename so size and mtime are genuinely preserved. + std::fs::rename(f.path("src/old.rs"), f.path("src/new.rs")).unwrap(); + let delta = f.feed([ + remove_file(f.path("src/old.rs")), + create(f.path("src/new.rs")), + ]); + + f.assert_no_rescan(&delta, "a rename"); + assert!(f.is_indexed("src/new.rs"), "destination must be indexed"); + assert_eq!( + f.access_score("src/new.rs"), + before, + "destination inherits the history" + ); + assert_eq!( + f.access_score("src/old.rs"), + before, + "source keeps its history so old revisions still rank" + ); + assert!( + f.indexed_access_score("src/new.rs") > 0, + "in-memory score must be refreshed, not left at zero until the next git pass" + ); +} + +#[test] +fn renaming_a_file_emits_a_single_renamed_event() { + let f = Fixture::new(); + f.write("src/old.rs", "fn old() {}"); + f.index(); + let events = f.subscribe("**"); + + std::fs::rename(f.path("src/old.rs"), f.path("src/new.rs")).unwrap(); + f.feed([ + remove_file(f.path("src/old.rs")), + create(f.path("src/new.rs")), + ]); + + let received = events.recv_timeout(Duration::from_secs(1)).unwrap(); + assert_eq!(received.len(), 1, "expected one event, got {received:?}"); + assert_eq!(received[0].kind, WatchEventKind::Renamed); + assert_eq!(received[0].path, f.path("src/new.rs")); + assert_eq!( + received[0].from.as_deref(), + Some(f.path("src/old.rs").as_path()) + ); +} + +#[test] +fn paired_rename_events_are_trusted_over_the_heuristic() { + let f = Fixture::new(); + f.write("src/old.rs", "fn old() {}"); + f.index(); + let events = f.subscribe("**"); + + std::fs::rename(f.path("src/old.rs"), f.path("src/new.rs")).unwrap(); + // Linux/inotify pairs the halves for us and hands over both paths at once. + f.feed([DebouncedEvent::new( + Event::new(EventKind::Modify(ModifyKind::Name(RenameMode::Both))) + .add_path(f.path("src/old.rs")) + .add_path(f.path("src/new.rs")), + Instant::now(), + )]); + + let received = events.recv_timeout(Duration::from_secs(1)).unwrap(); + assert_eq!(received.len(), 1, "expected one event, got {received:?}"); + assert_eq!(received[0].kind, WatchEventKind::Renamed); + assert_eq!(received[0].path, f.path("src/new.rs")); + assert_eq!( + received[0].from.as_deref(), + Some(f.path("src/old.rs").as_path()) + ); +} + +#[test] +fn editing_an_indexed_file_is_never_mistaken_for_a_rename() { + let f = Fixture::new(); + f.write("src/keep.rs", "same size!!"); + f.write("src/gone.rs", "same size!!"); + f.index(); + let events = f.subscribe("**"); + + // Both files share a size; copying the mtime across makes their identities + // collide. `keep.rs` is still indexed, so it is an edit, not a destination. + let mtime = std::fs::metadata(f.path("src/gone.rs")) + .unwrap() + .modified() + .unwrap(); + std::fs::remove_file(f.path("src/gone.rs")).unwrap(); + std::fs::File::open(f.path("src/keep.rs")) + .unwrap() + .set_modified(mtime) + .unwrap(); + + f.feed([ + remove_file(f.path("src/gone.rs")), + modify(f.path("src/keep.rs")), + ]); + + let received = events.recv_timeout(Duration::from_secs(1)).unwrap(); + assert!( + received.iter().all(|e| e.kind != WatchEventKind::Renamed), + "an edit to a live file must not be reported as a rename: {received:?}" + ); +} + +#[test] +fn unrelated_delete_and_create_never_share_history() { + let f = Fixture::with_frecency(); + f.write("src/tracked.rs", "fn tracked() {}"); + f.index(); + + for _ in 0..3 { + f.track_access("src/tracked.rs"); + } + assert!(f.access_score("src/tracked.rs") > 0); + + f.remove("src/tracked.rs"); + f.write( + "src/unrelated.rs", + "fn unrelated() { /* different size */ }", + ); + f.feed([ + remove_file(f.path("src/tracked.rs")), + create(f.path("src/unrelated.rs")), + ]); + + assert_eq!( + f.access_score("src/unrelated.rs"), + 0, + "an unrelated file must not inherit anyone's history" + ); +} + +#[test] +fn ambiguous_batch_falls_back_to_remove_and_create() { + let f = Fixture::new(); + f.write("src/a.rs", "same"); + f.write("src/b.rs", "same"); + f.index(); + let events = f.subscribe("**"); + + // Two identical files renamed at once: nothing can be paired safely. + std::fs::rename(f.path("src/a.rs"), f.path("src/x.rs")).unwrap(); + std::fs::rename(f.path("src/b.rs"), f.path("src/y.rs")).unwrap(); + f.feed([ + remove_file(f.path("src/a.rs")), + remove_file(f.path("src/b.rs")), + create(f.path("src/x.rs")), + create(f.path("src/y.rs")), + ]); + + let received = events.recv_timeout(Duration::from_secs(1)).unwrap(); + assert!( + received.iter().all(|e| e.kind != WatchEventKind::Renamed), + "ambiguous identities must not be guessed: {received:?}" + ); + assert_eq!(received.len(), 4, "{received:?}"); +} + struct Fixture { base: PathBuf, picker: SharedFilePicker, @@ -492,18 +667,25 @@ struct Fixture { // Dropped last so background work started by a triggered rescan still // sees the tree it was asked to walk. _tmp: TempDir, + _frecency_tmp: Option, } impl Fixture { fn new() -> Self { - Self::build(false) + Self::build(false, false) } fn with_git() -> Self { - Self::build(true) + Self::build(true, false) + } + + // `SharedFrecency::noop()` silently drops `init`, so any test asserting on + // scores needs a real LMDB-backed tracker instead. + fn with_frecency() -> Self { + Self::build(false, true) } - fn build(git: bool) -> Self { + fn build(git: bool, frecency: bool) -> Self { let tmp = tempfile::tempdir().unwrap(); let base = crate::path_utils::canonicalize(tmp.path()).unwrap(); let git_workdir = git.then(|| { @@ -516,16 +698,70 @@ impl Fixture { base.clone() }); + let (shared_frecency, frecency_tmp) = if frecency { + let db = tempfile::tempdir().unwrap(); + let shared = SharedFrecency::default(); + shared + .init(FrecencyTracker::open(db.path().join("frecency.mdb")).expect("open frecency")) + .expect("init frecency"); + (shared, Some(db)) + } else { + (SharedFrecency::noop(), None) + }; + Self { base, picker: SharedFilePicker::default(), - frecency: SharedFrecency::noop(), + frecency: shared_frecency, git_workdir, git_worker: GitStatusWorker::new(), _tmp: tmp, + _frecency_tmp: frecency_tmp, } } + fn track_access(&self, rel: &str) { + let guard = self.frecency.read().unwrap(); + guard + .as_ref() + .expect("fixture built without frecency") + .track_access(&self.path(rel)) + .expect("track access"); + } + + fn access_score(&self, rel: &str) -> i64 { + let guard = self.frecency.read().unwrap(); + guard + .as_ref() + .expect("fixture built without frecency") + .get_access_score(&self.path(rel), FFFMode::Neovim) + } + + fn indexed_access_score(&self, rel: &str) -> i16 { + let guard = self.picker.read().unwrap(); + guard + .as_ref() + .and_then(|p| p.get_file_by_path(self.path(rel))) + .map(|file| file.access_frecency_score) + .unwrap_or(0) + } + + fn subscribe(&self, pattern: &str) -> mpsc::Receiver> { + let (sender, receiver) = mpsc::channel(); + self.picker + .watch_registry() + .subscribe( + &self.base, + pattern, + WatchOptions::default(), + Box::new(move |_, events| { + let _ = sender.send(events.to_vec()); + }), + ) + .expect("subscribe"); + receiver + } + fn index(&self) { let mut picker = FilePicker::new(FilePickerOptions { base_path: self.base.to_string_lossy().into_owned(), diff --git a/crates/fff-core/src/watcher/watch.rs b/crates/fff-core/src/watcher/watch.rs index 2a877308..bdd55b69 100644 --- a/crates/fff-core/src/watcher/watch.rs +++ b/crates/fff-core/src/watcher/watch.rs @@ -26,6 +26,8 @@ pub enum WatchEventKind { Removed = 2, /// Individual events were lost; rescan the reported path. Rescan = 3, + /// The file moved. `path` is the destination, `from` the source. + Renamed = 4, } impl WatchEventKind { @@ -35,6 +37,7 @@ impl WatchEventKind { WatchEventKind::Modified => "modified", WatchEventKind::Removed => "removed", WatchEventKind::Rescan => "rescan", + WatchEventKind::Renamed => "renamed", } } } @@ -45,6 +48,8 @@ pub struct WatchEvent { /// Absolute affected path (the indexed base path for `Rescan`). pub path: PathBuf, pub kind: WatchEventKind, + /// Source path, set only for [`WatchEventKind::Renamed`]. + pub from: Option, } /// Per-subscription options. @@ -61,6 +66,7 @@ pub(crate) struct RawWatchEvent { pub(crate) path: PathBuf, pub(crate) kind: WatchEventKind, pub(crate) is_ignored: bool, + pub(crate) from: Option, } enum WatchMatcher { @@ -473,6 +479,10 @@ impl WatchRegistry { let mut paths = Vec::with_capacity(batch.len()); let mut visible_mask = 0; let mut rescan_mask = 0; + let has_renames = batch.iter().any(|event| event.from.is_some()); + // Parallel array of rename sources, so a subscription matching only + // the pre-rename path still hears about the move. + let mut from_paths = Vec::with_capacity(if has_renames { batch.len() } else { 0 }); for (index, event) in batch.iter().enumerate() { let relative = event @@ -481,6 +491,15 @@ impl WatchRegistry { .expect("watch event path must be inside the indexed base path"); paths.push(relative.to_string_lossy().replace('\\', "/")); + if has_renames { + let source = event + .from + .as_deref() + .and_then(|from| from.strip_prefix(base_path).ok()) + .map(|rel| rel.to_string_lossy().replace('\\', "/")); + from_paths.push(source.unwrap_or_else(|| paths[index].clone())); + } + let bit = 1 << index; if event.kind == WatchEventKind::Rescan { rescan_mask |= bit; @@ -490,10 +509,14 @@ impl WatchRegistry { } let path_refs: Vec<&str> = paths.iter().map(String::as_str).collect(); + let from_refs: Vec<&str> = from_paths.iter().map(String::as_str).collect(); let mut scratch = Vec::new(); let mut deliveries = Vec::with_capacity(state.subs.len()); for sub in &state.subs { - let matched = sub.filter_mask(&path_refs, &mut scratch); + let mut matched = sub.filter_mask(&path_refs, &mut scratch); + if has_renames { + matched |= sub.filter_mask(&from_refs, &mut scratch); + } let mut delivery_mask = (matched & visible_mask) | rescan_mask; if delivery_mask == 0 { continue; @@ -506,6 +529,7 @@ impl WatchRegistry { filtered.push(WatchEvent { path: event.path.clone(), kind: event.kind, + from: event.from.clone(), }); delivery_mask &= delivery_mask - 1; } @@ -533,6 +557,7 @@ impl WatchRegistry { path: base_path.to_path_buf(), kind: WatchEventKind::Rescan, is_ignored: false, + from: None, }], ); } @@ -556,6 +581,7 @@ mod tests { path: PathBuf::from(path), kind, is_ignored, + from: None, } } diff --git a/crates/fff-core/tests/rename_frecency_test.rs b/crates/fff-core/tests/rename_frecency_test.rs new file mode 100644 index 00000000..62521866 --- /dev/null +++ b/crates/fff-core/tests/rename_frecency_test.rs @@ -0,0 +1,119 @@ +use std::path::{Path, PathBuf}; + +use fff_search::file_picker::FFFMode; +use fff_search::frecency::FrecencyTracker; +use tempfile::TempDir; + +fn tracker() -> (FrecencyTracker, TempDir) { + let dir = TempDir::new().expect("mktemp frecency db"); + let tracker = FrecencyTracker::open(dir.path().join("frecency.mdb")).expect("open frecency db"); + (tracker, dir) +} + +fn score(tracker: &FrecencyTracker, path: &Path) -> i64 { + tracker.get_access_score(path, FFFMode::Neovim) +} + +#[test] +fn copy_history_preserves_source_entry() { + let (tracker, _dir) = tracker(); + let old = PathBuf::from("/w/src/old.rs"); + let new = PathBuf::from("/w/src/new.rs"); + + for _ in 0..3 { + tracker.track_access(&old).expect("track access"); + } + let before = score(&tracker, &old); + assert!(before > 0, "precondition: source must have a score"); + assert_eq!(score(&tracker, &new), 0, "precondition: target is unknown"); + + assert!(tracker.copy_history(&old, &new).expect("copy history")); + + assert_eq!( + score(&tracker, &new), + before, + "destination inherits the full history" + ); + // The whole point of copying rather than moving: checking out a revision + // where the old path still exists must not have lost anything. + assert_eq!( + score(&tracker, &old), + before, + "source history must survive the rename" + ); + assert_eq!(tracker.access_count(&old).unwrap(), 3); + assert_eq!(tracker.access_count(&new).unwrap(), 3); +} + +#[test] +fn copy_history_merges_into_existing_target() { + let (tracker, _dir) = tracker(); + let old = PathBuf::from("/w/src/old.rs"); + let new = PathBuf::from("/w/src/new.rs"); + + for _ in 0..2 { + tracker.track_access(&old).expect("track access"); + } + for _ in 0..3 { + tracker.track_access(&new).expect("track access"); + } + + assert!(tracker.copy_history(&old, &new).expect("copy history")); + + // Every access is a distinct data point for the score, including repeats + // inside one second, so the merge is the union of both histories. + assert_eq!( + tracker.access_count(&new).unwrap(), + 5, + "the target keeps its own history and gains the source's" + ); + assert_eq!( + tracker.access_count(&old).unwrap(), + 2, + "source is untouched by a merge" + ); +} + +#[test] +fn copy_history_caps_the_merged_history() { + let (tracker, _dir) = tracker(); + let old = PathBuf::from("/w/src/old.rs"); + let new = PathBuf::from("/w/src/new.rs"); + + // MAX_TIMESTAMPS_PER_FILE is 128; overshoot from both sides. + for _ in 0..200 { + tracker.track_access(&old).expect("track access"); + tracker.track_access(&new).expect("track access"); + } + + tracker.copy_history(&old, &new).expect("copy history"); + + assert!( + tracker.access_count(&new).unwrap() <= 128, + "merged history must respect MAX_TIMESTAMPS_PER_FILE" + ); +} + +#[test] +fn copy_history_is_noop_for_unknown_source() { + let (tracker, _dir) = tracker(); + let old = PathBuf::from("/w/src/never-seen.rs"); + let new = PathBuf::from("/w/src/new.rs"); + + assert!(!tracker.copy_history(&old, &new).expect("copy history")); + assert_eq!( + tracker.access_count(&new).unwrap(), + 0, + "no empty entry may be written for the destination" + ); +} + +#[test] +fn copy_history_is_noop_for_same_path() { + let (tracker, _dir) = tracker(); + let path = PathBuf::from("/w/src/file.rs"); + tracker.track_access(&path).expect("track access"); + + assert!(!tracker.copy_history(&path, &path).expect("copy history")); + assert_eq!(tracker.access_count(&path).unwrap(), 1); +} diff --git a/crates/fff-core/tests/watch_subscription_test.rs b/crates/fff-core/tests/watch_subscription_test.rs index 9c6385f3..347022df 100644 --- a/crates/fff-core/tests/watch_subscription_test.rs +++ b/crates/fff-core/tests/watch_subscription_test.rs @@ -274,6 +274,141 @@ fn moved_out_directory_delivers_removed_event_per_file() { ); } +#[test] +fn renaming_a_file_delivers_one_renamed_event_with_both_paths() { + let tmp = TempDir::new().unwrap(); + let base = fff_search::path_utils::canonicalize(tmp.path()).unwrap(); + seed(&base); + let old = base.join("src/renamed_from.rs"); + fs::write(&old, "pub fn hi() {}\n").unwrap(); + + let (picker, _frecency) = make_watched_picker(&base); + let events = watch_collect(&picker, "", WatchOptions::default()); + + let new = base.join("src/renamed_to.rs"); + fs::rename(&old, &new).unwrap(); + + assert!( + wait_for( + || events + .lock() + .iter() + .any(|e| e.kind == WatchEventKind::Renamed && e.path == new), + Duration::from_secs(10) + ), + "expected a Renamed event for {}, got: {:?}", + new.display(), + events.lock() + ); + + let got = events.lock(); + let renamed = got + .iter() + .find(|e| e.kind == WatchEventKind::Renamed) + .unwrap(); + assert_eq!(renamed.from.as_deref(), Some(old.as_path())); + assert!( + !got.iter() + .any(|e| e.path == old && e.kind == WatchEventKind::Removed), + "a recognised rename must not also report the source as Removed: {got:?}" + ); +} + +#[test] +fn renaming_out_of_a_watched_subtree_still_notifies() { + let tmp = TempDir::new().unwrap(); + let base = fff_search::path_utils::canonicalize(tmp.path()).unwrap(); + seed(&base); + fs::create_dir_all(base.join("vendor")).unwrap(); + let inside = base.join("src/leaving.rs"); + fs::write(&inside, "pub fn leaving() {}\n").unwrap(); + + let (picker, _frecency) = make_watched_picker(&base); + // Only `src/` is watched; the destination is outside it. + let events = watch_collect(&picker, "src", WatchOptions::default()); + + let outside = base.join("vendor/leaving.rs"); + fs::rename(&inside, &outside).unwrap(); + + assert!( + wait_for( + || events + .lock() + .iter() + .any(|e| e.kind == WatchEventKind::Renamed + && e.from.as_deref() == Some(inside.as_path())), + Duration::from_secs(10) + ), + "a subscriber on src/ must hear that a file moved out of it, got: {:?}", + events.lock() + ); +} + +#[test] +fn renaming_into_a_watched_subtree_still_notifies() { + let tmp = TempDir::new().unwrap(); + let base = fff_search::path_utils::canonicalize(tmp.path()).unwrap(); + seed(&base); + fs::create_dir_all(base.join("vendor")).unwrap(); + let outside = base.join("vendor/arriving.rs"); + fs::write(&outside, "pub fn arriving() {}\n").unwrap(); + + let (picker, _frecency) = make_watched_picker(&base); + let events = watch_collect(&picker, "src", WatchOptions::default()); + + let inside = base.join("src/arriving.rs"); + fs::rename(&outside, &inside).unwrap(); + + assert!( + wait_for( + || events + .lock() + .iter() + .any(|e| e.kind == WatchEventKind::Renamed && e.path == inside), + Duration::from_secs(10) + ), + "a subscriber on src/ must hear that a file moved into it, got: {:?}", + events.lock() + ); +} + +#[test] +fn renaming_across_the_base_boundary_degrades_to_removed() { + let tmp = TempDir::new().unwrap(); + let outside_root = TempDir::new().unwrap(); + let base = fff_search::path_utils::canonicalize(tmp.path()).unwrap(); + seed(&base); + let inside = base.join("src/exiting.rs"); + fs::write(&inside, "pub fn exiting() {}\n").unwrap(); + + let (picker, _frecency) = make_watched_picker(&base); + let events = watch_collect(&picker, "", WatchOptions::default()); + + // The destination is outside the indexed tree, so there is no pair to + // report — this stays a plain removal. + fs::rename(&inside, outside_root.path().join("exiting.rs")).unwrap(); + + assert!( + wait_for( + || events + .lock() + .iter() + .any(|e| e.path == inside && e.kind == WatchEventKind::Removed), + Duration::from_secs(10) + ), + "expected Removed for a file moved out of the base, got: {:?}", + events.lock() + ); + assert!( + events + .lock() + .iter() + .all(|e| e.kind != WatchEventKind::Renamed), + "no Renamed may escape with a path outside the base: {:?}", + events.lock() + ); +} + #[test] fn empty_pattern_watches_the_whole_tree() { let tmp = TempDir::new().unwrap(); diff --git a/crates/fff-python/src/finder.rs b/crates/fff-python/src/finder.rs index d2a7ad39..483b33b1 100644 --- a/crates/fff-python/src/finder.rs +++ b/crates/fff-python/src/finder.rs @@ -746,7 +746,7 @@ impl FileFinder { /// Patterns may be base-relative globs (./ works), exact paths inside the indexed /// tree, or existing directories. An empty pattern watches the whole tree. /// - /// Events are debounced and submitted in batches per 100-ms window at most 128 events. + /// Events are debounced over a 50-ms window and submitted in batches of at most 128 events. /// Gitignored and other ignored files are never triggering watcher. #[pyo3(signature = (pattern, callback, *, ignore = None))] fn watch( @@ -783,6 +783,10 @@ impl FileFinder { .map(|ev| WatchEvent { path: ev.path.to_string_lossy().to_string(), kind: ev.kind.as_str().to_string(), + from_path: ev + .from + .as_ref() + .map(|p| p.to_string_lossy().to_string()), }) .collect(); if let Err(e) = callback.call1(py, (batch,)) { diff --git a/crates/fff-python/src/types.rs b/crates/fff-python/src/types.rs index ec18b18a..152e4916 100644 --- a/crates/fff-python/src/types.rs +++ b/crates/fff-python/src/types.rs @@ -395,12 +395,21 @@ pub struct WatchEvent { pub path: String, #[pyo3(get)] pub kind: String, + /// Path the file moved from. Only set when `kind == "renamed"`. + #[pyo3(get)] + pub from_path: Option, } #[pymethods] impl WatchEvent { fn __repr__(&self) -> String { - format!("WatchEvent(path={:?}, kind={:?})", self.path, self.kind) + match &self.from_path { + Some(from) => format!( + "WatchEvent(path={:?}, kind={:?}, from_path={:?})", + self.path, self.kind, from + ), + None => format!("WatchEvent(path={:?}, kind={:?})", self.path, self.kind), + } } } diff --git a/packages/fff-bun/README.md b/packages/fff-bun/README.md index 6dded878..1521f82b 100644 --- a/packages/fff-bun/README.md +++ b/packages/fff-bun/README.md @@ -104,7 +104,7 @@ up to 128, so callbacks stay cheap even under heavy churn. ```typescript // Each path appears at most once per batch const sub = finder.watch("src/**/*.ts", (events) => { - for (const e of events) console.log(e.kind, e.path); // created | modified | removed | rescan + for (const e of events) console.log(e.kind, e.path); // created | modified | removed | renamed | rescan }); // No pattern: watch the entire indexed tree @@ -138,6 +138,9 @@ Notes: - `ignore` entries exclude matches per subscription: wildcards are globs, everything else is a path prefix (a file or a whole subtree). - Gitignored paths never produce events. +- A `renamed` event carries both paths: `e.path` is the destination and + `e.from` the source. It replaces the removed/created pair, so a move is one + event. Moves in or out of the indexed tree stay a plain `created`/`removed`. - A `rescan` event means changes were lost (index overflow, ignore-file change) — re-stat anything you care about. - Unsubscribing takes effect synchronously on the JS thread: once it diff --git a/packages/fff-bun/src/fff-api.ts b/packages/fff-bun/src/fff-api.ts index 2d0038aa..4dbacfed 100644 --- a/packages/fff-bun/src/fff-api.ts +++ b/packages/fff-bun/src/fff-api.ts @@ -299,14 +299,20 @@ export interface ScanProgress { * * rescan = internal OS buffers were overloaded, some events might be missing. * The `path` is going to be a folder needs to be rescanned + * + * renamed = the file moved and kept its identity. `path` is the destination + * and `from` the source; no separate removed/created pair is emitted. A move + * in or out of the indexed tree stays a plain created/removed instead. */ -export type WatchEventKind = "created" | "modified" | "removed" | "rescan"; +export type WatchEventKind = "created" | "modified" | "removed" | "rescan" | "renamed"; /** A single filesystem change notification. */ export interface WatchEvent { /** Absolute path of the affected file (base path to rescan if `kind ==rescan`) */ path: string; kind: WatchEventKind; + /** Absolute path the file moved from. Only set when `kind === "renamed"`. */ + from?: string; } /** Options for watch subscriptions. */ @@ -638,7 +644,7 @@ export interface FileFinderApi { * Patterns may be base-relative globs (./ works), exact paths inside the indexed * tree, or existing directories. An empty pattern watches the whole tree. * - * Events are debounced and submitted in batches per 100-ms window at most 128 events. + * Events are debounced over a 50-ms window and submitted in batches of at most 128 events. * Gitignored and other ignored files are never triggering watcher. */ watch(callback: WatchBatchCallback, options?: WatchOptions): Result; diff --git a/packages/fff-bun/src/ffi.ts b/packages/fff-bun/src/ffi.ts index 0a9f3b07..cea5275b 100644 --- a/packages/fff-bun/src/ffi.ts +++ b/packages/fff-bun/src/ffi.ts @@ -239,6 +239,10 @@ const ffiDefinition = { args: [FFIType.ptr, FFIType.u32], returns: FFIType.u8, }, + fff_watch_events_get_from_path: { + args: [FFIType.ptr, FFIType.u32], + returns: FFIType.ptr, + }, // Git fff_refresh_git_status: { @@ -1392,6 +1396,7 @@ const WATCH_EVENT_KINDS: readonly WatchEventKind[] = [ "modified", "removed", "rescan", + "renamed", ]; /** @@ -1412,10 +1417,16 @@ export function readWatchEventBatch(batchPtr: Pointer | number | null): WatchEve for (let i = 0; i < count; i++) { const path = symbols.fff_watch_events_get_path(bp, i) as Pointer | null; const kind = symbols.fff_watch_events_get_kind(bp, i) as number; - events.push({ + const event: WatchEvent = { path: readCString(path) ?? "", kind: WATCH_EVENT_KINDS[kind] ?? "rescan", - }); + }; + if (event.kind === "renamed") { + const from = symbols.fff_watch_events_get_from_path(bp, i) as Pointer | null; + const decoded = readCString(from); + if (decoded) event.from = decoded; + } + events.push(event); } symbols.fff_free_watch_events(bp); diff --git a/packages/fff-bun/test/watch.test.ts b/packages/fff-bun/test/watch.test.ts index ab75be46..03d724e8 100644 --- a/packages/fff-bun/test/watch.test.ts +++ b/packages/fff-bun/test/watch.test.ts @@ -1,5 +1,5 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; -import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, realpathSync, renameSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { WatchEvent } from "../src/fff-api"; @@ -96,12 +96,45 @@ describe("FileFinder - Watch Subscriptions", () => { expect(received.some((e) => e.path.endsWith(".js"))).toBe(false); expect(batchSizes.every((n) => n > 0)).toBe(true); for (const event of received) { - expect(["created", "modified", "removed", "rescan"]).toContain(event.kind); + expect(["created", "modified", "removed", "rescan", "renamed"]).toContain( + event.kind, + ); } sub.value(); }, 20_000); + test("a rename arrives as one renamed event carrying both paths", async () => { + const received: WatchEvent[] = []; + + const sub = finder.watch((events) => { + received.push(...events); + }); + expect(sub.ok).toBe(true); + if (!sub.ok) return; + + const from = join(baseDir, "move-me.txt"); + const to = join(baseDir, "moved.txt"); + writeFileSync(from, "move me\n"); + // Let the create land so the rename is its own batch. + expect(await waitFor(() => hasEventFor(received, "move-me.txt"))).toBe(true); + received.length = 0; + + renameSync(from, to); + + const gotRename = await waitFor(() => + received.some((e) => e.kind === "renamed" && e.path === to), + ); + expect(gotRename).toBe(true); + + const renamed = received.find((e) => e.kind === "renamed"); + expect(renamed?.from).toBe(from); + // The move is one event, not a removal plus a creation. + expect(received.some((e) => e.kind === "removed" && e.path === from)).toBe(false); + + sub.value(); + }, 20_000); + test("per-event consumption is a one-line loop over watch", async () => { const received: WatchEvent[] = []; diff --git a/packages/fff-node/README.md b/packages/fff-node/README.md index b0b7e85a..8e83f5cf 100644 --- a/packages/fff-node/README.md +++ b/packages/fff-node/README.md @@ -83,7 +83,7 @@ up to 128, so callbacks stay cheap even under heavy churn. ```typescript // Each path appears at most once per batch const sub = finder.watch("src/**/*.ts", (events) => { - for (const e of events) console.log(e.kind, e.path); // created | modified | removed | rescan + for (const e of events) console.log(e.kind, e.path); // created | modified | removed | renamed | rescan }); // No pattern: watch the entire indexed tree @@ -117,6 +117,9 @@ Notes: - `ignore` entries exclude matches per subscription: wildcards are globs, everything else is a path prefix (a file or a whole subtree). - Gitignored paths never produce events. +- A `renamed` event carries both paths: `e.path` is the destination and + `e.from` the source. It replaces the removed/created pair, so a move is one + event. Moves in or out of the indexed tree stay a plain `created`/`removed`. - A `rescan` event means changes were lost (index overflow, ignore-file change) — re-stat anything you care about. - Unsubscribing takes effect synchronously on the JS thread: once it diff --git a/packages/fff-node/src/fff-api.ts b/packages/fff-node/src/fff-api.ts index 2d0038aa..4dbacfed 100644 --- a/packages/fff-node/src/fff-api.ts +++ b/packages/fff-node/src/fff-api.ts @@ -299,14 +299,20 @@ export interface ScanProgress { * * rescan = internal OS buffers were overloaded, some events might be missing. * The `path` is going to be a folder needs to be rescanned + * + * renamed = the file moved and kept its identity. `path` is the destination + * and `from` the source; no separate removed/created pair is emitted. A move + * in or out of the indexed tree stays a plain created/removed instead. */ -export type WatchEventKind = "created" | "modified" | "removed" | "rescan"; +export type WatchEventKind = "created" | "modified" | "removed" | "rescan" | "renamed"; /** A single filesystem change notification. */ export interface WatchEvent { /** Absolute path of the affected file (base path to rescan if `kind ==rescan`) */ path: string; kind: WatchEventKind; + /** Absolute path the file moved from. Only set when `kind === "renamed"`. */ + from?: string; } /** Options for watch subscriptions. */ @@ -638,7 +644,7 @@ export interface FileFinderApi { * Patterns may be base-relative globs (./ works), exact paths inside the indexed * tree, or existing directories. An empty pattern watches the whole tree. * - * Events are debounced and submitted in batches per 100-ms window at most 128 events. + * Events are debounced over a 50-ms window and submitted in batches of at most 128 events. * Gitignored and other ignored files are never triggering watcher. */ watch(callback: WatchBatchCallback, options?: WatchOptions): Result; diff --git a/packages/fff-node/src/ffi.ts b/packages/fff-node/src/ffi.ts index 61080f42..e8dcb017 100644 --- a/packages/fff-node/src/ffi.ts +++ b/packages/fff-node/src/ffi.ts @@ -1616,8 +1616,9 @@ export function ffiGetHistoricalQuery( // is only supported as a top-level parameter. // // Batch contents are read through the C accessors (fff_watch_events_count / -// fff_watch_events_get_path / fff_watch_events_get_kind), so no struct -// layout knowledge lives on this side. +// fff_watch_events_get_path / fff_watch_events_get_kind / +// fff_watch_events_get_from_path), so no struct layout knowledge lives on +// this side. /** Map the C kind byte to the public WatchEventKind. */ function watchKindFromU8(kind: number): WatchEventKind { @@ -1628,6 +1629,8 @@ function watchKindFromU8(kind: number): WatchEventKind { return "modified"; case 2: return "removed"; + case 4: + return "renamed"; default: return "rescan"; } @@ -1685,10 +1688,22 @@ function consumeWatchBatch(address: number): WatchEvent[] { paramsType: [DataType.External, DataType.U32], paramsValue: [batchPtr, i], }) as unknown as number; - events.push({ + const event: WatchEvent = { path: readCString(path) ?? "", kind: watchKindFromU8(kind), - }); + }; + if (event.kind === "renamed") { + const from = load({ + library: LIBRARY_KEY, + funcName: "fff_watch_events_get_from_path", + retType: DataType.External, + paramsType: [DataType.External, DataType.U32], + paramsValue: [batchPtr, i], + }) as unknown as JsExternal; + const decoded = readCString(from); + if (decoded) event.from = decoded; + } + events.push(event); } load({ diff --git a/packages/fff-node/test/watch.mjs b/packages/fff-node/test/watch.mjs index 09ea7ace..82450b0c 100644 --- a/packages/fff-node/test/watch.mjs +++ b/packages/fff-node/test/watch.mjs @@ -11,7 +11,7 @@ import { after, before, describe, it, mock } from "node:test"; import { strict as assert } from "node:assert"; import { execFile } from "node:child_process"; -import { mkdtempSync, realpathSync, writeFileSync } from "node:fs"; +import { mkdtempSync, realpathSync, renameSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, sep } from "node:path"; import { promisify } from "node:util"; @@ -109,7 +109,7 @@ describe("fff-node watch", { concurrency: 1 }, () => { for (const event of deliveredEvents(callback)) { assert.equal(typeof event.path, "string"); assert.ok( - ["created", "modified", "removed", "rescan"].includes(event.kind), + ["created", "modified", "removed", "rescan", "renamed"].includes(event.kind), `unexpected kind: ${event.kind}`, ); } @@ -131,6 +131,41 @@ describe("fff-node watch", { concurrency: 1 }, () => { sub.value(); }); + it("a rename arrives as one renamed event carrying both paths", async () => { + const callback = mock.fn(); + const sub = finder.watch(callback); + assert.ok(sub.ok, `watch failed: ${!sub.ok ? sub.error : ""}`); + + const from = join(baseDir, "move-me.txt"); + const to = join(baseDir, "moved.txt"); + writeFileSync(from, "move me\n"); + // Let the create land so the rename lands in its own batch. + await waitFor(() => + deliveredEvents(callback).some((e) => e.path.endsWith("move-me.txt")), + ); + callback.mock.resetCalls(); + + renameSync(from, to); + + const renamed = await waitFor(() => + deliveredEvents(callback).find((e) => e.kind === "renamed" && e.path === to), + ); + assert.ok( + renamed, + `expected a renamed event, got: ${JSON.stringify(deliveredEvents(callback))}`, + ); + assert.equal(renamed.from, from); + // The move is one event, not a removal plus a creation. + assert.ok( + deliveredEvents(callback).every( + (e) => !(e.kind === "removed" && e.path === from), + ), + "a recognised rename must not also report the source as removed", + ); + + sub.value(); + }); + it("per-event consumption is a one-line loop over watch", async () => { const perEvent = mock.fn(); const sub = finder.watch("**/*.md", (events) => { diff --git a/packages/fff-python/README.md b/packages/fff-python/README.md index 61dd2624..5fd0d534 100644 --- a/packages/fff-python/README.md +++ b/packages/fff-python/README.md @@ -80,7 +80,7 @@ with FileFinder("/path/to/project") as finder: def on_change(events): for e in events: - print(e.kind, e.path) # created | modified | removed | rescan + print(e.kind, e.path) # created | modified | removed | renamed | rescan # Globs are relative to the project root; wildcard-free patterns resolve # inside the indexed tree — an existing directory watches its whole @@ -98,6 +98,10 @@ with FileFinder("/path/to/project") as finder: ... ``` +A `renamed` event carries both paths: `e.path` is the destination and +`e.from_path` the source. It replaces the removed/created pair, so a move is +one event. Moves in or out of the indexed tree stay a plain `created`/`removed`. + A `rescan` event means individual changes were lost (index overflow or an ignore-file change) — re-check anything you care about. diff --git a/packages/fff-python/src/fff/__init__.pyi b/packages/fff-python/src/fff/__init__.pyi index 44cee2f1..830c5752 100644 --- a/packages/fff-python/src/fff/__init__.pyi +++ b/packages/fff-python/src/fff/__init__.pyi @@ -147,7 +147,8 @@ class GrepCursor: class WatchEvent: path: str - kind: Literal["created", "modified", "removed", "rescan"] + kind: Literal["created", "modified", "removed", "rescan", "renamed"] + from_path: str | None def __repr__(self) -> str: ... class WatchSubscription: diff --git a/packages/fff-python/tests/test_watch.py b/packages/fff-python/tests/test_watch.py index 2e55c5d1..9ea9b355 100644 --- a/packages/fff-python/tests/test_watch.py +++ b/packages/fff-python/tests/test_watch.py @@ -117,6 +117,34 @@ def on_events(batch: list[WatchEvent]) -> None: assert ev.kind == "removed" +def test_watch_reports_renames_with_both_paths(finder: FileFinder, watch_dir: str) -> None: + events: list[WatchEvent] = [] + lock = threading.Lock() + + def on_events(batch: list[WatchEvent]) -> None: + with lock: + events.extend(batch) + + source = Path(watch_dir) / "docs" / "move-me.txt" + target = Path(watch_dir) / "docs" / "moved.txt" + with finder.watch("**/*.txt", on_events): + source.write_text("move me\n") + wait_for_event(events, lock, "move-me.txt") + with lock: + events.clear() + + source.rename(target) + ev = wait_for_event(events, lock, "moved.txt") + assert ev.kind == "renamed" + assert ev.path == str(target) + assert ev.from_path == str(source) + # The move is one event, not a removal plus a creation. + with lock: + assert not [ + e for e in events if e.kind == "removed" and e.path == str(source) + ] + + def test_multiple_subscriptions_are_filtered_independently( finder: FileFinder, watch_dir: str ) -> None: diff --git a/packages/pi-fff/src/index.ts b/packages/pi-fff/src/index.ts index 608d09fd..f3b87156 100644 --- a/packages/pi-fff/src/index.ts +++ b/packages/pi-fff/src/index.ts @@ -320,11 +320,7 @@ export default function fffExtension(pi: ExtensionAPI) { undefined; // flag (boolean) > env ("1"/"true", or "0"/"false") > default. - function resolveBoolOpt( - flagName: string, - envName: string, - fallback = false, - ): boolean { + function resolveBoolOpt(flagName: string, envName: string, fallback = false): boolean { const flag = pi.getFlag(flagName); if (typeof flag === "boolean") return flag; if (typeof flag === "string") return flag === "true" || flag === "1"; diff --git a/packages/shared/fff-api.ts b/packages/shared/fff-api.ts index dcd9f2ac..895f6d3e 100644 --- a/packages/shared/fff-api.ts +++ b/packages/shared/fff-api.ts @@ -293,14 +293,18 @@ export interface ScanProgress { * * rescan = internal OS buffers were overloaded, some events might be missing. * The `path` is going to be a folder needs to be rescanned + * + * renamed = the file was moved to the other path without changing the content */ -export type WatchEventKind = "created" | "modified" | "removed" | "rescan"; +export type WatchEventKind = "created" | "modified" | "removed" | "rescan" | "renamed"; /** A single filesystem change notification. */ export interface WatchEvent { - /** Absolute path of the affected file (base path to rescan if `kind ==rescan`) */ + /** Absolute path of the affected file (base path to rescan if `kind == rescan`) */ path: string; kind: WatchEventKind; + /** Absolute path the file moved from. Only set when `kind == "renamed"`. */ + from?: string; } /** Options for watch subscriptions. */ @@ -632,7 +636,7 @@ export interface FileFinderApi { * Patterns may be base-relative globs (./ works), exact paths inside the indexed * tree, or existing directories. An empty pattern watches the whole tree. * - * Events are debounced and submitted in batches per 100-ms window at most 128 events. + * Events are debounced over a 50-ms window and submitted in batches of at most 128 events. * Gitignored and other ignored files are never triggering watcher. */ watch(callback: WatchBatchCallback, options?: WatchOptions): Result; diff --git a/tests/test_cd_during_post_scan.lua b/tests/test_cd_during_post_scan.lua index 76aed949..71d5439f 100644 --- a/tests/test_cd_during_post_scan.lua +++ b/tests/test_cd_during_post_scan.lua @@ -33,8 +33,8 @@ fff_rust.restart_index_in_path(plugin_dir) local deadline = vim.uv.hrtime() + 30e9 -- 30s while true do vim.wait(500, function() return false end) - local ok, result = pcall(fff_rust.fuzzy_search_files, 'lib', 2, nil, 100, 3, 0, 10) - if ok and result and #result.items > 0 then + local searched, result = pcall(fff_rust.fuzzy_search_files, 'lib', 2, nil, 100, 3, 0, 10) + if searched and result and #result.items > 0 then print('PASS: :cd during post-scan did not crash (' .. #result.items .. ' results found)') break end