Add the DiffCommit model.

Review Request #6766 — Created Jan. 13, 2015 and discarded

brennie
Review Board
master
6779
reviewboard
chipx86, 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.

Ran unit tests.
Applied evolution successfully.

Description From Last Updated

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Something I realized is that we now have two things called Commit. This one and the one in reviewboard.scmtools.core. Perhaps ...

chipx86chipx86

Should localize the labels using ugettext_lazy.

chipx86chipx86

Let's use commit_type, since type is technically a reserved word.

chipx86chipx86

Is this intended to be the summary, or something else?

chipx86chipx86

I'm not sure the correct answer to this, but I'm wondering if this field is needed, given that it's only ...

chipx86chipx86

Maybe commit_id to be consistent with other uses in the codebase.

chipx86chipx86

Maybe worth allowing these to be null/blank? If used in some patchset tool (for, say, CVS/Subversion changes), we may not ...

chipx86chipx86

Let's define a constant for the length that we can reuse in all these. COMMIT_ID_LENGTH?

chipx86chipx86

Maybe to make this more useful for debugging/browsing in the admin UI, we can have this include the summary of ...

chipx86chipx86

Just a small thing, but it'd be nice to extend this a bit to talk generally about the kinds of ...

chipx86chipx86

If you use #: instead of #, we'll be able to include the comments in any auto-generated documentation (which I ...

chipx86chipx86

Can this explain what we consider to be a valid commit?

chipx86chipx86

If you make this a @cached_property (a utility provided by Django), then the summary will only ever be calculated once ...

chipx86chipx86

It's safe to split on a character that isn't in the string. In that case, you'll just get the entire ...

chipx86chipx86

Missing #:

chipx86chipx86

""" on the next line.

chipx86chipx86

We have a mix in the codebase, but I think "Returns" instead of "Gets" does a better job of saying ...

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. reviewboard/diffviewer/admin.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. reviewboard/diffviewer/models.py (Diff revision 3)
     
     
    Col: 1
     W391 blank line at end of file
    
  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
chipx86
  1. Looking good! I have a bunch of comments, but they're minor, or just design questions.

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

    Something I realized is that we now have two things called Commit. This one and the one in reviewboard.scmtools.core.

    Perhaps (in a separate change) we should rename the other one to SCMCommit.

    1. Should we rename this to DiffCommit to align with the DiffCommitResource and DraftDiffCommitResource or just leave everything alone?

    2. I think DiffCommit would be fine. Alignment is good.

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

    Should localize the labels using ugettext_lazy.

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

    Let's use commit_type, since type is technically a reserved word.

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

    Is this intended to be the summary, or something else?

    1. This is intended to be the name of the .diff file that gets uploaded for the Commit.

    2. Ah, okay. Sounds good.

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

    I'm not sure the correct answer to this, but I'm wondering if this field is needed, given that it's only really used for Subversion, and we have the info in the diffset. Since only RBTools or something custom would be able to post commits, we can probably get away with saying that all commits must have file paths relative to the DiffSet's basedir.

    1. I put this on the Commit model because the parent DiffSet is created without the diffs, but I guess its no big deal to just provide the basedir parameter to the DiffSet upon its creation. I'll drop this field from this model.

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

    Maybe commit_id to be consistent with other uses in the codebase.

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

    Maybe worth allowing these to be null/blank? If used in some patchset tool (for, say, CVS/Subversion changes), we may not have this information.

    1. Aren't patchset tools like CVS and SVN going to be incompatible with this because you won't have the notion of the changesets living outside what is checked in?

    2. CVS and SVN themselves wouldn't take advantage of this, but there are tools that sit on top of CVS/SVN/Perforce that let people work in patchsets, and they do all the hard work of managing them. Basically, think git-svn, but with a Subversion checkout. Quilt, for instance.

      That's all kinda theoretical, as I don't expect we'll ever explicitly support them. Someone out there may, but probably not us.

      However, it looks like Monotone doesn't have a concept of author vs. committer, so that looks to be a real-world example of one that needs to allow for blank values.

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

    Let's define a constant for the length that we can reuse in all these. COMMIT_ID_LENGTH?

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

    Maybe to make this more useful for debugging/browsing in the admin UI, we can have this include the summary of the commit message. (Could define a summary @property that grabs the first line from description).

  11. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
chipx86
  1. Looks great! I mostly just have some doc suggestions, but I think this is exactly what we want.

    (I'll want Steven to have a look as well, since he's been thinking about a lot of this more actively than I.)

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

    Just a small thing, but it'd be nice to extend this a bit to talk generally about the kinds of data that can be stored on here and how the commit information is used, just to flesh this out.

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

    If you use #: instead of #, we'll be able to include the comments in any auto-generated documentation (which I really want to start including at some point on the site).

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

    Can this explain what we consider to be a valid commit?

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

    If you make this a @cached_property (a utility provided by Django), then the summary will only ever be calculated once per Commit instance.

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

    It's safe to split on a character that isn't in the string. In that case, you'll just get the entire string.

    So, this can be:

    text = text.split('\n', 1)[0].strip()
    
  7. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
chipx86
  1. Last three little bits I noticed, and then I'm happy.

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

    Missing #:

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

    """ on the next line.

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

    We have a mix in the codebase, but I think "Returns" instead of "Gets" does a better job of saying what this will do. A function can get something and store it someplace without ever returning it.

  5. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/models.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/admin.py
        reviewboard/diffviewer/models.py
        reviewboard/diffviewer/evolutions/add_commit_history_fields.py
        reviewboard/diffviewer/evolutions/__init__.py
    
    
  2. 
      
brennie
Review request changed

Status: Discarded

Change Summary:

Splitting this up more.

Loading...