Added blame info feature for "cmd results" command #4008

cservakt · 2023-09-13T15:27:57Z

Blame information for reports was only available on the GUI. Now, if we wish to check the git commit info in the cli, we can do that with "CodeChecker cmd results --details" command We can only check blame info for runs that have a git repository. The server address and the run name should also be given, e.g.: "CodeChecker cmd results --details --url http://localhost:8001/Default test". The syntax is different in the GUI, as there you can check the blame info for each line of a reported file. In case of the cli, it would be huge output result to print long blame info for every report. A filtered one is added to the report data, which contains only the commit data of the given section.

bruntib · 2023-09-29T21:14:58Z

web/api/report_server.thrift

-  17: optional string analyzerName,    // Analyzer name.
+  15: i64             bugPathLength,  // Length of the bug path.
+  16: optional ReportDetails details, // Details of the report.
+  17: optional BlameInfo blameInfo,   // Blmae info.


New parameters should be added to the end of the list without changing previous numbers. As far as I know, these numbers play a role in API backward compatibility.

You're right. I've fixed it and added the new prop to the end of the struct.

bruntib · 2023-09-29T21:46:48Z

web/server/codechecker_server/api/report_server.py

+                    blame_info = blame_infos[report.id] \
+                        if report.id in blame_infos else None


Suggested change

blame_info = blame_infos[report.id] \

if report.id in blame_infos else None

blame_info = blame_infos.get(report.id)

Thanks for your suggestion!

bruntib · 2023-09-29T21:49:31Z

web/server/codechecker_server/api/report_server.py

+                    report_ids, blames = zip(*[
+                        (
+                            r[0].id,
+                            (r[0].id, self.getBlameInfo(r[0].file_id))


We could consider caching fetched blame info in case there are many reports in a file.

Szelethus

We definitely can't land this without tests.

Also, I saw the following blame info from a tinyxml2 analysis in json format:

    "blameInfo": {
      "commits": {
        "a9cf3f9f3fe65df392caa5aecd3b77a260d7921f": {
          "author": {
            "name": "Lee Thomason",
            "email": "blabla123123123@sdkljfghsdjlkfgh.sdflkoghj"
          },
          "summary": "Switched to Artistic Style auto-formatting to allow integration of patches from other coding styles.",
          "message": "Switched to Artistic Style auto-formatting to allow integration of patches from other coding styles.\n",
          "committedDateTime": "2012-10-11 16:56:51-07:00"
        }
      },
      "blame": [
        {
          "startLine": 2646,
          "endLine": 2646,
          "commitHash": "a9cf3f9f3fe65df392caa5aecd3b77a260d7921f"
        }
      ]
    }

It may not be your doing, but are we sure we want the commit hash to be a key?

Szelethus · 2023-10-05T11:14:39Z

web/api/report_server.thrift

@@ -325,6 +325,7 @@ struct ReportData {
  // of custom labels that describe some properties of a report. For example the
  // timestamp in case of dynamic analyzers when the report was actually emitted.
  18: optional map<string, string> annotations,
+  19: optional BlameInfo blameInfo,    // Blmae info.


Its a typo, but more importantly, maybe this isn't the comment we need here :) How about
"Contains the git blame information of the report. May be NULL if the analysis was not done in a git repository."

Thanks for your suggestion! I completely agree. It would be better if there was a more precise description.

cservakt · 2023-10-06T12:53:52Z

We definitely can't land this without tests.

Also, I saw the following blame info from a tinyxml2 analysis in json format:

    "blameInfo": {
      "commits": {
        "a9cf3f9f3fe65df392caa5aecd3b77a260d7921f": {
          "author": {
            "name": "Lee Thomason",
            "email": "blabla123123123@sdkljfghsdjlkfgh.sdflkoghj"
          },
          "summary": "Switched to Artistic Style auto-formatting to allow integration of patches from other coding styles.",
          "message": "Switched to Artistic Style auto-formatting to allow integration of patches from other coding styles.\n",
          "committedDateTime": "2012-10-11 16:56:51-07:00"
        }
      },
      "blame": [
        {
          "startLine": 2646,
          "endLine": 2646,
          "commitHash": "a9cf3f9f3fe65df392caa5aecd3b77a260d7921f"
        }
      ]
    }

It may not be your doing, but are we sure we want the commit hash to be a key?

I think this is the best way to print the result in cli. Blame info belongs to a file, not to a report and each commit is identified by the commit hash. So we can only filter it to get a shorter form for the given report. If we want to get a different syntax when using the cli, we need to modify the blame info stuct in thirift, which is also used by the GUI.

Blame information for reports was only available on the GUI. Now, if we wish to check the git commit info in the cli, we can do that with "CodeChecker cmd results --details" command We can only check blame info for runs that have a git repository. The server address and the run name should also be given, e.g.: "CodeChecker cmd results --details --url http://localhost:8001/Default test". The syntax is different in the GUI, as there you can check the blame info for each line of a reported file. In case of the cli, it would be huge output result to print long blame info for every report. A filtered one is added to the report data, which contains only the commit data of the given section.

Adding test to check cmd results blmae info feature.

Szelethus · 2023-10-11T09:53:05Z

Shouldn't the value for key commits be a list instead of a dict? How about this:

"blameInfo": {
  "commits": [
    {
      "commit_hash": "a9cf3f9f3fe65df392caa5aecd3b77a260d7921f",
      "author": {
        "name": "Lee Thomason",
        "email": "blabla123123123@sdkljfghsdjlkfgh.sdflkoghj"
      },
      "summary": "Switched to Artistic Style auto-formatting to allow integration of patches from other coding styles.",
      "message": "Switched to Artistic Style auto-formatting to allow integration of patches from other coding styles.\n",
      "committedDateTime": "2012-10-11 16:56:51-07:00"
    },
    {
      "commit_hash": "a9cf3f9f3fe65df392caa5aecd3b77a260d7921f",
      "author": {
        "name": "Lee Thomason",
        "email": "blabla123123123@sdkljfghsdjlkfgh.sdflkoghj"
      },
      "summary": "Switched to Artistic Style auto-formatting to allow integration of patches from other coding styles.",
      "message": "Switched to Artistic Style auto-formatting to allow integration of patches from other coding styles.\n",
      "committedDateTime": "2012-10-11 16:56:51-07:00"
    }
  ],
  "blame": [
    {
      "startLine": 2646,
      "endLine": 2646,
      "commitHash": "a9cf3f9f3fe65df392caa5aecd3b77a260d7921f"
    }
  ]
}

Mind that the hash is no longer a key here either, but a value. This looks more idiomatic to me.

Szelethus · 2023-10-11T09:45:35Z

web/api/report_server.thrift

@@ -325,6 +325,7 @@ struct ReportData {
  // of custom labels that describe some properties of a report. For example the
  // timestamp in case of dynamic analyzers when the report was actually emitted.
  18: optional map<string, string> annotations,
+  19: optional BlameInfo blameInfo,    // Contains the git blame information of the report if it exists.


This is longer than 79 columns.

Szelethus · 2023-10-11T11:03:01Z

web/server/codechecker_server/api/report_server.py

+                blame_infos = {}
+                if get_details and len(query_result):
+                    report_ids, blames = zip(*[
+                        (
+                            r[0].id,
+                            (r[0].id, self.getBlameInfo(r[0].file_id))
+                        ) for r in query_result])
                    report_details = get_report_details(session, report_ids)
+                    blame_infos = dict(blames)


getRunResults is already stomach-churningly long. Can we put these lines a new function instead?

Szelethus · 2023-10-11T11:03:33Z

web/server/codechecker_server/api/report_server.py

+                    blame_info = blame_infos.get(report.id)
+                    if blame_info and blame_info.commits and blame_info.blame:
+                        blame_data = [b for b in blame_info.blame
+                                      if report.line >= b.startLine
+                                      and report.line <= b.endLine]
+                        commitHash = blame_data[0].commitHash \
+                            if len(blame_data) else None
+                        commitInfo = {cHash: commit for cHash, commit
+                                      in blame_info.commits.items()
+                                      if cHash == commitHash}
+                        blame_info = BlameInfo(
+                            commits=commitInfo,
+                            blame=blame_data
+                        )
+


Same thing here.

Szelethus · 2023-10-11T11:04:31Z

web/server/codechecker_server/api/report_server.py

+                            commits=commitInfo,
+                            blame=blame_data
+                        )
+


These code snippets are basically copy-paste from above, right? Can we do something about that?

It is more readable form.

Szelethus · 2023-10-11T11:07:58Z

web/tests/functional/report_viewer_api/test_get_run_results.py

+        run_results = self._cc_client.getRunResults([runid],
+                                                    100,
+                                                    0,
+                                                    None,
+                                                    simple_filter,
+                                                    None,
+                                                    True)


Can we name the parameters we're assign to here? LLVM Coding Standards has a well written guidince on this. By no means are we strictly abiding all of these, but they make a lot of sense :)

We do not name the parameters in the above functions either. If we want to do it, then in all the other cases we must also assigne them.

Szelethus · 2023-10-11T11:09:43Z

web/tests/functional/report_viewer_api/test_get_run_results.py

+                                                    None,
+                                                    True)
+
+        self.assertTrue(any(res.blameInfo for res in run_results))


Can we test the json output as well? At least that is doesn't crash.

I think it is sufficient to check only the existence of the blame info. If someone modified the test files, the blame info whould be also changed, which whould break the test.

cservakt added API change 📄 Content of patch changes API! WIP 💣 Work In Progress CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands CI 📦 new feature 👍 New feature request labels Sep 13, 2023

cservakt added this to the release 6.23.0 milestone Sep 13, 2023

cservakt requested a review from Szelethus September 13, 2023 15:27

cservakt self-assigned this Sep 13, 2023

cservakt requested review from bruntib and vodorok as code owners September 13, 2023 15:27

cservakt force-pushed the cmd-results-blameinfo branch 2 times, most recently from 76e443e to 5baf2f7 Compare September 14, 2023 11:37

cservakt removed the WIP 💣 Work In Progress label Sep 22, 2023

bruntib requested changes Sep 29, 2023

View reviewed changes

cservakt force-pushed the cmd-results-blameinfo branch from 5baf2f7 to 2bc73d1 Compare October 2, 2023 12:47

cservakt requested a review from bruntib October 2, 2023 13:39

Szelethus requested changes Oct 5, 2023

View reviewed changes

cservakt force-pushed the cmd-results-blameinfo branch 2 times, most recently from dd5734f to 9d0ac9e Compare October 10, 2023 12:47

cservakt requested a review from Szelethus October 10, 2023 13:21

cservakt force-pushed the cmd-results-blameinfo branch 3 times, most recently from 781796f to 987e196 Compare October 11, 2023 09:29

Added blame info feature for "cmd results" command

5fc5059

Adding test to check cmd results blmae info feature.

cservakt force-pushed the cmd-results-blameinfo branch from 987e196 to 5fc5059 Compare October 11, 2023 09:34

Szelethus reviewed Oct 11, 2023

View reviewed changes

Szelethus requested changes Oct 11, 2023

View reviewed changes

bruntib modified the milestones: release 6.23.0, release 6.24.0 Nov 27, 2023

bruntib modified the milestones: release 6.24.0, release 6.25.0 Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added blame info feature for "cmd results" command #4008

Added blame info feature for "cmd results" command #4008

cservakt commented Sep 13, 2023

bruntib Sep 29, 2023

cservakt Oct 10, 2023 •

edited

bruntib Sep 29, 2023

cservakt Oct 10, 2023

bruntib Sep 29, 2023

Szelethus left a comment •

edited

Szelethus Oct 5, 2023

cservakt Oct 6, 2023

cservakt commented Oct 6, 2023

Szelethus commented Oct 11, 2023

Szelethus Oct 11, 2023

Szelethus Oct 11, 2023

Szelethus Oct 11, 2023

Szelethus Oct 11, 2023

cservakt Oct 11, 2023

Szelethus Oct 11, 2023

cservakt Oct 11, 2023

Szelethus Oct 11, 2023

cservakt Oct 11, 2023

		blame_info = blame_infos[report.id] \
		if report.id in blame_infos else None

	blame_info = blame_infos[report.id] \
	if report.id in blame_infos else None
	blame_info = blame_infos.get(report.id)

Added blame info feature for "cmd results" command #4008

Are you sure you want to change the base?

Added blame info feature for "cmd results" command #4008

Conversation

cservakt commented Sep 13, 2023

Choose a reason for hiding this comment

cservakt Oct 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szelethus left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cservakt commented Oct 6, 2023

Szelethus commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cservakt Oct 10, 2023 •

edited

Szelethus left a comment •

edited