Show squashed diffs in the diffviewer

Review Request #7135 — Created March 28, 2015 and submitted

Information

Review Board
dvcs
af9f908...

Reviewers

Previously we would show multiple FileDiffs for the same file in the
diff viewer if the file was modified in multiple commits. Now we
determine what FileDiffs are ancestors of other FileDiffs in the
commit history and filter out all the ancestors. The result is a
squashed diff between the original file and the resulting file.

Ran unit tests.

Manually verified that the squashed diffs were correct.

Still able to view the FileDiffs of review requests without
commit history.

Description From Last Updated

Col: 12 E111 indentation is not a multiple of four

reviewbotreviewbot

We should probably just import logging at the top of the file.

daviddavid

This should use exc_info=True (we have =1 elsewhere, but I'm slowly cleaning that up).

daviddavid

I think I liked your approach up above of changing all of these to add the - and the beginning …

daviddavid

Unindent.

brenniebrennie

"In this case" twice is awkward phrasing.

brenniebrennie

diffviewer.commitutils before diffviewer.differ

daviddavid

Blank line after this.

daviddavid

This should also have a '-'

daviddavid

Can you maybe describe this option in the docstring above?

chipx86chipx86

Does not check if there is a diff_commit.

brenniebrennie

This has the potential to be cheaper by doing: self.filediff.diff_commit_id. Reduces the possibility of a lookup.

chipx86chipx86

Alignment is inconsistent between these.

chipx86chipx86

Maybe flesh this out a bit so its description stands alone without needing to know more of the architecture as …

chipx86chipx86

Can you use a list comprehension instead of map? They're generally easier to read and are faster, since they don't …

chipx86chipx86

Since this code is all basically the same as the above, we should factor this out into another function.

chipx86chipx86

Does not check if there is a diff_commit

brenniebrennie

As above, we should use diff_commit_id here instead.

chipx86chipx86

No need for six.text_type anymore.

chipx86chipx86

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

local variable 'have_interdiffset_commits' is assigned to but never used

reviewbotreviewbot

"FileDiffs" and "DiffSet".

chipx86chipx86

Looks like we're doing this twice now?

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. Show all issues
    Col: 12
     E111 indentation is not a multiple of four
    
  3. 
      
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 3)
     
     
    Show all issues

    We should probably just import logging at the top of the file.

    1. Oops, this was left over from debugging where I broke something else :)

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

    This should use exc_info=True (we have =1 elsewhere, but I'm slowly cleaning that up).

  4. reviewboard/diffviewer/renderers.py (Diff revision 3)
     
     
    Show all issues

    I think I liked your approach up above of changing all of these to add the - and the beginning of each appended string.

  5. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Would it be worth it to build a list of parts for the key and use ''.join versus all the appends we're doing, or does that matter? Here and in the renderer.

    1. Not really. Joins are better when you have a lot of strings you're working with (for, say, text files), but there's not that many here. In fact, it can have the opposite effect and actually take longer for smaller sets of strings.

      To check, I ran comparisons using the timeit module, comparing the above with and without using join (and with hard-coded values for all PKs and such). 100,000 runs of each.

      Without joins, 100,000 runs took 0.0366 seconds.

      With joins, 100,000 runs took 0.0422 seconds.

  3. reviewboard/diffviewer/models.py (Diff revision 4)
     
     
     
     
    Show all issues

    Unindent.

  4. reviewboard/diffviewer/models.py (Diff revision 4)
     
     
     
     
    Show all issues

    "In this case" twice is awkward phrasing.

  5. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 5)
     
     
     
    Show all issues

    diffviewer.commitutils before diffviewer.differ

  3. reviewboard/diffviewer/models.py (Diff revision 5)
     
     
    Show all issues

    Blank line after this.

  4. reviewboard/diffviewer/renderers.py (Diff revision 5)
     
     
    Show all issues

    This should also have a '-'

  5. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 6)
     
     
     
    Show all issues

    Does not check if there is a diff_commit.

  3. reviewboard/diffviewer/renderers.py (Diff revision 6)
     
     
     
     
    Show all issues

    Does not check if there is a diff_commit

  4. 
      
brennie
chipx86
  1. 
      
  2. Show all issues

    Can you maybe describe this option in the docstring above?

  3. Show all issues

    This has the potential to be cheaper by doing: self.filediff.diff_commit_id. Reduces the possibility of a lookup.

  4. reviewboard/diffviewer/chunk_generator.py (Diff revision 6)
     
     
     
    Show all issues

    Alignment is inconsistent between these.

  5. reviewboard/diffviewer/commitutils.py (Diff revision 6)
     
     
    Show all issues

    Maybe flesh this out a bit so its description stands alone without needing to know more of the architecture as a whole.

  6. reviewboard/diffviewer/diffutils.py (Diff revision 6)
     
     
     
    Show all issues

    Can you use a list comprehension instead of map? They're generally easier to read and are faster, since they don't require the extra function calls.

  7. reviewboard/diffviewer/diffutils.py (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    Since this code is all basically the same as the above, we should factor this out into another function.

  8. reviewboard/diffviewer/renderers.py (Diff revision 6)
     
     
     
    Show all issues

    As above, we should use diff_commit_id here instead.

  9. reviewboard/diffviewer/renderers.py (Diff revision 6)
     
     
    Show all issues

    No need for six.text_type anymore.

  10. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. reviewboard/diffviewer/diffutils.py (Diff revision 8)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  3. reviewboard/diffviewer/diffutils.py (Diff revision 8)
     
     
    Show all issues
     local variable 'have_interdiffset_commits' is assigned to but never used
    
  4. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/diffviewer/commitutils.py (Diff revision 9)
     
     
    Show all issues

    "FileDiffs" and "DiffSet".

  3. reviewboard/diffviewer/diffutils.py (Diff revision 9)
     
     
     
    Show all issues

    Looks like we're doing this twice now?

  4. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/renderers.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/commitutils.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to dvcs (dcaca07)