You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-ups from the review of #85 (both noted as non-blocking there — this issue tracks them). Neither is a safety problem: both currently fail safe by disqualifying the selective route more often than necessary, they just limit how much of #83 the GO_TESTDATA route delivers in real repos.
1. The bare "testdata" reference token over-disqualifies the route
In goPackageTestDataOwnerFor (src/repo/impact.ts), the disqualification check builds its patterns from referenceTokens(dataDir) for p/testdata, and that token list includes the bare testdata literal. With ancestors: true, contentNamesPath matches a path-less word, so any other package containing the very common idioms
disqualifies p's route — even though in package q, a bare testdata resolves to q/testdata, which has nothing to do with p/testdata.
In a repo where two or more packages each use their own testdata, the selective route would rarely be chosen (always falling back), failing safe but wasting the analysis.
Suggested direction (from the review of #85): resolve a bare testdata token against the reader's own package directory and treat it as a reference to that package's testdata only. A slash-bearing reference (p/testdata/…) should keep its current treatment, since that genuinely names another package's tree.
2. Reader scans are repeated per changed file — memoize per dataDir
Every changed file under the same testdata directory runs the same disqualifies scan over goReadersByPackage and goOutsideReaders again: same readers, same contents, same result. On a golden-file refresh that touches many files in one tree, the scan is repeated for each.
Suggested direction: cache the scan outcome keyed by the owning dataDir (valid for one analysis run, keeping the current readRepositoryFile single-read semantics), so a large refresh only pays once per tree.
Happy to take these in a separate PR if a maintainer confirms scope.
Activity