Summary

Add the DiffCommit model.

Review Request #6766 — Created Jan. 13, 2015 and discarded Jan. 20, 2015, 9:27 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

6779

Reviewers

Groups

reviewboard

People

chipx86, smacleod

Description

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.

Testing Done

Ran unit tests.
Applied evolution successfully.

Issues

Description	From	Last Updated
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 13, 2015, 7:40 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Jan. 14, 2015, 12:38 a.m.
Something I realized is that we now have two things called Commit. This one and the one in reviewboard.scmtools.core. Perhaps …	chipx86	Jan. 14, 2015, 2:09 p.m.
Should localize the labels using ugettext_lazy.	chipx86	Jan. 14, 2015, 2:10 p.m.
Let's use commit_type, since type is technically a reserved word.	chipx86	Jan. 14, 2015, 2:10 p.m.
Is this intended to be the summary, or something else?	chipx86	Jan. 14, 2015, 2:10 p.m.
I'm not sure the correct answer to this, but I'm wondering if this field is needed, given that it's only …	chipx86	Jan. 14, 2015, 2:12 p.m.
Maybe commit_id to be consistent with other uses in the codebase.	chipx86	Jan. 14, 2015, 3:22 p.m.
Maybe worth allowing these to be null/blank? If used in some patchset tool (for, say, CVS/Subversion changes), we may not …	chipx86	Jan. 14, 2015, 3:22 p.m.
Let's define a constant for the length that we can reuse in all these. COMMIT_ID_LENGTH?	chipx86	Jan. 14, 2015, 2:14 p.m.
Maybe to make this more useful for debugging/browsing in the admin UI, we can have this include the summary of …	chipx86	Jan. 14, 2015, 3:22 p.m.
Just a small thing, but it'd be nice to extend this a bit to talk generally about the kinds of …	chipx86	Jan. 15, 2015, 11:23 p.m.
If you use #: instead of #, we'll be able to include the comments in any auto-generated documentation (which I …	chipx86	Jan. 15, 2015, 11:17 p.m.
Can this explain what we consider to be a valid commit?	chipx86	Jan. 15, 2015, 11:15 p.m.
If you make this a @cached_property (a utility provided by Django), then the summary will only ever be calculated once …	chipx86	Jan. 15, 2015, 11:15 p.m.
It's safe to split on a character that isn't in the string. In that case, you'll just get the entire …	chipx86	Jan. 15, 2015, 11:13 p.m.
Missing #:	chipx86	Jan. 16, 2015, 8:53 p.m.
""" on the next line.	chipx86	Jan. 16, 2015, 8:53 p.m.
We have a mix in the codebase, but I think "Returns" instead of "Gets" does a better job of saying …	chipx86	Jan. 16, 2015, 8:53 p.m.

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

reviewboard/diffviewer/admin.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

Fix PEP8 review.

Commit:

b80b2a09718dbf71740df953d68bf7564a0cedb4

9408a6a7111537ee0fcaf9bef4555338f3f13cc0

Diff:

Revision 2 (+92 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Change Summary:: Update description.
Description:: The DiffSet model has been updated to be able to contain several
Commits in addition to FileDiffs. The Commit model represents a
single commit in distributed version control system (DVCS). Each
Commit has one or more FileDiffs that belong to it (as well as the
~ DiffSet)
~ DiffSet).

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

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

Change Summary:

Remvoe commit_ordinal field. It is going to be removed in the next patch anyway so may aswell do it now.

Commit:

9408a6a7111537ee0fcaf9bef4555338f3f13cc0

6a1b776ef8f14dc1ec4b9de44b1414948aa7827d

Diff:

Revision 3 (+90 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

reviewboard/diffviewer/models.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 1
 W391 blank line at end of file
```

Change Summary:

Remove trailing newline

Commit:

6a1b776ef8f14dc1ec4b9de44b1414948aa7827d

181943d4a7b4f100265cb6e4bc3addc183e5e199

Diff:

Revision 4 (+89 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Looking good! I have a bunch of comments, but they're minor, or just design questions.

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

The issue has been resolved. Show all issues

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.

brennie Jan. 14, 2015, 4:49 a.m.

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

chipx86 Jan. 14, 2015, 6:21 a.m.

I think DiffCommit would be fine. Alignment is good.

reviewboard/diffviewer/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Should localize the labels using ugettext_lazy.
```
reviewboard/diffviewer/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Let's use commit_type, since type is technically a reserved word.
```

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

The issue has been resolved. Show all issues

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

brennie Jan. 14, 2015, 4:49 a.m.

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

chipx86 Jan. 14, 2015, 6:21 a.m.
```
Ah, okay. Sounds good.
```

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

The issue has been resolved. Show all issues

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.

brennie Jan. 14, 2015, 4:49 a.m.

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.

reviewboard/diffviewer/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Maybe commit_id to be consistent with other uses in the codebase.
```

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

The issue has been resolved. Show all issues

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.

brennie Jan. 14, 2015, 4:49 a.m.

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?

chipx86 Jan. 14, 2015, 6:21 a.m.

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.

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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

Change Summary:

Address Christian's issues. Remove the CommitModel reference because it isn't in this patch.

Summary:

Add support for the Commit model.

Add support for the DiffCommit model.

Description:

		The `DiffSet` model has been updated to be able to contain several
~		`Commit`s in addition to `FileDiff`s. The `Commit` model represents a
~		single commit in distributed version control system (DVCS). Each
~		`Commit` has one or more `FileDiff`s that belong to it (as well as the
~		`DiffSet`).
	~	`DiffCommit`s in addition to `FileDiff`s. The `DiffCommit` model
	~	represents a single commit in distributed version control system
	~	(DVCS). Each `DiffCommit` has one or more `FileDiff`s 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 `Commit` model. Also add an evolution to add a
~		`RelationCounterField` to the `DiffSet` model that counts the number of
~		commits it currently has.
	~	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 `Commit`s. The `Commit`s can be as inline models of the
~		`DiffSet` model and have `FileDiff` as an inline model.
	~	view individual `DiffCommit`s. The `DiffCommit`s can be as inline
	~	models of the `DiffSet` model and have `FileDiff` as an inline model.

Testing Done:

~		Ran unit tests.
	~	Ran unit tests.
	+	Applied evolution successfully.

Commit:

181943d4a7b4f100265cb6e4bc3addc183e5e199

7546eba27f3b45f5fec086d81e8fbb5a9031404c

Diff:

Revision 5 (+123 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Change Summary:

Add a RegexValidator for commit_id fields in the DiffCommit model.

Commit:

7546eba27f3b45f5fec086d81e8fbb5a9031404c

9974639adca7d3fbfe1da7619c361b57e0fe21f6

Diff:

Revision 6 (+130 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Change Summary:

Don't use compiled re objects. Add a constant for the regex itself.

Commit:

9974639adca7d3fbfe1da7619c361b57e0fe21f6

5cb6682333d47857dcfc061b4cf7961b5bdb3c4e

Diff:

Revision 7 (+133 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

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

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

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

reviewboard/diffviewer/models.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Can this explain what we consider to be a valid commit?
```

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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()

Commit:

5cb6682333d47857dcfc061b4cf7961b5bdb3c4e

fa063362bd1cd248359b7c4b005cde3afc1f4480

Diff:

Revision 8 (+151 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Change Summary:

Don't use ^ or $ in the regex.

Summary:

Add support for the DiffCommit model.

Add the DiffCommit model.

Commit:

fa063362bd1cd248359b7c4b005cde3afc1f4480

bc44bd468919bd5ac44a6062a47e03fe80632866

Diff:

Revision 9 (+150 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Change Summary:: Add Christian and Steven to the reviewers list.
People:: chipx86
smacleod

Last three little bits I noticed, and then I'm happy.

reviewboard/diffviewer/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Missing #:
```
reviewboard/diffviewer/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
""" on the next line.
```

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

The issue has been resolved. Show all issues

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.

Change Summary:

Address Christian's issues.

Commit:

bc44bd468919bd5ac44a6062a47e03fe80632866

0d528c2869824612b692240f3e0a99ba55b7a0f4

Diff:

Revision 10 (+151 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Change Summary:

Allow multiple parents.

Commit:

0d528c2869824612b692240f3e0a99ba55b7a0f4

3aaf9ed8840d9e924ffdb622b469b0d89382be5e

Diff:

Revision 11 (+173 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Change Summary:

Forgot to remove merge_parent_id field.

Commit:

3aaf9ed8840d9e924ffdb622b469b0d89382be5e

8f6c1fb5a27a1236ffb09d3e2f3b549738ba01ff

Diff:

Revision 12 (-4)

Show changes

reviewboard/diffviewer/models.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/models.py

Change Summary:

Fix the attached diff

Diff:

Revision 13 (+169 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Change Summary:

Rip out datetime fields. When we go to repro the commit, we will stick the header into the extra_data because that stuff can change over time

Commit:

8f6c1fb5a27a1236ffb09d3e2f3b549738ba01ff

a78656edefd431fb24485c3874bfc3b639110c94

Diff:

Revision 14 (+162 -5)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.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



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

Status:: Discarded
Change Summary:: Splitting this up more.