deno icon indicating copy to clipboard operation
deno copied to clipboard

fix: Duplicate Paths in include Patterns

Open yazan-abdalrahman opened this issue 1 year ago • 4 comments

Prevents adding the base path and duplicate entries when using "." in include patterns. Ensures unique paths are added to PathOrPatternSet in FileFlags::as_file_patterns.

fix #25949

yazan-abdalrahman avatar Oct 02 '24 13:10 yazan-abdalrahman

@nathanwhit @dsherret @bartlomieju Please check this PR.

image

yazan-abdalrahman avatar Oct 02 '24 13:10 yazan-abdalrahman

@dsherret @bartlomieju @lucacasonato

Hello, I wrote this solution for removing the base path from include test Paths. I also eliminated duplicate paths from it.

impl FileFlags {
  pub fn as_file_patterns(
    &self,
    base: &Path,
  ) -> Result<FilePatterns, AnyError> {
    let include = if self.include.is_empty() {
      None
    } else {
      let path_or_pattern_set =
        PathOrPatternSet::from_include_relative_path_or_patterns(
          base,
          &self.include,
        )?;

      let deduped_set = FileFlags::remove_duplicates(path_or_pattern_set);
      let final_set = FileFlags::remove_base_path(deduped_set, base);
      if final_set.inner().is_empty() {
        None
      } else {
        Some(final_set)
      }
    };

    let exclude = PathOrPatternSet::from_exclude_relative_path_or_patterns(
      base,
      &self.ignore,
    )?;

    Ok(FilePatterns {
      include,
      exclude,
      base: base.to_path_buf(),
    })
  }

  fn remove_duplicates(pattern_set: PathOrPatternSet) -> PathOrPatternSet {
    let mut seen_paths: HashSet<PathBuf> = HashSet::new();
    let filtered_patterns: Vec<PathOrPattern> = pattern_set
      .inner()
      .iter()
      .filter(|pattern| match pattern {
        PathOrPattern::Path(path) | PathOrPattern::NegatedPath(path) => {
          seen_paths.insert(path.clone())
        }
        _ => true,
      })
      .cloned()
      .collect();

    PathOrPatternSet::new(filtered_patterns)
  }

  fn remove_base_path(
    pattern_set: PathOrPatternSet,
    base: &Path,
  ) -> PathOrPatternSet {
    let filtered_patterns: Vec<PathOrPattern> = pattern_set
      .inner()
      .iter()
      .filter(|pattern| match pattern {
        PathOrPattern::Path(path) | PathOrPattern::NegatedPath(path) => {
          path != base
        }
        _ => true,
      })
      .cloned()
      .collect();

    PathOrPatternSet::new(filtered_patterns)
  }
}

But it failed these tests:

integration_tests watcher::bench_watch_basic
integration_tests watcher::test_watch_basic
integration_tests watcher::test_watch_doc
PS D:\yad\deno\ola> cargo test --package cli_tests --test integration_tests watcher::test_watch_basic --features="run"  -- --exact -- --test-threads=100000;
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.45s
     Running integration/mod.rs (D:\yad\deno\target\debug\deps\integration_tests-d941e45dc0f97950.exe)

running 1 test
test watcher::test_watch_basic has been running for over 60 seconds

After debugging, I discovered that this test failed because it uses a temp directory, so thebaseshould be in include test paths , Otherwise, it fails as it used an async function without await, so the test would still be stuck in running.

So, I rewrote the solution just to remove duplicated paths and exclude .  from include test paths to resolve the main case of the issue.

      let mut unique_patterns = Vec::new();
      for pattern in &self.include {
        if pattern != "." && !unique_patterns.contains(pattern) {
          unique_patterns.push(pattern.clone());
        }
      }

So please decide which solution is correct.

yazan-abdalrahman avatar Oct 07 '24 13:10 yazan-abdalrahman

@nathanwhit Please review this change.

yazan-abdalrahman avatar Oct 09 '24 13:10 yazan-abdalrahman

@nathanwhit Please review this change.

yazan-abdalrahman avatar Oct 21 '24 12:10 yazan-abdalrahman

@dsherret Please review this change.

yazan-abdalrahman avatar Nov 12 '24 08:11 yazan-abdalrahman

Thanks for the PR but closing because old. If this is still an issue please reopen.

ry avatar May 09 '25 00:05 ry