Show commit changes in interdiffs

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

Information

Review Board
release-4.0.x
25b7f10...

Reviewers

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.

Description From Last Updated

Can you flesh out the description, describe how it's basic and what's missing?

chipx86chipx86

E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 54 Expected a 'break' statement before 'default'.

reviewbotreviewbot

Col: 13 Unexpected ')'.

reviewbotreviewbot

Col: 13 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 17 Expected ')' and instead saw ';'.

reviewbotreviewbot

Col: 18 Missing semicolon.

reviewbotreviewbot

Col: 21 It's not necessary to initialize 'rowClass' to 'undefined'.

reviewbotreviewbot

Col: 17 Unexpected ')'.

reviewbotreviewbot

Col: 17 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 21 Expected ')' and instead saw ';'.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 13 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 13 Unexpected ')'.

reviewbotreviewbot

Col: 19 Missing semicolon.

reviewbotreviewbot

Col: 18 Expected ')' and instead saw ';'.

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

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

chipx86chipx86

Needs to be the full type path.

chipx86chipx86

Can you pass as keyword arguments, so this is self-describing? Same with all below.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Full type path, here and below.

chipx86chipx86

No , or or in these.

chipx86chipx86

Same here.

chipx86chipx86

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

chipx86chipx86

No blank line.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This can easily be one line.

chipx86chipx86

These should be on the same line.

chipx86chipx86

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

chipx86chipx86

Blank line between these.

chipx86chipx86

Wrong number of spaces for indents.

chipx86chipx86

Missing docstring.

chipx86chipx86

You can use isEmpty().

chipx86chipx86

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

chipx86chipx86

These can start on the line with the call.

chipx86chipx86

Typo: "cellection" -> "collection".

chipx86chipx86

F401 'collections.namedtuple' imported but unused

reviewbotreviewbot

F821 undefined name 'cls'

reviewbotreviewbot

F821 undefined name 'cls'

reviewbotreviewbot

F821 undefined name 'cls'

reviewbotreviewbot

F401 'collections.namedtuple' imported but unused

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

F401 'collections.namedtuple' imported but unused

reviewbotreviewbot

F821 undefined name 'cls'

reviewbotreviewbot

F821 undefined name 'cls'

reviewbotreviewbot

F821 undefined name 'cls'

reviewbotreviewbot

F401 'collections.namedtuple' imported but unused

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

No trailing period.

chipx86chipx86

No trailing period.

chipx86chipx86

__init__ doesn't return.

chipx86chipx86

Parameters should align.

chipx86chipx86

No spaces within {..}

chipx86chipx86

E501 line too long (83 > 79 characters)

reviewbotreviewbot

This doesn't look right.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

We always compare the result to the expectation. A failure here with these values backwards could be confusing to debug …

chipx86chipx86

""" on the next line.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Lookups would be faster if we just have a mapping defined (say, on RB.CommitHistoryDiffEntry, not the prototype) that mapped the …

chipx86chipx86

Can you use the expanded form?

chipx86chipx86

Same here.

chipx86chipx86

E501 line too long (83 > 79 characters)

reviewbotreviewbot

F821 undefined name 'j'

reviewbotreviewbot

Col: 2 Missing semicolon.

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

Docstring?

daviddavid

Docstring?

daviddavid

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

daviddavid

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

daviddavid

Same here with $()

daviddavid

And here.

daviddavid

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

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

chipx86chipx86

Typo: "attribtues" -> "attributes"

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

We can reduce the generated HTML and the test changes needed if we do: <th<% if (showExpandCollapse) { %>colspan="2"<% } …

chipx86chipx86

These are a too long for the lines. How about building the response payload higher up, before the return maybe, …

chipx86chipx86

E501 line too long (83 > 79 characters)

reviewbotreviewbot

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

chipx86chipx86

This isn't a boolean.

chipx86chipx86

E501 line too long (83 > 79 characters)

reviewbotreviewbot
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...