Skip to content
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.

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

[cmd][diff] Ignore detection status for tags #4013

Merged

Conversation

Szelethus
Copy link
Contributor

@Szelethus Szelethus commented Sep 20, 2023

We already do this on the GUI, but on the cmdline, we default the
detection status to NEW, REOPENED and UNRESOLVED. This makes sense for
runs (where we check whether a report is outstanding at the time of the
query), but not for tags (as a report can have a RESOLVED detection
status but still be outstanding at the time of the tag).

@Szelethus Szelethus added the CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands label Sep 20, 2023
@Szelethus Szelethus added this to the release 6.23.0 milestone Sep 20, 2023
Copy link
Contributor

@bruntib bruntib left a comment

Choose a reason for hiding this comment

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

Please, rebase. Otherwise looks good.

@Szelethus Szelethus force-pushed the ignore_detection_status_for_tags branch from 57eba10 to 5f5c8c0 Compare October 5, 2023 11:34
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

Please refer to this document from the usage guide.
Also please connect the sections to real PR analysis, branch analysis use-cases which should be introduced in the intro section. The user should understand how to use these concepts in practice.

docs/web/diff.md Outdated Show resolved Hide resolved
docs/web/diff.md Outdated Show resolved Hide resolved
docs/web/diff.md Outdated Show resolved Hide resolved
docs/web/diff.md Outdated
----=================----
```

## Review status rules and diffs
Copy link
Member

Choose a reason for hiding this comment

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

Refer to the use cases introduced in the introduction section.

E.g.

If you whish to store the suppressions on the server, you can use review status rules. With the review status rule you can set all instances of report type (reports with the same hash) e.g. false positive. This feature makes it easy to mark a report false positive in all runs at the same time.

Copy link
Contributor Author

@Szelethus Szelethus Jan 8, 2024

Choose a reason for hiding this comment

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

I don't see how this adds anything to the document. Also, I just don't think this is the place where we should properly document review status rules -- merely the place to discuss how their usage affects diff.

Also, I think the current shorthand explanation already does a decent job.

docs/web/diff.md Outdated Show resolved Hide resolved
docs/web/diff.md Outdated Show resolved Hide resolved
docs/web/diff.md Outdated

Compare the outstanding reports in a "before" (_baseline_) and an "after" (_new_ or newline) analysis sets, and display the differences in between them the in the same
format as `results`. A report is outstanding if all of the following is true:
* its detection status is _new_, _reopened_ or _unresolved_,
Copy link
Member

@dkrupp dkrupp Oct 5, 2023

Choose a reason for hiding this comment

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

A report is outstanding in a given time T if all the following is true:

  • if it was detected before T
  • if it was closed after T, or not closed at all
  • a report can be closed if becomes undetected, considered false positive, intentional bug.

The server stores the past states of a reports with certain simplifications (see section **), so it can tell for any past time points whether the report was outstanding or closed.

The local report directory only stores a single present state.

In CodeChecker the most recent status of a report is also represented in the detection status:
-new: the report is new compared to the last analysis
-reopened: the report is new compared to the last analysis and it is reappearing
-unresolved: the report is still detected
-resolved: the report is not detected anymore

This is useful to quickly see if for example the analysis set of the "master" branch is updated, what are the new/unresolved reports we still have to work with.

Please note however, the detection status only describe the present state and does not play a role when comparing any two (past) analysis result sets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the "outstanding report" definition when we diff tags/timestamps.

I added links to the detection status and review status docs.

For the rest, this is just not the place to describe what detection statuses are, but rather their own page. On the fact that we ignore detection statuses for tags/timestamps there already is paragraph.

@dkrupp dkrupp self-requested a review October 6, 2023 08:03
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

With the condition of fixin my comments in diff.md in #4006 (review)

Szelethus added a commit to Szelethus/codechecker that referenced this pull request Oct 9, 2023
We already do this on the GUI, but on the cmdline, we default the
detection status to NEW, REOPENED and UNRESOLVED. This makes sense for
runs (where we check whether a report is outstanding at the time of the
query), but not for tags (as a report can have a RESOLVED detection
status but still be outstanding at the time of the tag).
@Szelethus Szelethus force-pushed the ignore_detection_status_for_tags branch from 5f5c8c0 to a9a4926 Compare October 9, 2023 08:18
@Szelethus
Copy link
Contributor Author

I cherry-picked the docs changes into #4006, so it no longer has any dependencies.

@Szelethus Szelethus merged commit 6f57fa2 into Ericsson:master Oct 9, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants