Skip to content

Fix hidden folder checking on Unix-based OS#574

Open
Tuupertunut wants to merge 1 commit intoUltraStar-Deluxe:masterfrom
Tuupertunut:fix/unix-hidden-files
Open

Fix hidden folder checking on Unix-based OS#574
Tuupertunut wants to merge 1 commit intoUltraStar-Deluxe:masterfrom
Tuupertunut:fix/unix-hidden-files

Conversation

@Tuupertunut
Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a bug where all paths that had a hidden folder as a parent were ignored, even if the whole software was inside a hidden folder. On Linux the default config is found inside $HOME/.config so the whole config folder was ignored, causing a wide range of issues. The fix is to only ignore hidden folders that are children of the path being searched.

As discussed in #538 it might still be wise to review usage of the file scanner, as many or maybe all usages may not actually need to ignore hidden files. However this PR fixes the acute issue.

Closes Issue(s)

#538, #569, maybe also some other Linux and MacOS issues.

Fixes a bug where all paths that had a hidden folder as a parent were ignored, even if the whole software was inside a hidden folder. On Linux the default config is found inside `$HOME/.config` so the whole config folder was ignored, causing a wide range of issues. The fix is to only ignore hidden folders that are children of the path being searched.
@basisbit basisbit requested a review from Copilot March 27, 2026 22:51
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes Unix hidden-folder filtering so that only hidden folders under the scanned root are excluded, preventing configs inside hidden roots (e.g., $HOME/.config) from being accidentally ignored.

Changes:

  • Passes the scan root (folder) into hidden-folder detection.
  • Updates hidden-folder detection logic to only consider folders relative to the scan root.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +71 to +72
string folderPath = Path.GetDirectoryName(filePath).Substring(rootPath.Length);
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.
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);
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.

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.
@Tuupertunut
Copy link
Copy Markdown
Contributor Author

What should I do with the AI review? It may point out potential bugs but it may also very well hallucinate some imaginary problems. The changes it suggest seem overly complicated. Should I answer it or what?

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

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

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
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants