Fix #11735: GUI: Adding suppression with empty file name does not match

[rwiesenfarth]

When the file name was left empty when defining a suppression, it got replaced with the directory path of the project file and did not match any file during analysis.

Now, an empty file name is changed to the wildcard pattern ("*"), matching all files - which is probably more intuitive.

[danmar]

I feel I am a bit on "thing ice". I am not sure how this is supposed to work..

[danmar]

Could you clarify what suppression path is added to the project file? It would be unfortunate if paths are always made absolute. The intention is that users should be able to share and reuse the same project file even if they store the project in different paths.

[rwiesenfarth]

I assumed that the initial comment "// Replace relative file path in the suppression with the absolute one" had exactly the goal to make the path absolute if it seems to be a relative path (i.e. contains no dir separator).

However, this did not work. Here the behavior of main (107eea2):

• the input "sample.h" is changed to "D:/Git/repo\sample.h" (both on GUI and in project file)
• "src\sample.cpp" is left as "src\sample.cpp"
• "src/sample.cpp" is changed to "D:/Git/repo\src/sample.cpp" (no '\' found)
• "" is changed to "D:/Git/repo\"

As you see, this is not intuitive, and probably also not intended.

The behavior of the PR version (7f569f9):

• "sample.h" is changed to "D:\Git\repo\sample.h"
• "src\sample.cpp" is changed to "D:\Git\repo\src\sample.cpp"
• "src/sample.cpp" is changed to "D:\Git\repo\src\sample.cpp"
• "" is changed to "*"

This is at least way more consistent - even if it is not intended to store absolute paths in the project file.

In any case, Suppression::fileName must be absolute, otherwise it won't match during isSuppressed() check. There seem to be quite a number of related issues:

1. while a RootPath may be given on the GUI, the absolute file path is generated to use the project file path. This should only be the case if RootPath is empty - I'd assume?
2. the project file always contains the absolute path, otherwise matchglob() won't match files. Thus RootPath is kind of obsolete for suppressions?
3. changing RootPath will not affect any of the suppressions, so they will keep any old absolute path

These issues should be addressed in a separate ticket. IMHO, the behavior should be:

• the "basePath" should be the RootPath (if defined) or else the project file path
• excludedSourcePaths and suppressionFileNames should all be stored in the project relative to basePath
• Suppression::fileName is always relative to basePath to avoid the need of changing all suppressions when basePath changes (probably same for excludedSourcePaths)
• matchglob() and similar routines must honor basePath together with Suppression::fileName when matching - or Suppression has to keep both absolute and relative path

If you write the ticket, I may volunteer to implement it...

[danmar]

hmm.. yes during the checking the absolute file paths are used for some reason I don't remember why directly.

so it makes sense that we somehow have suppressions with absolute paths during the checking.

As you see, this is not intuitive, and probably also not intended.

Yes that is not intended.

This is at least way more consistent

yeah I agree.

It's not your code to start with... but still I wonder if the paths should be changed by ProjectFileDialog at all. They can be stored in the project file just like the user wrote. In the checking the paths should be absolute and as far as I remember '/' separators are better than native separators.. so spontanously it feels to me that MainWindow::getCppcheckSettings() should tweak the paths used in the checking:

    for (const Suppressions::Suppression &suppression : mProjectFile->getSuppressions()) {
        // todo ensure path is absolute and convert path separators if necessary.
        result.nomsg.addSuppression(suppression);
    }

[danmar]

Spontanously I don't feel we should use native separators in suppressions. But well the old code seems to do that also. hmm..
I am not sure how these are used. Is the suppression paths written in the project file using native separators?

[rwiesenfarth]

The one I got had a mixture of both. The "rootpath" part had '/' separators, but got a '\' appended ("D:/Git/repo/src\").

[rwiesenfarth]

Can we merge this PR?

I would like to work on a refactoring of the handling of project path, root path and paths given in suppressions, but on a new branch, and referring to a new trac ticket.

[danmar]

but does this fix have some proper code then?

My spontanous feeling is that we should remove the code in ProjectFileDialog that changes the paths

[danmar]

But well.. I haven't looked at this in detail.

[rwiesenfarth]

but does this fix have some proper code then?

At least the results are more consistent than before - even if still not as intended. See above for comparison between old and new behavior.

My spontanous feeling is that we should remove the code in ProjectFileDialog that changes the paths

That would be part of the refactoring: where should the conversion relative path to absolute path occur? It seems to be strongly tied to the GUI or the XML project, so it should probably be done in ProjectFile or ProjectFileDialog.

[danmar]

well I need to check the code more I guess so I understand why we tweak the paths at all.

[danmar]

I opened #5127

I see no good reason to tweak the paths.

The paths was made absolute to fix https://trac.cppcheck.net/ticket/10377 but I don't see that #5127 will cause such problems again.