Show commit changes in interdiffs

Review Request #10126 — Created Aug. 25, 2018 and submitted

brennie
Review Board
release-4.0.x
10125
10127
25b7f10...
reviewboard

When an interdiff is displayed in the diffviewer, we now show a history
of how the commits changed from the diffset to the interdiffset. The
implementation of diffing between two histories is very basic at the
moment. We currently only directly compare commit IDs as our metric of
whether or not two commits are equivalent. Once any commit IDs differ,
we assume the rest of the commits in the old history will be removed and
the remaining new commits are all added. This is because commit IDs
depend on prior commit IDs in most DVCS systems. Ideally we would use
use commit ancestor information (which is trivally obtainable from
Mercurial and less so from Git).

  • Ran JS tests.
  • Ran unit tests.
Loading file attachments...

  • 0
  • 0
  • 83
  • 8
  • 91
Description From Last Updated
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

brennie
Review request changed
brennie
chipx86
  1. 
      
  2. Can you flesh out the description, describe how it's basic and what's missing?

  3. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
     
     
     
     
     

    This seems unnecessary. Can't we just move that stuff into an __init__ in this class?

  4. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     

    Needs to be the full type path.

  5. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     

    Can you pass as keyword arguments, so this is self-describing?

    Same with all below.

  6. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
     
     

    Is it really worth having these? The construction is pretty simple.

  7. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
     

    Why aren't we just using __init__? This class is feeling unnecessarily complicated, and I don't have a sense as to why it needs to be.

  8. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     

    Full type path, here and below.

  9. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
     
     

    No , or or in these.

  10. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
     
     

    Same here.

  11. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     

    Can we say "Iterate through the diff between ...", so it's a bit more clear?

  12. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
     
     

    No blank line.

  13. Doesn't seem like we'd need this namedtuple either. They kinda came out of nowhere, and I'm not sure they're appropriate for this work.

  14. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
     
     
     

    This should be a list so that ordering is consistent in the query. That allows the database to cache the queryset.

  15. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
     
     
     

    This can easily be one line.

  16. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
     

    These should be on the same line.

  17. We have a trailing comma here, but this isn't a .es6.js file. Let's rename it.

  18. Wrong number of spaces for indents.

  19. It's always good to have a break, even in the last case.

  20. These can start on the line with the call.

  21. Typo: "cellection" -> "collection".

  22. 
      
brennie
Review request changed

Change Summary:

Addressed feedback.

Description:

   

When an interdiff is displayed in the diffviewer, we now show a history

    of how the commits changed from the diffset to the interdiffset. The
    implementation of diffing between two histories is very basic at the
~   moment.

  ~ moment. We currently only directly compare commit IDs as our metric of
  + whether or not two commits are equivalent. Once any commit IDs differ,
  + we assume the rest of the commits in the old history will be removed and
  + the remaining new commits are all added. This is because commit IDs
  + depend on prior commit IDs in most DVCS systems. Ideally we would use
  + use commit ancestor information (which is trivally obtainable from
  + Mercurial and less so from Git).

Commit:

-833175cae00e9fb725f52a6059ef10f3827780d5
+e292e377e3ed8798546ce2fe49612c1771839c95

Diff:

Revision 4 (+608 -93)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed
chipx86
  1. 
      
  2. reviewboard/diffviewer/commit_utils.py (Diff revision 6)
     
     

    No trailing period.

  3. reviewboard/diffviewer/commit_utils.py (Diff revision 6)
     
     

    No trailing period.

  4. reviewboard/diffviewer/commit_utils.py (Diff revision 6)
     
     
     
     

    __init__ doesn't return.

  5. Parameters should align.

  6. reviewboard/staticbundles.py (Diff revision 6)
     
     
     

    This doesn't look right.

  7. 
      
brennie
Review request changed
chipx86
  1. 
      
  2. reviewboard/diffviewer/commit_utils.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Are i and j ever different? They start as 0, are both incremented at the same time, and never seem to otherwise change.

  3. Can you put the full module path? Looks like it'd fit.

  4. We always compare the result to the expectation. A failure here with these values backwards could be confusing to debug at first. These should all be fixed to follow the typical argument order.

  5. """ on the next line.

  6. Alphabetical order.

  7. reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Lookups would be faster if we just have a mapping defined (say, on RB.CommitHistoryDiffEntry, not the prototype) that mapped the entryType values to the symbols. Lookups are faster, and getSymbol() could just wrap this.

  8. Can you use the expanded form?

  9. 
      
brennie
Review request changed
brennie
Review request changed
brennie
Review request changed
david
  1. 
      
  2. reviewboard/diffviewer/commit_utils.py (Diff revision 10)
     
     

    Docstring?

  3. reviewboard/diffviewer/commit_utils.py (Diff revision 10)
     
     

    Docstring?

  4. Babel turns this into a monster. We'll need to do it with a bunch of assignments instead.

  5. We don't need the $() here, since append() can take an HTML string.

  6. 
      
brennie
Review request changed
brennie
Review request changed
chipx86
  1. 
      
  2. reviewboard/diffviewer/commit_utils.py (Diff revision 12)
     
     

    It's best to also provide __ne__ for Python 2.x. On 3.x, we get this for free.

  3. reviewboard/diffviewer/commit_utils.py (Diff revision 12)
     
     

    Typo: "attribtues" -> "attributes"

  4. reviewboard/diffviewer/commit_utils.py (Diff revision 12)
     
     
     

    Kind of confused by the wording here around the double enumerate.

    1. I think it got garbled while reflowing it, oops

  5. Can you wrap this at the "at"? It's slightly too long.

  6. We can reduce the generated HTML and the test changes needed if we do:

    <th<% if (showExpandCollapse) { %>colspan="2"<% } %>>
    
  7. reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These are a too long for the lines. How about building the response payload higher up, before the return maybe, and just doing cb(responsePayload) or something.

  8. Similar to above, let's only add the attribute if we have something other than "1" to set.

  9. 
      
brennie
Review request changed
chipx86
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (76f671a)
Loading...