HTML format bug report for local compare #1278

dkrupp · 2018-01-05T12:53:30Z

"CodeChecker cmd diff" command can generate HTML output
even if the new report set is a local report directory.

whisperity · 2018-01-05T16:03:32Z

(@dkrupp Your commit environment uses a machine-local e-mail address and thus the commit isn't assigned to you properly.)

whisperity · 2018-01-05T16:05:02Z

libcodechecker/cmd/cmd_line_client.py

@@ -356,6 +357,50 @@ def get_report_data(client, reports, file_cache):
        return {'files': file_sources,
                'reports': report_data}

+    def reports2report_data(reports):


This function looks very familiar yet this patch only introduces it as block code. Can't we import the library where this is originally in (optionally with a proper handling of import errors?)

This is a totally new code. The function get_report_data() a little bit above is similarish, but that is converting from another report format.

reports_to_report_data function name would be better.

gyorb · 2018-01-08T07:37:42Z

libcodechecker/cmd/cmd_line_client.py

+            # Not all report in this list may refer to the same files
+            # thus we need to create a single file list with
+            # all files from all reports.
+            for f in report.files:


You could use enumerate here, so you will not need a separate findex variable.

for index, f in enumerate(report.files): ...

The same applies for the other index variable below.

I prefer the old fashion way. ;)

csordasmarton · 2018-01-08T08:51:46Z

libcodechecker/cmd/cmd_line_client.py

@@ -356,6 +357,50 @@ def get_report_data(client, reports, file_cache):
        return {'files': file_sources,
                'reports': report_data}

+    def reports2report_data(reports):


reports_to_report_data function name would be better.

csordasmarton · 2018-01-08T08:57:03Z

libcodechecker/cmd/cmd_line_client.py

+
+            events = []
+            pathElements = report.bug_path
+            index = 0


Let this variable be equal with 1 (index = 1). This way step field of an event below can be just simply equal with this index.

'step': index})

csordasmarton · 2018-01-08T08:58:20Z

libcodechecker/cmd/cmd_line_client.py

@@ -365,17 +410,26 @@ def report_to_html(client, reports, output_dir):

        file_report_map = defaultdict(list)
        for report in reports:
-            file_report_map[report.fileId].append(report)
+            filePath = ""


Do not use camel cased variable names in python. Use underscore to separate the part of the variable name:

file_path

csordasmarton · 2018-01-08T09:21:07Z

libcodechecker/cmd/cmd_line_client.py

            filename = os.path.basename(checked_file)
+            h = int(hashlib.md5(file_path).hexdigest(), 16) % (10 ** 8)


Why do we convert the hash to a number and limit the values to 10^8? This way the probability of hash collision is a much larger risk. Would not be better to use the full hash?

Hash collision is minimal, as a file name is always the prefix. So this number is only to distinguish between files with the same name and different paths.
The full hash is 16 bytes long and in hex it is a 32 long character chain.
I think a 8 long decimal chain is enough to distinguish between files.
So an output HTML will look like:
alma.c_95479304.html

instead of
alma.c_ebbc3c26a34b609dc46f5c3378f96e08.html

See

h.digest_size
16L
h.hexdigest()
'ebbc3c26a34b609dc46f5c3378f96e08'
int(h.hexdigest(), 16) % (10 ** 8)
95479304L

dkrupp

Fixed the review comments

dkrupp · 2018-01-08T11:04:39Z

libcodechecker/cmd/cmd_line_client.py

            filename = os.path.basename(checked_file)
+            h = int(hashlib.md5(file_path).hexdigest(), 16) % (10 ** 8)


Hash collision is minimal, as a file name is always the prefix. So this number is only to distinguish between files with the same name and different paths.
The full hash is 16 bytes long and in hex it is a 32 long character chain.
I think a 8 long decimal chain is enough to distinguish between files.
So an output HTML will look like:
alma.c_95479304.html

instead of
alma.c_ebbc3c26a34b609dc46f5c3378f96e08.html

See

h.digest_size
16L
h.hexdigest()
'ebbc3c26a34b609dc46f5c3378f96e08'
int(h.hexdigest(), 16) % (10 ** 8)
95479304L

dkrupp · 2018-01-08T11:06:09Z

libcodechecker/cmd/cmd_line_client.py

+            # Not all report in this list may refer to the same files
+            # thus we need to create a single file list with
+            # all files from all reports.
+            for f in report.files:


I prefer the old fashion way. ;)

csordasmarton · 2018-01-08T13:08:12Z

libcodechecker/cmd/cmd_line_client.py

+            if isinstance(report, Report):
+                file_path = report.main['location']['file_name']
+            else:
+                file_path = report.checked_file


ReportData instance has no attribute checked_file. Use checkedFile attribute:

file_path = report.checkedFile

whisperity

Commit author information is still bogus.

whisperity · 2018-01-08T14:59:49Z

libcodechecker/cmd/cmd_line_client.py

@@ -441,6 +496,7 @@ def print_reports(client, reports, output_format):
                    get_line_from_file(report.main['location']['file_name'],
                                       bug_line)
            else:


Do we know the type that comes from the server? Maybe it should be explicitly put as an elif, instead of an else.

"CodeChecker cmd diff" command can generate HTML output even if the new report set is a local report directory.

dkrupp force-pushed the diff_fix branch from 2684911 to 658e6ba Compare January 5, 2018 15:48

whisperity added bugfix 🔨 CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands labels Jan 5, 2018

whisperity reviewed Jan 5, 2018

View reviewed changes

dkrupp requested review from csordasmarton, Xazax-hun and gyorb January 5, 2018 17:35

gyorb suggested changes Jan 8, 2018

View reviewed changes

csordasmarton suggested changes Jan 8, 2018

View reviewed changes

dkrupp force-pushed the diff_fix branch from 658e6ba to 6c5e615 Compare January 8, 2018 11:09

dkrupp commented Jan 8, 2018

View reviewed changes

csordasmarton suggested changes Jan 8, 2018

View reviewed changes

dkrupp force-pushed the diff_fix branch from 6c5e615 to eb60093 Compare January 8, 2018 13:38

dkrupp requested review from csordasmarton and gyorb January 8, 2018 13:49

csordasmarton approved these changes Jan 8, 2018

View reviewed changes

whisperity requested changes Jan 8, 2018

View reviewed changes

HTML format bug report for local compare

b0f6b43

"CodeChecker cmd diff" command can generate HTML output even if the new report set is a local report directory.

dkrupp force-pushed the diff_fix branch from eb60093 to b0f6b43 Compare January 8, 2018 15:56

csordasmarton merged commit 136fabc into Ericsson:master Jan 8, 2018

dkrupp deleted the diff_fix branch February 17, 2018 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML format bug report for local compare #1278

HTML format bug report for local compare #1278

dkrupp commented Jan 5, 2018

whisperity commented Jan 5, 2018

whisperity Jan 5, 2018

dkrupp Jan 5, 2018

csordasmarton Jan 8, 2018

gyorb Jan 8, 2018

dkrupp Jan 8, 2018

csordasmarton Jan 8, 2018

csordasmarton Jan 8, 2018

csordasmarton Jan 8, 2018

csordasmarton Jan 8, 2018

dkrupp Jan 8, 2018

dkrupp left a comment

dkrupp Jan 8, 2018

dkrupp Jan 8, 2018

csordasmarton Jan 8, 2018

whisperity left a comment

whisperity Jan 8, 2018

		filename = os.path.basename(checked_file)
		h = int(hashlib.md5(file_path).hexdigest(), 16) % (10 ** 8)

HTML format bug report for local compare #1278

HTML format bug report for local compare #1278

Conversation

dkrupp commented Jan 5, 2018

whisperity commented Jan 5, 2018

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

Choose a reason for hiding this comment

dkrupp left a 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

whisperity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment