Add DiffCommit model for review requests with commit histories.

Review Request #6815 — Created Jan. 20, 2015 and submitted

brennie
Review Board
master
6933, 6931, 6816, 6934
aebd320...
reviewboard
chipx86, mike_conley, smacleod

The DiffSet model has been updated to be able to contain several
DiffCommits in addition to FileDiffs. The DiffCommit model
represents a single commit in distributed version control system
(DVCS). Each DiffCommit has one or more FileDiffs that belong to
it (as well as the DiffSet).

Add an evolution to add a foreign key on the FileDiff model to point
to an optional DiffCommit model. Also add an evolution to add a
RelationCounterField to the DiffSet model that counts the number
of DiffCommits it currently has.

Update the reviewboard.diffviewer.admin module to add the ability to
view individual DiffCommits. The DiffCommits can be as inline
models of the DiffSet model and have FileDiff as an inline model.

Add unit tests for creating empty DiffSets as well as creating
DiffCommits from .diff files and uploading via the
UploadDiffCommit form.

Updated the ValidateDiffResource with respect to the new refactoring
of the DiffSetManager.

Unit tests pass.
Applied evolution successfully.

  • 0
  • 0
  • 47
  • 2
  • 49
Description From Last Updated
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. reviewboard/diffviewer/tests.py (Diff revision 1)
     
     
     'MergeParent' imported but unused
    
  3. 
      
brennie
brennie
chipx86
  1. Some new code, so some new comments. Sorry! :) Looking great, though.

  2. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    Can you add a docstring to this?

  3. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    User-visible strings typically use double quotes, so the original version of this is preferred. We also try to avoid changing localized strings unless we really need to, to help keep translators from wanting to kill us in our sleep.

  4. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    The labels in these forms have inconsistent casing. For example, "Parent Diff" vs. "Commit type" below. This is not a new problem, but one we should address here.

    Looking at other forms, it seems we're more standardized now on sentence casing (e.g., "Parent diff"), so I'd update all the field labels for that.

  5. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     
     
     
     

    This can probably be removed, I think? Looks like the argument defaults and order are all the same as the parent's, so we'll fall back on that.

  6. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    While ''' technically works, docstrings traditionally always use """.

  7. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    "Parent commit ID" might be a bit more consistent here.

  8. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    Can you go into some detail on what this is checking?

  9. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     
     

    id is a reserved keyword in Python, so we should call this something different. merge_parent_id probably.

  10. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    Should use _(...)

  11. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    ValidationError can take lists of errors and display them appropriately, so you can pass errors directly to it.

  12. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     

    clean_* functions are expected to return the result. Django will handle updating cleaned_data.

  13. reviewboard/diffviewer/managers.py (Diff revision 1)
     
     

    Should this be "DiffSetManager"?

  14. reviewboard/diffviewer/managers.py (Diff revision 1)
     
     

    Instead of referencing the parameter, how about "Creates a list of FileDiffs for a list of files."?

  15. reviewboard/diffviewer/managers.py (Diff revision 1)
     
     
     
     
     
     

    When this gets turned into documentation via Sphinx down the road, it's probably going to appear as a paragraph with lots of dashes inbetween the key names. For that, you'll need to use a ReST-style list, like:

    When calling blah blah:
    
    * basedir
    * diffset_history
    * base_commit_id
    
  16. reviewboard/diffviewer/managers.py (Diff revision 1)
     
     

    The "F" in "parent_diff_File_contents" should be lowercase.

  17. reviewboard/diffviewer/managers.py (Diff revision 1)
     
     
     
     

    We can reduce database overhead by using bulk_create. The code will be similar, but you'd create a local instance of MergeParent (no .objects.create()), put those into a list, and then create them in one go afterward using MergeParent.objects.bulk_create(merge_parents)

  18. reviewboard/diffviewer/models.py (Diff revision 1)
     
     
     
     

    No blank line needed here.

  19. reviewboard/diffviewer/models.py (Diff revision 1)
     
     
     

    This looks like a constant, but it's really a function. Checking with how Django uses these, I think this should be called validate_commit_id.

    Hmm, I also wonder if this should just go in a validators.py, so it's a bit more reusable without needing to reference DiffCommit. What do you think?

    1. Perhaps, but then there is the issue of the COMMIT_ID_RE, which IMO belongs on the DiffCommit class, but if we leave it there we now have a circular import. For now I'm going to leave it on DiffCommit and we can talk about this later.

  20. reviewboard/diffviewer/models.py (Diff revision 1)
     
     

    Should wrap in _(...)

  21. reviewboard/diffviewer/models.py (Diff revision 1)
     
     

    Can you add a docstring for this class?

  22. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
  1. At about the 75% through this review my brain was just shutting it down. I gave it my best shot but I should probably look over everything again at some point.

  2. reviewboard/diffviewer/admin.py (Diff revision 2)
     
     
     

    from reviewboard.diffviewer.models import (DiffCommit,
                                               DiffSet,
                                               DiffSetHistory,
                                               FileDiff)
    
  3. reviewboard/diffviewer/forms.py (Diff revision 2)
     
     

    This isn't long enough, author will most likely be of the form Firstname LastName <email> and emails can be like 254 characters alone.

  4. reviewboard/diffviewer/forms.py (Diff revision 2)
     
     

    Length, same as above.

  5. reviewboard/diffviewer/forms.py (Diff revision 2)
     
     
     
     

    Label and help text need to be updated.

  6. reviewboard/diffviewer/managers.py (Diff revision 2)
     
     

    It's not obvious to me why this is a good place to find permissions errors? In fact it seems like a really strange place?

    What am I missing?

    1. This is refactored from somewhere else that wasn't removed from the diff.

  7. reviewboard/diffviewer/models.py (Diff revision 2)
     
     

    Aside: I know we do this all over the code base, but reading PEP8 today, this is actually frowned upon (including more than one import on the same line like that)

  8. reviewboard/diffviewer/models.py (Diff revision 2)
     
     
     

    Too short

  9. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/diffviewer/managers.py (Diff revisions 1 - 8)
     
     
     
     
     
     
     
     

    If save is False, we'll never actually save any of the merge parents, since they're not returned anywhere. Is this okay? If so, we should be sure to document it in the docstring.

  3. reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
     
     
     
     

    Does this not fit on one line?

  4. reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
     
     
     

    Blank line between these.

  5. reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
     
     
     
     

    Private functions should go after the public ones.

  6. reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
     
     
     
     
     

    Attributes should go before properties/functions. Same with ones below.

  7. reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
     
     

    Can we add the comment here about the conversion to UTC as well?

  8. reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
     
     
     
     
     
     
     
     
     

    This is really an API detail. I think it should be limited to the resources, rather than having the support here and in the form. Internally, everything else should be solely using the constants.

    1. The uploading form has to support the 'change' and 'merge' values, so should the parsing logic be moved into the form's clean_commit_type method?

      I ask because the form is the object that creates the DiffCommit object, via the UploadDiffCommitForm.create method (which mirrors the UploadDiffForm.create method).

      For now, I'm going to move the logic into the form (pending further discussion) and leave this issue open.

    2. Yeah, the form's a good place for this.

  9. reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
     
     

    Should also go after all public functions.

  10. reviewboard/diffviewer/tests.py (Diff revisions 1 - 8)
     
     
     

    The nose import is old, but both of these should go before the froms, in alphabetical order.

  11. reviewboard/diffviewer/tests.py (Diff revisions 1 - 8)
     
     
     

    This can easily fit on one line.

  12. reviewboard/diffviewer/forms.py (Diff revision 8)
     
     

    This should go before the from statements.

  13. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. reviewboard/diffviewer/forms.py (Diff revision 9)
     
     
    Col: 1
     W391 blank line at end of file
    
  3. reviewboard/diffviewer/models.py (Diff revision 9)
     
     
     'ValidationError' imported but unused
    
  4. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
chipx86
  1. Just a few things left.

  2. reviewboard/diffviewer/forms.py (Diff revision 12)
     
     
     

    The raise is over-indented.

    Missing a %s.

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

    Maybe put the comment before the assert.

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

    We should have a docstring on this describing its purpose.

  5. reviewboard/diffviewer/tests.py (Diff revision 12)
     
     
     
     
     
     

    Over-indentation problems.

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

    "Testing", and no trailing period.

  7. reviewboard/diffviewer/tests.py (Diff revision 12)
     
     
     
     
     
     

    Too many blank lines.

  8. reviewboard/diffviewer/tests.py (Diff revision 12)
     
     

    "Testing", and no trailing period.

  9. reviewboard/diffviewer/tests.py (Diff revision 12)
     
     
     

    If we're just grabbing the commit_id from each of these, you can probably do:

    commit.merge_parent_ids.values_list('commit_id', flat=True)
    
  10. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/webapi/resources/validate_diff.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/webapi/resources/validate_diff.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. reviewboard/diffviewer/tests.py (Diff revision 13)
     
     
     'os' imported but unused
    
  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/webapi/resources/validate_diff.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/tests.py
        reviewboard/webapi/resources/validate_diff.py
        reviewboard/diffviewer/evolutions/__init__.py
        reviewboard/diffviewer/forms.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/managers.py
    
    
  2. 
      
chipx86
  1. Woot! Looks good. Ship It!

    So we need a branch, both for RB and RBTools. For consistency, they should be the same. While this set of changes are about commit histories, we may want something more general for all the work happening for DVCS-related improvements. Given that, maybe just "dvcs"?

  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (bc9d45a)
Loading...