Added SHA256 hashes for original and patched files in FileDiffs.

Review Request #10767 — Created Oct. 28, 2019 and submitted

Information

Review Board
release-4.0.x
8059a93...

Reviewers

Back in Review Board 2.0, we introduced SHA1 hashes in FileDiffs, which
were in part used to determine if filediffs and interfilediffs matched.
SHA1 is not technically safe anymore (though there's little harm it can
do in our situation, aside from generate patch errors in the diff
viewer), and we want to move to SHA256 where possible.

This change adds storage and properties for SHA256 hashes on FileDiff,
and updates our FileDiff comparison logic to prioritize them over SHA1
hashes (though it will still check SHA1 if the SHA256 hashes are
missing, since they're created only when viewing the diff for the first
time).

The comparison logic has been moved into a new get_filediffs_match()
function, and unit tests were added.

Unit tests pass.

Didn't see any regressions when viewing interdiffs.

Verified that FileDiffs were recording the SHA256 hashes.

Description From Last Updated

This comment seems a little confusing given the code.

daviddavid

How about "if we already have a SHA256 checksum, then we'll also have a SHA1 checksum"?

daviddavid

Same here.

daviddavid

Same here.

daviddavid
david
  1. 
      
  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
    Show all issues

    This comment seems a little confusing given the code.

    1. I don't know if I improved the situation, but the old comment was definitely wrong (I probably rewrote the logic).

  3. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
    Show all issues

    Same here.

  4. 
      
chipx86
david
  1. 
      
  2. reviewboard/diffviewer/chunk_generator.py (Diff revisions 1 - 2)
     
     
     
    Show all issues

    How about "if we already have a SHA256 checksum, then we'll also have a SHA1 checksum"?

  3. reviewboard/diffviewer/chunk_generator.py (Diff revisions 1 - 2)
     
     
     
     
    Show all issues

    Same here.

  4. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (a78f960)