fix(common): list symlinked files when scanning directories - #136
Draft
Hathor875 wants to merge 1 commit into
Draft
fix(common): list symlinked files when scanning directories#136Hathor875 wants to merge 1 commit into
Hathor875 wants to merge 1 commit into
Conversation
readdir() reports a symlink as DT_LNK. The POSIX listing loop in NSDirectory::GetFiles2() classified only DT_REG, DT_DIR and DT_UNKNOWN, so DT_LNK entries fell through unclassified and were dropped. Fonts installed as symlinks were therefore never picked up — the symptom reported in Euro-Office#128, which hits NixOS hardest because its font directories are largely symlinks into the store, but which also affects a symlinked font in ~/.local/share/fonts on Ubuntu. DT_LNK now goes through stat(), which follows the link, so the target decides. Only regular-file targets are listed. Symlinked directories are deliberately left unclassified: DeleteDirectory() is built on GetFiles()/GetDirectories() and recurses over what they return, so reporting a symlinked directory would make it delete the link target's contents instead of just removing the link. A directory symlink passed to GetFiles() directly is unaffected — opendir() follows it — so the common case of a font directory that is itself a symlink keeps working. Adds DesktopEditor/common/test, a new GoogleTest suite registered with CTest. It builds its tree at run time (regular file, symlink to it, dangling symlink, self-referencing symlink), so no fixtures are committed. The suite covers the fix and pins the deliberate limitation: one case fails if a future change starts following directory symlinks. Verified on Linux by removing the fix and restoring it: without it the suite reports exactly one failure (symlinked_file_is_listed — "link.bin" missing), with it 5/5 pass, repeatably. The other built CTest suites still pass. Known, not addressed here: the MAC/_IOS variants of these loops have the same gap and additionally lack the DT_UNKNOWN fallback, and CopyDirectory() likewise skips symlinked files. Neither is testable on this machine. Signed-off-by: Krzysztof Cieślik <132496025+Hathor875@users.noreply.github.com> Assisted-by: Claude Code:claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #128.
What was wrong
readdir()reports a symlink asDT_LNK. The POSIX listing loop inNSDirectory::GetFiles2()(DesktopEditor/common/Directory.cpp) classified onlyDT_REG,DT_DIRandDT_UNKNOWN, soDT_LNKentries fell through unclassified and were dropped. Fonts installed as symlinks were therefore never picked up.The change
DT_LNKnow goes throughstat(), which follows the link, so the target decides. Only regular-file targets are listed.What this deliberately does not do
Symlinked directories are still not reported.
DeleteDirectory()is built onGetFiles()/GetDirectories()and recurses over what they return — reporting a symlinked directory there would make it delete the link target's contents instead of just removing the link. That is a data-loss regression, so the limitation is intentional and pinned by a test (symlinked_directory_is_not_listed), with the reasoning in a comment next to the fix.This is narrower than it sounds: a directory symlink passed to
GetFiles()directly is scanned normally, becauseopendir()follows it. Only symlinked subdirectories discovered during recursion are skipped. There is a test for that too.So #128 stays open for the symlinked-subdirectory half.
Tests
Adds
DesktopEditor/common/test, a new GoogleTest suite registered with CTest — the first unit test forDesktopEditor/common. It builds its tree at run time (regular file, symlink to it, dangling symlink, self-referencing symlink), so no fixtures are committed.Five cases: the fix itself, the deliberate limitation, the symlinked-root case, a dangling symlink, and a self-referencing symlink that would blow up if a future change started following directory symlinks.
Verification
Red/green was verified in both directions — the fix was removed and restored:
Repeated three times with identical results, and the other built CTest suites still pass (6/6). Built on Fedora with
-DEO_BUILD_TESTS=ON.Two things I checked before proposing this, in case they come up in review:
NSFile::CFileBinary::Remove()usesstd::remove(), which unlinks the link, not the target — soDeleteDirectory()now also cleans up symlinked files instead of leaving them behind.CFontList::Add()deduplicates by font name + bold + italic, so a symlink sitting next to its target does not produce a duplicate font entry.Known, not addressed here
MAC/_IOSvariants of these loops have the same gap and additionally lack theDT_UNKNOWNfallback.CopyDirectory()likewise skips symlinked files.Neither is testable on this machine, so I left both alone rather than shipping unverified code.
AI assistance
Prepared with AI assistance (Claude Code,
claude-opus-5); the commit carries anAssisted-by:trailer.