Add cumulative diff API

Review Request #10233 — Created Oct. 15, 2018 and submitted

brennie
Review Board
release-4.0.x
10216, 10136
10240, 10245
2edb57f...
reviewboard

Previously, we uploaded only per-commit FileDiffs to review requests
with history support. Now, we also require API clients to upload a
"cumulative diff" -- that is, a diff that represents the squashed commit
range -- to the draft diff API. This also results in the "finalization"
of the commit series, which prevents uploading of additional commits, as
this would result in a cumulative diff that does not represent the
entire uploaded range.

Ran unit tests.

Description From Last Updated

F401 'django.utils.functional.cached_property' imported but unused

reviewbotreviewbot

F841 local variable 'commit' is assigned to but never used

reviewbotreviewbot

F841 local variable 'commit' is assigned to but never used

reviewbotreviewbot

F401 'reviewboard.reviews.signals.review_request_published' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.fields.BooleanFieldType' imported but unused

reviewbotreviewbot

F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

F811 redefinition of unused 'test_put_finalize_invalid_validation_info_json_format' from line 649

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

F821 undefined name 'commit'

reviewbotreviewbot

F821 undefined name 'commit'

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'django.utils.functional.cached_property' imported but unused

reviewbotreviewbot

F841 local variable 'commit' is assigned to but never used

reviewbotreviewbot

F841 local variable 'commit' is assigned to but never used

reviewbotreviewbot

F401 'reviewboard.diffviewer.models.DiffSet' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.signals.review_request_published' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.fields.BooleanFieldType' imported but unused

reviewbotreviewbot

F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

F811 redefinition of unused 'test_put_finalize_invalid_validation_info_json_format' from line 649

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

F841 local variable 'rsp' is assigned to but never used

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

This is to prevent interdiffs when viewing a subset of commits, right? Can you add a comment and assertion string ...

chipx86chipx86

This can be simplified a bit (might as well bail early if self.extra_data is empty): return (self.extra_data and self.extra_data.get(self._FINALIZED_COMMIT_SERIES_KEY, False))

chipx86chipx86

Comma after True.

chipx86chipx86

Remove one of the "been"s.

chipx86chipx86

Can you change "incurred" to "performed"? Seems more appropriate.

chipx86chipx86

Is the first implementation Django 1.6, and the other 1.8+? If so, can you tag with: if blah: # Django ...

chipx86chipx86

related

chipx86chipx86

per_commit_files

chipx86chipx86

objects

chipx86chipx86

This should be moved in one level, right?

chipx86chipx86

Swap these.

chipx86chipx86

Can you change this to use the standard styling? for ... should be on its own line in each case.

chipx86chipx86

Only one blank line.

chipx86chipx86

Missing ugettext.

chipx86chipx86

Two newlines. (One on the next line, to help with readability.)

chipx86chipx86

Double backticks for the field names, and for true (which should be lowercase, since it's not Python's version).

chipx86chipx86

Typo: "cumulative"

chipx86chipx86

Missing API docs.

chipx86chipx86

We should be saying "commit history support".

chipx86chipx86

I feel like we should be moving this out into a proper method somewhere that can be called on DiffSet. ...

chipx86chipx86

The backtick is in the wrong place.

chipx86chipx86

No blank line.

chipx86chipx86

Let's use .exists(). It can be optimized better.

chipx86chipx86

Yes, it should be. We use DOES_NOT_EXIST for resources that don't exist.

chipx86chipx86

We quote the field name here, but not in the other errors above or below.

chipx86chipx86

Can you expand this into the standard multi-line format?

chipx86chipx86

Alphabetical order.

chipx86chipx86

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused

reviewbotreviewbot

F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused

reviewbotreviewbot

F401 'django.utils.six' imported but unused

reviewbotreviewbot

F841 local variable 'errors_by_code' is assigned to but never used

reviewbotreviewbot

Can you indent the right-hand side of the != one level?

chipx86chipx86

"commit history"

chipx86chipx86

"commit history"

chipx86chipx86

"commit history"

chipx86chipx86

Probably best as an itemized list.

chipx86chipx86

This could probably just be a single line.

chipx86chipx86

"commit history"

chipx86chipx86

"commit history"

chipx86chipx86

"commit history"

chipx86chipx86

Let's add a blank line between these.

chipx86chipx86

The second line should be aligned with the first.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Summary:

-WIP: Commit Series Finalization API
+Add cumulative diff API

Description:

~  

WIP

  ~

Previously, we uploaded only per-commit FileDiffs to review requests

  + with history support. Now, we also require API clients to upload a
  + "cumulative diff" -- that is, a diff that represents the squashed commit
  + range -- to the draft diff API. This also results in the "finalization"
  + of the commit series, which prevents uploading of additional commits, as
  + this would result in a cumulative diff that does not represent the
  + entire uploaded range.

Testing Done:

  +

Ran unit tests.

Depends On:

-10232 - Split filediff creation into preparation and finalization methods
+10216 - Remove DiffCommit.file_count and DiffSet.file_count fields
+10136 - Switch between selected commit revisions shown in the diffviewer

Commit:

-321daa834786320d19ffa9ac7638ddc57004d5dd
+5bf70826895fcffb72c9525db1ae0bad8b52a505

Diff:

Revision 2 (+1232 -89)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 6)
     
     

    This is to prevent interdiffs when viewing a subset of commits, right? Can you add a comment and assertion string describing this? It'll help if we need to change it later.

  3. reviewboard/diffviewer/models/diffset.py (Diff revision 6)
     
     
     
     
     

    This can be simplified a bit (might as well bail early if self.extra_data is empty):

    return (self.extra_data and
            self.extra_data.get(self._FINALIZED_COMMIT_SERIES_KEY, False))
    
  4. Comma after True.

  5. Remove one of the "been"s.

  6. Can you change "incurred" to "performed"? Seems more appropriate.

  7. reviewboard/diffviewer/models/diffset.py (Diff revision 6)
     
     
     

    Is the first implementation Django 1.6, and the other 1.8+? If so, can you tag with:

    if blah:
        # Django == 1.6
    else:
        # Django >= 1.8
    

    Same further down.

  8. per_commit_files

  9. This should be moved in one level, right?

    1. Why? It doesn't matter.

  10. Swap these.

  11. reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6)
     
     
     
     
     

    Can you change this to use the standard styling? for ... should be on its own line in each case.

  12. reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6)
     
     
     
     
     

    Only one blank line.

  13. Missing ugettext.

  14. Two newlines. (One on the next line, to help with readability.)

  15. reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
     
     
     

    Double backticks for the field names, and for true (which should be lowercase, since it's not Python's version).

  16. Typo: "cumulative"

  17. reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
     
     
     

    Missing API docs.

  18. We should be saying "commit history support".

  19. I feel like we should be moving this out into a proper method somewhere that can be called on DiffSet. The reason being that for pull request support, we're going to need to be able to finalize commits. Ideally, the REST API would be a somewhat-thin wrapper around a proper DiffSet API.

    1. Ive ripped out everything except the validation portion. I dont know that it feels correct to raise ValidationErrors in a non-form context. Should this be a form?

    2. forms re-imports ValidationError for historical reasons/convenience, but it's a core-level exception in Django. It's okay to raise them if it's validation-related.

  20. The backtick is in the wrong place.

  21. reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
     
     
     
     

    No blank line.

  22. Let's use .exists(). It can be optimized better.

  23. reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
     
     
     

    Yes, it should be. We use DOES_NOT_EXIST for resources that don't exist.

  24. We quote the field name here, but not in the other errors above or below.

  25. Can you expand this into the standard multi-line format?

  26. Alphabetical order.

  27. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/models/diffset.py (Diff revision 11)
     
     
     

    Can you indent the right-hand side of the != one level?

  3. reviewboard/webapi/resources/draft_diff.py (Diff revision 11)
     
     
     
     
     
     

    Probably best as an itemized list.

  4. reviewboard/webapi/resources/draft_diff.py (Diff revision 11)
     
     
     
     

    This could probably just be a single line.

  5. "commit history"

  6. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/webapi/resources/draft_diff.py (Diff revision 12)
     
     
     

    Let's add a blank line between these.

  3. reviewboard/webapi/resources/draft_diff.py (Diff revision 12)
     
     
     

    The second line should be aligned with the first.

  4. 
      
brennie
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (725c597)
Loading...