Skip to content

Fix hidden folder checking on Unix-based OS #574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
achimmihca merged 4 commits into from
May 22, 2026
Merged

Fix hidden folder checking on Unix-based OS #574

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5717941
Fix hidden folder checking on Unix-based OS
Tuupertunut Mar 23, 2026
879a550
Fix error in FileScanner when path had trailing or duplicate slashes
Tuupertunut Apr 20, 2026
db0b71c
Add tests for hidden folders as parents
Tuupertunut May 7, 2026
bfd4f0e
Fix hidden folder checking also for DirectoryScanner
Tuupertunut May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions UltraStar Play/Packages/playshared/Runtime/FileScanner/FileScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public static List<string> GetFiles(string folder, FileScannerConfig config)
List<string> filteredResult = unfilteredResult
// Ignore hidden files and folders
.Where(filePath => (!config.ExcludeHiddenFiles || !IsHiddenFile(filePath))
&& (!config.ExcludeHiddenFolders || !IsInsideHiddenFolder(filePath)))
&& (!config.ExcludeHiddenFolders || !IsInsideHiddenFolder(filePath, folder)))
.ToList();

return filteredResult;
Expand All @@ -64,10 +64,11 @@ private static bool IsHiddenFile(string filePath)
return Path.GetFileName(filePath).StartsWith(".");
}

private static bool IsInsideHiddenFolder(string filePath)
private static bool IsInsideHiddenFolder(string filePath, string rootPath)
Copy link
Copy Markdown
Collaborator

@achimmihca achimmihca May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a common method for this. Currently, it is duplicated across DirectoryScanner and FileScanner.

Consider a HiddenFolderChecker or similar.

Copy link
Copy Markdown
Collaborator Author

@Tuupertunut Tuupertunut May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileScanner and DirectoryScanner seem to be duplicates of each other anyway, they could probably be merged.

{
// On Unix based operating systems, files and folders that start with a dot are hidden.
string folderPath = Path.GetDirectoryName(filePath);
return folderPath.Contains("\\.") || folderPath.Contains("/.");
// Only check for hidden folders that are children of the root path, not the root path itself or its parents.
string folderPath = Path.GetDirectoryName(filePath).Substring(rootPath.Length);
Copy link
Copy Markdown
Collaborator

@achimmihca achimmihca Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for tackling this.
I think this should work fine when rootPath is known to be a prefix of filePath.
But I am not sure whether this is the case when only looking at this code.

Consider to wrap rootPath in Path.GetFullPath(), to make it obvious that both are absolute paths.

Besides, performance is relevant here because there can be thousands of files (some users have +20k UltraStar songs). So only wrap it once, not inside the loop.

Please test performance with lots of dummy files. Ideally, performance should be similar as before. For example, this repo has several thousand txt files that you can use to test performance https://github.com/Asvarox/allkaraoke

Copy link
Copy Markdown
Collaborator

@achimmihca achimmihca Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review [...] The changes it suggest seem overly complicated.

I agree, seems over engineered. Also performance has to be tested.

Copy link
Copy Markdown
Collaborator Author

@Tuupertunut Tuupertunut Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure rootPath will always be a prefix of filePath. The Directory.GetFiles documentation says that

The returned file names are appended to the supplied path parameter.

I read that as a promise that the path parameter is always a prefix of any returned file paths. A quick test confirms that if you give it . it will return ./a and ./b. If you give it /home/user/dir it will return /home/user/dir/a and /home/user/dir/b. If you give it ../.. it will return ../../a and ../../b.

I agree it's not clear from the code that this is the case, so maybe a comment would clarify it. Performance should be about the same as before as Substring and StartsWith are cheap operations compared to filesystem checks. I considered adding an explicit check if rootPath is a prefix of filePath before doing Substring but that feels unnecessary as the documentation seems to already promise that.

Copy link
Copy Markdown
Collaborator Author

@Tuupertunut Tuupertunut Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment for that, as well as fixed a bug where Path.GetDirectoryName() removed trailing or duplicate slashes, resulting in changes in the string length. I also added a test case for that and the tests pass.

return folderPath.StartsWith(".") || folderPath.Contains("\\.") || folderPath.Contains("/.");
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Substring(rootPath.Length) is unsafe and can produce incorrect results (or throw) if rootPath isn’t a strict prefix of the directory path due to normalization differences (relative vs absolute paths, trailing separators, casing on case-insensitive FS, symlinks, etc.). Prefer computing a relative path via Path.GetRelativePath(rootPath, directoryPath) (after normalizing both with Path.GetFullPath) and only applying hidden-folder checks on that relative path; additionally handle cases where Path.GetDirectoryName(filePath) returns null.

Suggested change
string folderPath = Path.GetDirectoryName(filePath).Substring(rootPath.Length);
return folderPath.StartsWith(".") || folderPath.Contains("\\.") || folderPath.Contains("/.");
string directoryPath = Path.GetDirectoryName(filePath);
if (directoryPath == null)
{
return false;
}
string fullRootPath = Path.GetFullPath(rootPath);
string fullDirectoryPath = Path.GetFullPath(directoryPath);
string relativePath;
try
{
relativePath = Path.GetRelativePath(fullRootPath, fullDirectoryPath);
}
catch
{
// If we cannot compute a relative path, conservatively assume it's not inside a hidden folder under root.
return false;
}
// Exclude the root path itself (".") and any paths that are outside the root ("..").
if (relativePath == "." ||
relativePath == ".." ||
relativePath.StartsWith(".." + Path.DirectorySeparatorChar) ||
relativePath.StartsWith(".." + Path.AltDirectorySeparatorChar))
{
return false;
}
// Check for hidden folders (segments starting with a dot) in the relative path.
return relativePath.StartsWith(".")
|| relativePath.Contains(Path.DirectorySeparatorChar + ".")
|| relativePath.Contains(Path.AltDirectorySeparatorChar + ".");

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hidden-folder detection via Contains(\"/.\") / Contains(\"\\\\.\") is brittle and mixes separator conventions; it’s easier to reason about (and less error-prone) if you split the (relative) path into segments and check segment.StartsWith('.') for each segment. If you keep the current approach, StartsWith(\".\") likely won’t match when the relative path begins with a separator (e.g., \"/.config\"), so consider trimming leading directory separators before applying StartsWith.

Suggested change
return folderPath.StartsWith(".") || folderPath.Contains("\\.") || folderPath.Contains("/.");
// Normalize the relative folder path and check each segment for a leading dot.
if (string.IsNullOrEmpty(folderPath))
{
return false;
}
folderPath = folderPath.TrimStart(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
if (folderPath.Length == 0)
{
return false;
}
string[] segments = folderPath.Split(
new[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar },
StringSplitOptions.RemoveEmptyEntries);
return segments.Any(segment => segment.StartsWith("."));

Copilot uses AI. Check for mistakes.
}
}
Loading