Fixed .lmsignore sometimes not taken into account, fixes #868

src/libs/services/scanner/bench/CMakeLists.txt

@@ -1,5 +1,6 @@
 
 add_executable(bench-scanner
+	IgnoreRules.cpp
 	Lyrics.cpp
 	Scanner.cpp
 	TrackMetadataParser.cpp

src/libs/services/scanner/bench/IgnoreRules.cpp

@@ -0,0 +1,104 @@
+/*
+ * Copyright (C) 2025 Emeric Poupon
+ *
+ * This file is part of LMS.
+ *
+ * LMS is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * LMS is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with LMS.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <string>
+
+#include <benchmark/benchmark.h>
+
+#include "IgnoreRules.hpp"
+
+namespace lms::scanner::benchmarks
+{
+    namespace
+    {
+        std::filesystem::path makeDeepPath(std::size_t depth)
+        {
+            std::filesystem::path path;
+            for (std::size_t i{}; i < depth; ++i)
+                path /= "artist_" + std::to_string(i);
+
+            path /= "track.flac";
+            return path;
+        }
+
+        std::string makeManyRules(std::size_t count)
+        {
+            std::string content;
+            for (std::size_t i{}; i < count; ++i)
+                content += "*.rule_" + std::to_string(i) + "\n";
+
+            return content;
+        }
+
+        void BM_IgnoreRules_isIgnored_ShallowNoMatch(benchmark::State& state)
+        {
+            const IgnoreRules rules{ "*.nfo\ncovers/\n" };
+
+            for (auto _ : state)
+                benchmark::DoNotOptimize(rules.isIgnored("track.flac", IgnoreRules::IsDirectory{ false }));
+        }
+
+        void BM_IgnoreRules_isIgnored_ShallowMatch(benchmark::State& state)
+        {
+            const IgnoreRules rules{ "*.nfo\ncovers/\n" };
+
+            for (auto _ : state)
+                benchmark::DoNotOptimize(rules.isIgnored("track.nfo", IgnoreRules::IsDirectory{ false }));
+        }
+
+        // Worst case for the ancestor walk: no rule ever matches, so every
+        // ancestor of a deep path gets fully evaluated on every call.
+        void BM_IgnoreRules_isIgnored_DeepPath_NoMatch(benchmark::State& state)
+        {
+            const IgnoreRules rules{ "*.nfo\ncovers/\n" };
+            const std::filesystem::path path{ makeDeepPath(static_cast<std::size_t>(state.range(0))) };
+
+            for (auto _ : state)
+                benchmark::DoNotOptimize(rules.isIgnored(path, IgnoreRules::IsDirectory{ false }));
+        }
+
+        // Best case: the leaf itself matches, so the ancestor walk exits on its first iteration.
+        void BM_IgnoreRules_isIgnored_DeepPath_LeafMatch(benchmark::State& state)
+        {
+            const IgnoreRules rules{ "*.flac\n" };
+            const std::filesystem::path path{ makeDeepPath(static_cast<std::size_t>(state.range(0))) };
+
+            for (auto _ : state)
+                benchmark::DoNotOptimize(rules.isIgnored(path, IgnoreRules::IsDirectory{ false }));
+        }
+
+        // Combines a deep path with a large rule set: every level of the
+        // ancestor walk pays the full per-rule fnmatch fold.
+        void BM_IgnoreRules_isIgnored_DeepPath_ManyRules_NoMatch(benchmark::State& state)
+        {
+            const IgnoreRules rules{ makeManyRules(static_cast<std::size_t>(state.range(1))) };
+            const std::filesystem::path path{ makeDeepPath(static_cast<std::size_t>(state.range(0))) };
+
+            for (auto _ : state)
+                benchmark::DoNotOptimize(rules.isIgnored(path, IgnoreRules::IsDirectory{ false }));
+        }
+    } // namespace
+
+    BENCHMARK(BM_IgnoreRules_isIgnored_ShallowNoMatch);
+    BENCHMARK(BM_IgnoreRules_isIgnored_ShallowMatch);
+    BENCHMARK(BM_IgnoreRules_isIgnored_DeepPath_NoMatch)->Arg(4)->Arg(8);
+    BENCHMARK(BM_IgnoreRules_isIgnored_DeepPath_LeafMatch)->Arg(4)->Arg(8);
+    BENCHMARK(BM_IgnoreRules_isIgnored_DeepPath_ManyRules_NoMatch)->Args({ 4, 10 })->Args({ 8, 20 });
+
+} // namespace lms::scanner::benchmarks

src/libs/services/scanner/impl/IgnoreRules.cpp

@@ -21,6 +21,7 @@
 
 #include <fnmatch.h>
 
+#include <cassert>
 #include <fstream>
 
 namespace lms::scanner
@@ -79,9 +80,28 @@ namespace lms::scanner
 
     bool IgnoreRules::isIgnored(const std::filesystem::path& relativePath, IsDirectory isDir) const
     {
+        assert(relativePath.is_relative());
+
         if (_rules.empty())
             return false;
 
+        std::filesystem::path currentPath{ relativePath };
+        IsDirectory currentIsDir{ isDir };
+
+        while (!currentPath.empty())
+        {
+            if (matchesRules(currentPath, currentIsDir))
+                return true;
+
+            currentPath = currentPath.parent_path();
+            currentIsDir = IsDirectory{ true };
+        }
+
+        return false;
+    }
+
+    bool IgnoreRules::matchesRules(const std::filesystem::path& relativePath, IsDirectory isDir) const
+    {
         bool ignored{};
         for (const Rule& rule : _rules)
         {

src/libs/services/scanner/impl/IgnoreRules.hpp

@@ -56,6 +56,8 @@ namespace lms::scanner
             bool operator==(const Rule&) const = default;
         };
 
+        bool matchesRules(const std::filesystem::path& relativePath, IsDirectory isDir) const;
+
         std::vector<Rule> _rules;
 
     public:

src/libs/services/scanner/impl/steps/ScanStepCheckForRemovedFiles.cpp

@@ -19,6 +19,7 @@
 
 #include "ScanStepCheckForRemovedFiles.hpp"
 
+#include <algorithm>
 #include <deque>
 #include <filesystem>
 #include <span>
@@ -107,17 +108,22 @@ namespace lms::scanner
                     return false;
                 }
 
-                const auto isInActiveLibrary{ [&](const MediaLibraryInfo& lib) {
-                    if (!core::pathUtils::isPathInRootPath(p, lib.rootDirectory))
-                        return false;
-                    return lib.ignoreRules.isEmpty() || !lib.ignoreRules.isIgnored(std::filesystem::relative(p, lib.rootDirectory), IgnoreRules::IsDirectory{ false });
-                } };
-                if (std::none_of(std::cbegin(_settings.mediaLibraries), std::cend(_settings.mediaLibraries), isInActiveLibrary))
+                // media library root paths never overlap: at most one library can own this path
+                const auto itOwningLibrary{ std::find_if(std::cbegin(_settings.mediaLibraries), std::cend(_settings.mediaLibraries), [&](const MediaLibraryInfo& lib) {
+                    return core::pathUtils::isPathInRootPath(p, lib.rootDirectory);
+                }) };
+                if (itOwningLibrary == std::cend(_settings.mediaLibraries))
                 {
                     LMS_LOG(DBUPDATER, DEBUG, "Removing " << p << ": out of media directory");
                     return false;
                 }
 
+                if (itOwningLibrary->ignoreRules.isIgnored(std::filesystem::relative(p, itOwningLibrary->rootDirectory), IgnoreRules::IsDirectory{ false }))
+                {
+                    LMS_LOG(DBUPDATER, DEBUG, "Removing " << p << ": ignored by .lmsignore rules");
+                    return false;
+                }
+
                 if (!_scanners.select(p))
                 {
                     LMS_LOG(DBUPDATER, DEBUG, "Removing " << p << ": file format no longer handled");

src/libs/services/scanner/test/CMakeLists.txt

@@ -3,7 +3,7 @@ include(GoogleTest)
 add_executable(test-scanner
 	ArtistInfo.cpp
 	AudioFileUtils.cpp
-	IgnoreFilter.cpp
+	IgnoreRules.cpp
 	Lyrics.cpp
 	PlayList.cpp
 	ScannerStats.cpp

src/libs/services/scanner/test/IgnoreRules.cpp

@@ -30,6 +30,13 @@ namespace lms::scanner::tests
         EXPECT_FALSE(f.isIgnored("track.flac", IgnoreRules::IsDirectory{ false }));
     }
 
+    TEST(IgnoreRules, EmptyPath_NeverIgnored)
+    {
+        const IgnoreRules f{ "*.mp3\ncovers/\n" };
+        EXPECT_FALSE(f.isIgnored("", IgnoreRules::IsDirectory{ false }));
+        EXPECT_FALSE(f.isIgnored("", IgnoreRules::IsDirectory{ true }));
+    }
+
     TEST(IgnoreRules, CommentsAndBlanksOnly)
     {
         const IgnoreRules f{ "# this is a comment\n\n# another comment\n" };
@@ -79,7 +86,7 @@ namespace lms::scanner::tests
         const IgnoreRules f{ "covers*/\n" };
         EXPECT_TRUE(f.isIgnored("covers", IgnoreRules::IsDirectory{ true }));
         EXPECT_TRUE(f.isIgnored("covers_2024", IgnoreRules::IsDirectory{ true }));
-        EXPECT_TRUE(f.isIgnored("jazz/covers_hq", IgnoreRules::IsDirectory{ true })); // unanchored — matches at any depth
+        EXPECT_TRUE(f.isIgnored("jazz/covers_hq", IgnoreRules::IsDirectory{ true })); // unanchored: matches at any depth
         EXPECT_FALSE(f.isIgnored("notcovers", IgnoreRules::IsDirectory{ true }));
         EXPECT_FALSE(f.isIgnored("covers_2024", IgnoreRules::IsDirectory{ false })); // dirOnly
     }
@@ -112,29 +119,51 @@ namespace lms::scanner::tests
     TEST(IgnoreRules, DirOnly_AnchoredOnlyMatchesRoot)
     {
         const IgnoreRules f{ "/untagged/\n" };
-        EXPECT_TRUE(f.isIgnored("untagged", IgnoreRules::IsDirectory{ true }));       // root level — match
-        EXPECT_FALSE(f.isIgnored("jazz/untagged", IgnoreRules::IsDirectory{ true })); // nested — no match
-        EXPECT_FALSE(f.isIgnored("untagged", IgnoreRules::IsDirectory{ false }));     // file, not dir — no match
+        EXPECT_TRUE(f.isIgnored("untagged", IgnoreRules::IsDirectory{ true }));       // root level: match
+        EXPECT_FALSE(f.isIgnored("jazz/untagged", IgnoreRules::IsDirectory{ true })); // nested: no match
+        EXPECT_FALSE(f.isIgnored("untagged", IgnoreRules::IsDirectory{ false }));     // file, not dir: no match
     }
 
     TEST(IgnoreRules, DirOnly_UnanchoredMatchesAnyDepth)
     {
         const IgnoreRules f{ "covers/\n" };
-        EXPECT_TRUE(f.isIgnored("covers", IgnoreRules::IsDirectory{ true }));       // root — match
-        EXPECT_TRUE(f.isIgnored("jazz/covers", IgnoreRules::IsDirectory{ true }));  // nested — also match
-        EXPECT_TRUE(f.isIgnored("a/b/c/covers", IgnoreRules::IsDirectory{ true })); // deep — also match
-        EXPECT_FALSE(f.isIgnored("covers", IgnoreRules::IsDirectory{ false }));     // file, not dir — no match
+        EXPECT_TRUE(f.isIgnored("covers", IgnoreRules::IsDirectory{ true }));       // root: match
+        EXPECT_TRUE(f.isIgnored("jazz/covers", IgnoreRules::IsDirectory{ true }));  // nested: also match
+        EXPECT_TRUE(f.isIgnored("a/b/c/covers", IgnoreRules::IsDirectory{ true })); // deep: also match
+        EXPECT_FALSE(f.isIgnored("covers", IgnoreRules::IsDirectory{ false }));     // file, not dir: no match
     }
 
     TEST(IgnoreRules, FullPath_ExactDir)
     {
         const IgnoreRules f{ "jazz/covers\n" };
         EXPECT_TRUE(f.isIgnored("jazz/covers", IgnoreRules::IsDirectory{ true }));
-        EXPECT_FALSE(f.isIgnored("jazz/covers/foo", IgnoreRules::IsDirectory{ true })); // never reached in practice: scanner prunes jazz/covers/ first
-        EXPECT_FALSE(f.isIgnored("foo/jazz/covers", IgnoreRules::IsDirectory{ true })); // never reached in practice: scanner prunes jazz/covers/ first
+        EXPECT_TRUE(f.isIgnored("jazz/covers/foo", IgnoreRules::IsDirectory{ true }));  // descendant of an ignored dir
+        EXPECT_FALSE(f.isIgnored("foo/jazz/covers", IgnoreRules::IsDirectory{ true })); // unrelated path structure, no prefix matches
         EXPECT_FALSE(f.isIgnored("rock/covers", IgnoreRules::IsDirectory{ true }));
     }
 
+    TEST(IgnoreRules, AncestorDirIgnored_MatchesDescendantFile)
+    {
+        const IgnoreRules f{ "import\n" };
+        EXPECT_TRUE(f.isIgnored("import/song.mp3", IgnoreRules::IsDirectory{ false }));
+        EXPECT_TRUE(f.isIgnored("import/sub/song.mp3", IgnoreRules::IsDirectory{ false }));
+        EXPECT_FALSE(f.isIgnored("other/song.mp3", IgnoreRules::IsDirectory{ false }));
+    }
+
+    TEST(IgnoreRules, AncestorDirIgnored_AnchoredRuleMatchesNestedFile)
+    {
+        const IgnoreRules f{ "/import/\n" };
+        EXPECT_TRUE(f.isIgnored("import/song.mp3", IgnoreRules::IsDirectory{ false }));
+        EXPECT_FALSE(f.isIgnored("jazz/import/song.mp3", IgnoreRules::IsDirectory{ false })); // anchored to root only
+    }
+
+    TEST(IgnoreRules, AncestorDirIgnored_NegationDoesNotReinclude)
+    {
+        const IgnoreRules f{ "jazz/\n!jazz/keep.flac\n" };
+        EXPECT_TRUE(f.isIgnored("jazz/keep.flac", IgnoreRules::IsDirectory{ false })); // ancestor dir ignored: negation deeper inside has no effect
+        EXPECT_TRUE(f.isIgnored("jazz", IgnoreRules::IsDirectory{ true }));
+    }
+
     TEST(IgnoreRules, FullPath_GlobInDir)
     {
         const IgnoreRules f{ "jazz/*.nfo\n" };