Fixup #14021 (Add file mode specifier to PathMatch) (danmar#7689)

This is an addendum to danmar#7645 that adds the ability to specify
directory-only matching in PathMatch patterns with a trailing slash. I
originally didn't add this because I wanted to keep PathMatch purely
syntactic, and this feature seemed like it would require filesystem
calls in the PathMatch code.

This PR solves that problem by lifting the responsibility of file mode
checking to the caller, thus keeping PathMatch purely syntactic while
still supporting directory-only matching.

Previously `/test/foo/` would match `/test/foo` even if `foo` is a
regular file. With this change the caller can specify the file mode of
the file named by the provided path, and `/test/foo` will only match
when the file mode is directory.

The semantics of patterns that do not have a trailing slash is
unchanged, e.g. `/test/foo` still matches `/test/foo/` and
`/test/foo/bar.cpp`, regardless of the file mode.

Also adds some more tests.

Author: glankk

cli/filelister.cpp

@@ -129,7 +129,7 @@ static std::string addFiles2(std::list<FileWithDetails>&files, const std::string
             } else {
                 // Directory
                 if (recursive) {
-                    if (!ignored.match(fname)) {
+                    if (!ignored.match(fname, PathMatch::Filemode::directory)) {
                         std::list<FileWithDetails> filesSorted;
 
                         std::string err = addFiles2(filesSorted, fname, extra, recursive, ignored);
@@ -241,7 +241,7 @@ static std::string addFiles2(std::list<FileWithDetails> &files,
 #endif
         if (path_is_directory) {
             if (recursive) {
-                if (!ignored.match(new_path)) {
+                if (!ignored.match(new_path, PathMatch::Filemode::directory)) {
                     std::string err = addFiles2(files, new_path, extra, recursive, ignored, debug);
                     if (!err.empty()) {
                         return err;

lib/pathmatch.cpp

@@ -30,29 +30,44 @@ PathMatch::PathMatch(std::vector<std::string> patterns, std::string basepath, Sy
     mPatterns(std::move(patterns)), mBasepath(std::move(basepath)), mSyntax(syntax)
 {}
 
-bool PathMatch::match(const std::string &path) const
+bool PathMatch::match(const std::string &path, Filemode mode) const
 {
     return std::any_of(mPatterns.cbegin(), mPatterns.cend(), [=] (const std::string &pattern) {
-        return match(pattern, path, mBasepath, mSyntax);
+        return match(pattern, path, mBasepath, mode, mSyntax);
     });
 }
 
-bool PathMatch::match(const std::string &pattern, const std::string &path, const std::string &basepath, Syntax syntax)
+bool PathMatch::match(const std::string &pattern, const std::string &path, const std::string &basepath, Filemode mode, Syntax syntax)
 {
+    /* Fast paths for common patterns */
     if (pattern.empty())
         return false;
 
     if (pattern == "*" || pattern == "**")
         return true;
 
-    /* A "real" path is absolute or relative to the base path. A pattern that isn't "real" can match at any
+    /* If the pattern ends with a path separator it matches only directories. If the path names a regular file then
+     * the last path component can't match. */
+    bool dir_mismatch = PathIterator::issep(pattern.back(), syntax) && mode != Filemode::directory;
+
+    if (!dir_mismatch && pattern == path)
+        return true;
+
+    /* A "real" pattern is absolute or relative to the base path. A pattern that isn't real can match at any
      * path component boundary. */
     bool real = Path::isAbsolute(pattern) || isRelativePattern(pattern);
 
     /* Pattern iterator */
     PathIterator s = PathIterator::fromPattern(pattern, basepath, syntax);
     /* Path iterator */
     PathIterator t = PathIterator::fromPath(path, basepath, syntax);
+
+    if (dir_mismatch) {
+        /* Final compponent can't match, so skip it. */
+        while (*t != '\0' && *t != '/')
+            ++t;
+    }
+
     /* Pattern restart position */
     PathIterator p = s;
     /* Path restart position */
@@ -72,8 +87,6 @@ bool PathMatch::match(const std::string &pattern, const std::string &path, const
                 slash = true;
                 ++s;
             }
-            /* Add backtrack for matching zero characters */
-            b.emplace(s.getpos(), t.getpos());
             while (*t != '\0' && (slash || *t != '/')) {
                 if (*s == *t) {
                     /* Could stop here, but do greedy match and add

lib/pathmatch.h

@@ -54,8 +54,11 @@
  * - Otherwise:
  *   - Match all files where the pattern matches any part of the file's canonical absolute path up until a
  *     path separator or the end of the pathname, and the matching part directly follows a path separator.
+ * - If a pattern ends with a path separator before canonicalization then the final path component of the pattern
+ *   only matches the final path component of the file path if the file is a directory (specified by the file mode
+ *   parameter).
  *
- * TODO: Handle less common windows windows syntaxes:
+ * TODO: Handle less common windows syntaxes:
  *   - Drive-specific relative path: C:dir\foo.cpp
  *   - Root-relative path: \dir\foo.cpp
  **/
@@ -87,6 +90,17 @@ class CPPCHECKLIB PathMatch {
     static constexpr Syntax platform_syntax = Syntax::unix;
 #endif
 
+    /**
+     * @brief File mode of a file being matched
+     *
+     * regular: File is a regular file.
+     * directory: File is a directory.
+     */
+    enum class Filemode : std::uint8_t {
+        regular,
+        directory,
+    };
+
     /**
      * The constructor.
      *
@@ -100,20 +114,22 @@ class CPPCHECKLIB PathMatch {
      * @brief Match path against list of patterns.
      *
      * @param path Path to match.
+     * @param mode The file mode of the file named by the path.
      * @return true if any of the masks match the path, false otherwise.
      */
-    bool match(const std::string &path) const;
+    bool match(const std::string &path, Filemode mode = Filemode::regular) const;
 
     /**
      * @brief Match path against a single pattern.
      *
      * @param pattern Pattern to use.
      * @param path Path to match.
      * @param basepath Path to which the pattern and path is relative, when applicable.
+     * @param mode The file mode of the file named by the path.
      * @param syntax Path syntax.
      * @return true if the pattern matches the path, false otherwise.
      */
-    static bool match(const std::string &pattern, const std::string &path, const std::string &basepath = std::string(), Syntax syntax = platform_syntax);
+    static bool match(const std::string &pattern, const std::string &path, const std::string &basepath = std::string(), Filemode mode = Filemode::regular, Syntax syntax = platform_syntax);
 
     /**
      * @brief Check if a pattern is a relative path name.
@@ -209,12 +225,6 @@ class PathMatch::PathIterator {
     explicit PathIterator(const char *path_a = nullptr, const char *path_b = nullptr, Syntax syntax = platform_syntax) :
         mStart{path_a, path_b}, mSyntax(syntax)
     {
-        const auto issep = [syntax] (char c) {
-            return c == '/' || (syntax == Syntax::windows && c == '\\');
-        };
-        const auto isdrive = [] (char c) {
-            return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
-        };
 
         for (int i = 0; i < 2; i++) {
             const char *&p = mEnd[i];
@@ -225,19 +235,19 @@ class PathMatch::PathIterator {
 
             if (mPos.l == 0) {
                 /* Check length of root component */
-                if (issep(p[0])) {
+                if (issep(p[0], syntax)) {
                     mRootLength++;
-                    if (syntax == Syntax::windows && issep(p[1])) {
+                    if (syntax == Syntax::windows && issep(p[1], syntax)) {
                         mRootLength++;
                         if (p[2] == '.' || p[2] == '?') {
                             mRootLength++;
-                            if (issep(p[3]))
+                            if (issep(p[3], syntax))
                                 mRootLength++;
                         }
                     }
                 } else if (syntax == Syntax::windows && isdrive(p[0]) && p[1] == ':') {
                     mRootLength += 2;
-                    if (issep(p[2]))
+                    if (issep(p[2], syntax))
                         mRootLength++;
                 }
                 p += mRootLength;
@@ -308,6 +318,18 @@ class PathMatch::PathIterator {
         return str;
     }
 
+    /* Syntax helper, check if a character is a path separator */
+    static bool issep(char c, Syntax syntax)
+    {
+        return c == '/' || (syntax == Syntax::windows && c == '\\');
+    }
+
+    /* Syntax helper, check if a chracter is a drive letter */
+    static bool isdrive(char c)
+    {
+        return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
+    }
+
 private:
     /* Read the current character */
     char current() const

lib/suppressions.cpp

@@ -401,7 +401,7 @@ SuppressionList::Suppression::Result SuppressionList::Suppression::isSuppressed(
             if (!thisAndNextLine || lineNumber + 1 != errmsg.lineNumber)
                 return Result::None;
         }
-        if (!fileName.empty() && fileName != errmsg.getFileName() && !PathMatch::match(fileName, errmsg.getFileName()))
+        if (!fileName.empty() && !PathMatch::match(fileName, errmsg.getFileName()))
             return Result::None;
         if (hash > 0 && hash != errmsg.hash)
             return Result::Checked;

test/testpathmatch.cpp

@@ -30,6 +30,8 @@ class TestPathMatch : public TestFixture {
 private:
     static constexpr auto unix = PathMatch::Syntax::unix;
     static constexpr auto windows = PathMatch::Syntax::windows;
+    static constexpr auto ifreg = PathMatch::Filemode::regular;
+    static constexpr auto ifdir = PathMatch::Filemode::directory;
 #ifdef _WIN32
     const std::string basepath{"C:\\test"};
 #else
@@ -73,7 +75,8 @@ class TestPathMatch : public TestFixture {
         TEST_CASE(filemaskpath3);
         TEST_CASE(filemaskpath4);
         TEST_CASE(mixedallmatch);
-        TEST_CASE(glob);
+        TEST_CASE(glob1);
+        TEST_CASE(glob2);
         TEST_CASE(globstar1);
         TEST_CASE(globstar2);
         TEST_CASE(pathiterator);
@@ -85,16 +88,16 @@ class TestPathMatch : public TestFixture {
     }
 
     void emptymaskpath1() const {
-        ASSERT(!emptyMatcher.match("src/"));
+        ASSERT(!emptyMatcher.match("src/", ifdir));
     }
 
     void emptymaskpath2() const {
-        ASSERT(!emptyMatcher.match("../src/"));
+        ASSERT(!emptyMatcher.match("../src/", ifdir));
     }
 
     void emptymaskpath3() const {
-        ASSERT(!emptyMatcher.match("/home/user/code/src/"));
-        ASSERT(!emptyMatcher.match("d:/home/user/code/src/"));
+        ASSERT(!emptyMatcher.match("/home/user/code/src/", ifdir));
+        ASSERT(!emptyMatcher.match("d:/home/user/code/src/", ifdir));
     }
 
     // Test PathMatch containing "src/"
@@ -103,17 +106,20 @@ class TestPathMatch : public TestFixture {
     }
 
     void onemasksamepath() const {
-        ASSERT(srcMatcher.match("src/"));
+        ASSERT(srcMatcher.match("src/", ifdir));
+        ASSERT(!srcMatcher.match("src/", ifreg));
     }
 
     void onemasksamepathdifferentslash() const {
         PathMatch srcMatcher2({"src\\"}, basepath, windows);
-        ASSERT(srcMatcher2.match("src/"));
+        ASSERT(srcMatcher2.match("src/", ifdir));
+        ASSERT(!srcMatcher2.match("src/", ifreg));
     }
 
     void onemasksamepathdifferentcase() const {
         PathMatch match({"sRc/"}, basepath, windows);
-        ASSERT(match.match("srC/"));
+        ASSERT(match.match("srC/", ifdir));
+        ASSERT(!match.match("srC/", ifreg));
     }
 
     void onemasksamepathwithfile() const {
@@ -125,8 +131,8 @@ class TestPathMatch : public TestFixture {
         const std::string shorterToMatch("src/");
         ASSERT(shorterToMatch.length() < longerExclude.length());
         PathMatch match({longerExclude});
-        ASSERT(match.match(longerExclude));
-        ASSERT(!match.match(shorterToMatch));
+        ASSERT(match.match(longerExclude, ifdir));
+        ASSERT(!match.match(shorterToMatch, ifdir));
     }
 
     void onemaskdifferentdir1() const {
@@ -146,40 +152,43 @@ class TestPathMatch : public TestFixture {
     }
 
     void onemasklongerpath1() const {
-        ASSERT(srcMatcher.match("/tmp/src/"));
-        ASSERT(srcMatcher.match("d:/tmp/src/"));
+        ASSERT(srcMatcher.match("/tmp/src/", ifdir));
+        ASSERT(srcMatcher.match("d:/tmp/src/", ifdir));
     }
 
     void onemasklongerpath2() const {
-        ASSERT(srcMatcher.match("src/module/"));
+        ASSERT(srcMatcher.match("src/module/", ifdir));
     }
 
     void onemasklongerpath3() const {
-        ASSERT(srcMatcher.match("project/src/module/"));
+        ASSERT(srcMatcher.match("project/src/module/", ifdir));
     }
 
     void onemaskcwd() const {
-        ASSERT(srcMatcher.match("./src"));
+        ASSERT(srcMatcher.match("./src", ifdir));
     }
 
     void twomasklongerpath1() const {
         PathMatch match({ "src/", "module/" });
-        ASSERT(!match.match("project/"));
+        ASSERT(!match.match("project/", ifdir));
     }
 
     void twomasklongerpath2() const {
         PathMatch match({ "src/", "module/" });
-        ASSERT(match.match("project/src/"));
+        ASSERT(match.match("project/src/", ifdir));
+        ASSERT(!match.match("project/src/", ifreg));
     }
 
     void twomasklongerpath3() const {
         PathMatch match({ "src/", "module/" });
-        ASSERT(match.match("project/module/"));
+        ASSERT(match.match("project/module/", ifdir));
+        ASSERT(!match.match("project/module/", ifreg));
     }
 
     void twomasklongerpath4() const {
         PathMatch match({ "src/", "module/" });
-        ASSERT(match.match("project/src/module/"));
+        ASSERT(match.match("project/src/module/", ifdir));
+        ASSERT(match.match("project/src/module/", ifreg));
     }
 
     // Test PathMatch containing "foo.cpp"
@@ -224,11 +233,12 @@ class TestPathMatch : public TestFixture {
     void mixedallmatch() const { // #13570
         // when trying to match a directory against a directory entry it erroneously modified a local variable also used for file matching
         PathMatch match({ "tests/", "file.c" });
-        ASSERT(match.match("tests/"));
+        ASSERT(match.match("tests/", ifdir));
+        ASSERT(!match.match("tests/", ifreg));
         ASSERT(match.match("lib/file.c"));
     }
 
-    void glob() const {
+    void glob1() const {
         PathMatch match({"test?.cpp"});
         ASSERT(match.match("test1.cpp"));
         ASSERT(match.match("src/test1.cpp"));
@@ -237,12 +247,22 @@ class TestPathMatch : public TestFixture {
         ASSERT(!match.match("test.cpp"));
     }
 
+    void glob2() const {
+        PathMatch match({"test*.cpp"});
+        ASSERT(match.match("test1.cpp"));
+        ASSERT(match.match("src/test1.cpp"));
+        ASSERT(match.match("test1.cpp/src"));
+        ASSERT(!match.match("test1.c"));
+        ASSERT(match.match("test.cpp"));
+    }
+
     void globstar1() const {
         PathMatch match({"src/**/foo.c"});
         ASSERT(match.match("src/lib/foo/foo.c"));
         ASSERT(match.match("src/lib/foo/bar/foo.c"));
         ASSERT(!match.match("src/lib/foo/foo.cpp"));
         ASSERT(!match.match("src/lib/foo/bar/foo.cpp"));
+        ASSERT(!match.match("src/foo.c"));
     }
 
     void globstar2() const {
@@ -251,6 +271,7 @@ class TestPathMatch : public TestFixture {
         ASSERT(match.match("src/lib/foo/bar/foo.c"));
         ASSERT(!match.match("src/lib/foo/foo.cpp"));
         ASSERT(!match.match("src/lib/foo/bar/foo.cpp"));
+        ASSERT(!match.match("src/foo.c"));
     }
 
     void pathiterator() const {