mirror of
https://github.com/openai/codex.git
synced 2026-11-11 21:10:39 +00:00
Deduplicate file search results from overlapping roots (#60845)
## Why Repeated or nested search roots can cause the same path to consume multiple result slots and inflate the total match count. ## What changed Skip duplicate walk roots and deduplicate paths within overlapping subtrees before sending them to the matcher. Keep separate walks for nested roots so their ignore rules can still expose different entries. ## Testing Add a regression test covering repeated parent and child roots in both orders, verifying that two distinct matching files fill a two-result limit and produce a total match count of two. GitOrigin-RevId: 86483e572cb3d4dd69ec335fc2d27916301e1332
This commit is contained in:
@@ -14,6 +14,7 @@ use nucleo::Utf32String;
|
||||
use nucleo::pattern::CaseMatching;
|
||||
use nucleo::pattern::Normalization;
|
||||
use serde::Serialize;
|
||||
use std::collections::HashSet;
|
||||
use std::num::NonZero;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
@@ -430,15 +431,30 @@ fn walker_worker(
|
||||
override_matcher: Option<ignore::overrides::Override>,
|
||||
injector: Injector<IndexedEntry>,
|
||||
) {
|
||||
let Some(first_root) = inner.search_directories.first() else {
|
||||
let roots = &inner.search_directories;
|
||||
let Some((first_root, other_roots)) = roots.split_first() else {
|
||||
let _ = inner.work_tx.send(WorkSignal::WalkComplete);
|
||||
return;
|
||||
};
|
||||
|
||||
let mut walk_builder = WalkBuilder::new(first_root);
|
||||
for root in inner.search_directories.iter().skip(1) {
|
||||
walk_builder.add(root);
|
||||
let mut walk_roots = HashSet::from([first_root.as_path()]);
|
||||
for root in other_roots {
|
||||
if walk_roots.insert(root.as_path()) {
|
||||
walk_builder.add(root);
|
||||
}
|
||||
}
|
||||
// Keep both walks for nested roots: ignore rules can expose different entries.
|
||||
// Only those subtrees need to remember paths already sent to the matcher.
|
||||
let seen_paths: Vec<_> = roots
|
||||
.iter()
|
||||
.map(|root| {
|
||||
root.ancestors()
|
||||
.skip(1)
|
||||
.any(|parent| walk_roots.contains(parent))
|
||||
.then(|| Mutex::new(HashSet::new()))
|
||||
})
|
||||
.collect();
|
||||
walk_builder
|
||||
.threads(inner.threads)
|
||||
// Allow hidden entries.
|
||||
@@ -465,7 +481,7 @@ fn walker_worker(
|
||||
walker.run(|| {
|
||||
const CHECK_INTERVAL: usize = 1024;
|
||||
let mut n = 0;
|
||||
let search_directories = inner.search_directories.clone();
|
||||
let seen_paths = &seen_paths;
|
||||
let injector = injector.clone();
|
||||
let cancelled = inner.cancelled.clone();
|
||||
let shutdown = inner.shutdown.clone();
|
||||
@@ -479,7 +495,13 @@ fn walker_worker(
|
||||
if path.to_str().is_none() {
|
||||
return ignore::WalkState::Continue;
|
||||
}
|
||||
if let Some((root_idx, relative_path)) = get_file_path(path, &search_directories) {
|
||||
if let Some((root_idx, relative_path)) = get_file_path(path, roots)
|
||||
&& seen_paths[root_idx].as_ref().is_none_or(|seen| {
|
||||
seen.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.insert(PathBuf::from(relative_path))
|
||||
})
|
||||
{
|
||||
let match_type = match entry.file_type() {
|
||||
Some(file_type) if file_type.is_dir() => MatchType::Directory,
|
||||
_ => MatchType::File,
|
||||
|
||||
@@ -91,6 +91,42 @@ fn matching_filenames_survive_the_path_result_limit() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn overlapping_roots_share_result_slots_by_path() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let parent = dir.path().to_path_buf();
|
||||
let child = parent.join("sub");
|
||||
fs::create_dir(&child).unwrap();
|
||||
let expected = [parent.clone(), child.clone()].map(|root| (root, PathBuf::from("target.txt")));
|
||||
for (root, path) in &expected {
|
||||
fs::write(root.join(path), "").unwrap();
|
||||
}
|
||||
|
||||
for roots in [
|
||||
vec![parent.clone(), child.clone(), parent.clone()],
|
||||
vec![child.clone(), parent, child],
|
||||
] {
|
||||
let result = run(
|
||||
"target",
|
||||
roots,
|
||||
FileSearchOptions {
|
||||
limit: NonZero::new(2).unwrap(),
|
||||
..Default::default()
|
||||
},
|
||||
/*cancel_flag*/ None,
|
||||
)
|
||||
.unwrap();
|
||||
let mut paths: Vec<_> = result
|
||||
.matches
|
||||
.into_iter()
|
||||
.map(|m| (m.root, m.path))
|
||||
.collect();
|
||||
paths.sort();
|
||||
assert_eq!(result.total_match_count, expected.len());
|
||||
assert_eq!(paths, expected);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn file_name_from_path_uses_basename() {
|
||||
assert_eq!(file_name_from_path("foo/bar.txt"), "bar.txt");
|
||||
|
||||
Reference in New Issue
Block a user