beyond-all-reason / beyond-all-reason/RecoilEngine

Discuss: FindFilesStd / FindFiles path-result semantics (dataDir prefix in matches)

Open
#3,063 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
679
Forks
290
Avg merge
3d 2h
Merged PRs (30d)
40

Description

## Context

While reviewing the macOS draft PR #3060, one cross-platform (non-macOS-specific) change stood out as worth discussing on its own rather than riding along in a platform PR.

In `rts/System/FileSystem/FileSystem.cpp`, `Impl::FindFilesStd()` currently emits each match using the **full** iterated path:

```cpp
auto entryPathStr = entry.path().generic_u8string();
...
matches.emplace_back(Impl::StoreUTF8AsString(entryPathStr));
```

That full path includes the `dataDir` prefix that was only supposed to be used to *locate* the search directory. #3060 proposes reconstructing each result as `dir + ` instead, so the `dataDir` root never leaks into results:

```cpp
const std::string dirRelPrefix = FileSystem::ForwardSlashes(dirStr);
...
const std::u8string entryRelStr = entry.path().lexically_relative(dirFullPath).generic_u8string();
std::string entryPathStr = dirRelPrefix + Impl::StoreUTF8AsString(entryRelStr);
```

(plus a header doc comment formalising the contract: result is absolute iff `dir` is absolute; `dataDir` is never part of the result).

## Why this needs discussion, not a quiet port

This changes the **observable result of `FileSystem::FindFiles` for every caller on every platform**, not just macOS. It's either:

- a genuine latent bug fix (callers that pass a non-empty `dataDir` getting the root spliced into their results), or
- a behaviour change that could regress callers that currently rely on (or are unaffected by) the existing output.

Questions to resolve before adopting:

1. Is the `dataDir`-prefix-in-results behaviour actually wrong today, or do all current callers pass `dataDir = ""` (so it never manifests)?
2. Which call sites would change output, and are any of them sensitive (archive scanning, VFS path matching, Lua `VFS.DirList`, etc.)?
3. Should this be paired with a small test covering both the relative-`dir` and absolute-`dir` cases described in the proposed header contract?

Flagging here so it can be decided on its merits independently of the macOS bring-up. The other clearly-portable bits of #3060 (Mac crash-handler arm64 symbolication, the `` guard) are being split into their own focused PRs; the `ArchiveScanner` empty-pool guard from #3060 is already in master via #2961.

Ref: #3060

Contributor guide

Open the contributing guide

Research direction

Start in rts/System/FileSystem/FileSystem.cpp at Impl::FindFilesStd(), then trace the callers named in the issue, including archive scanning, VFS path matching, and Lua VFS.DirList. Compare relative- and absolute-dir behavior with and without dataDir, review the proposed contract from #3060, and determine whether a focused test should accompany the decision.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
operating-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.