Skip to content

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

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

Closed
rwiesenfarth wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
242107b
Refactor method
rwiesenfarth May 25, 2023
7f569f9
Fix #11735 Replace empty file name in suppression by wildcard
rwiesenfarth May 25, 2023
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
37 changes: 12 additions & 25 deletions gui/projectfiledialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,34 +753,21 @@ void ProjectFileDialog::setLibraries(const QStringList &libraries)

void ProjectFileDialog::addSingleSuppression(const Suppressions::Suppression &suppression)
{
QString suppression_name;
static const char sep = QDir::separator().toLatin1();
bool found_relative = false;

// Replace relative file path in the suppression with the absolute one
if ((suppression.fileName.find('*') == std::string::npos) &&
(suppression.fileName.find(sep) == std::string::npos)) {
QFileInfo inf(mProjectFile->getFilename());
QString rootpath = inf.absolutePath();
if (QFile::exists(QString{"%1%2%3"}.arg(rootpath,
QDir::separator(),
QString::fromStdString(suppression.fileName)))) {
Suppressions::Suppression sup = suppression;
sup.fileName = rootpath.toLatin1().constData();
sup.fileName += sep;
sup.fileName += suppression.fileName;
mSuppressions += sup;
suppression_name = QString::fromStdString(sup.getText());
found_relative = true;
Suppressions::Suppression sup = suppression;

if (sup.fileName.empty()) {
// Empty file name, replace by wildcard
sup.fileName = "*";
} else if (sup.fileName.find('*') == std::string::npos) {
// File name does not contain wildcard, enforce absolute path (dir is ignored if fileName already is absolute)
QFileInfo inf(QFileInfo(mProjectFile->getFilename()).absoluteDir(), QString::fromStdString(sup.fileName));
if (inf.exists()) {
sup.fileName = QDir::toNativeSeparators(inf.absoluteFilePath()).toStdString();
Copy link
Copy Markdown
Collaborator

@danmar danmar May 25, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@rwiesenfarth rwiesenfarth May 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
}

if (!found_relative) {
mSuppressions += suppression;
suppression_name = QString::fromStdString(suppression.getText());
}

mUI->mListSuppressions->addItem(suppression_name);
mSuppressions += sup;
Copy link
Copy Markdown
Collaborator

@danmar danmar May 25, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@rwiesenfarth rwiesenfarth May 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Collaborator

@danmar danmar May 25, 2023

Choose a reason for hiding this comment

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

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);
    }

mUI->mListSuppressions->addItem(QString::fromStdString(sup.getText()));
}

void ProjectFileDialog::setSuppressions(const QList<Suppressions::Suppression> &suppressions)
Expand Down