- Download Diff

Summary:

Require validation for DiffCommits when posting

Review Request #9920 — Created May 9, 2018 and submitted Sept. 2, 2018, 12:04 p.m.

Information
Owner:	brennie
Repository:	Review Board
Branch:	release-4.0.x
Bugs:
Depends On:	~~10119~~, ~~9905~~
Blocks:	9921
Commit:	1e5bd7d...
Reviewers
Groups:	reviewboard
People:

Description

We now require that diffs for an entire series of commits be validated
before posting to the DraftDiffCommitResource. Validation is performed
by making a series of POST requests to the
ValidateDiffCommitResource, each of which returns validation data to
be included in the next request. The validation data from each request
to the validation resource must then be used to POST to the
DraftDiffCommitResource.

This ensures that we do not upload broken diffs and also allows us to
upload more complex commit series. Previously, only linear commit series
where each commit modified different files were capable of being
uploaded. Now all linear commit series can be uploaded.

Testing Done

Posted /r/9609/ through to this commit to a single review request.
Ran unit tests.

Issues

Description	From	Last Updated
Can you rename the file to "commit_utils.py"?	chipx86	July 10, 2018, 12:03 p.m.
F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused	reviewbot	May 9, 2018, 10:47 p.m.
This is way too generic a name. I'd prefer get_file_exists_for_validation. Not sure this file is the right place, actually. This …	chipx86	July 10, 2018, 11:51 a.m.
This can wrap.	david	July 3, 2018, 4:12 p.m.
You can now split this across lines at a .	chipx86	July 10, 2018, 12:03 p.m.
bool	chipx86	July 10, 2018, 12:03 p.m.
Did you mean to unindent the line? Doesn't seem like it would particularly matter in this change.	chipx86	July 10, 2018, 11:54 a.m.
Can you import specifically waht you need from the module, instead of the module itself?	chipx86	July 10, 2018, 8:14 p.m.
I imagine this should be a HiddenInput? We don't want users mucking about in it if we display this anywhere.	chipx86	July 10, 2018, 12:10 p.m.
"Validation metadata"	chipx86	July 10, 2018, 12:10 p.m.
The text should specifically be self.fields['validation_info'].error_messages['required']. No need for ugettext.	chipx86	July 10, 2018, 12:12 p.m.
Shouldn't include literals in summaries.	chipx86	July 10, 2018, 12:12 p.m.
Extra period on this line.	david	July 3, 2018, 4:12 p.m.
Too many periods.	chipx86	July 10, 2018, 12:12 p.m.
"exceptions"	chipx86	July 10, 2018, 12:12 p.m.
Maybe better as: if not validation_info: return {} # .. rest of the method	chipx86	July 10, 2018, 12:13 p.m.
b64decode can also raise TypeError.	chipx86	July 10, 2018, 12:13 p.m.
The return can be inline.	chipx86	July 10, 2018, 12:13 p.m.
Maybe "A form for validating DiffCommits."	chipx86	July 10, 2018, 12:13 p.m.
"Parent diff"	chipx86	July 10, 2018, 12:14 p.m.
"Validation metadata"	chipx86	July 10, 2018, 12:14 p.m.
"Base commit ID"	chipx86	July 10, 2018, 12:14 p.m.
No literals in summaries.	chipx86	July 10, 2018, 12:14 p.m.
Too many periods.	chipx86	July 10, 2018, 12:14 p.m.
Same comments as above. Seems we have some stuff we could easily pull out into a base class?	chipx86	July 10, 2018, 12:18 p.m.
This seems like code we have elsewhere. I'd like to deduplicate this if possible.	chipx86	July 10, 2018, 12:24 p.m.
We should pull out the specific functions we need.	chipx86	July 10, 2018, 8:14 p.m.
This can be wrapped.	david	July 3, 2018, 4:13 p.m.
This can be broken into multiple lines now at any .	chipx86	July 10, 2018, 12:24 p.m.
No literals in summaries.	chipx86	July 10, 2018, 12:25 p.m.
Mixing import groups here.	chipx86	July 10, 2018, 12:27 p.m.
Shouldn't DiffCommit be handling this?	chipx86	July 10, 2018, 12:11 p.m.
Ordering issues.	chipx86	July 10, 2018, 12:27 p.m.
The DIFF_* should be in alphabetical order.	chipx86	July 10, 2018, 12:27 p.m.
E501 line too long (90 > 79 characters)	reviewbot	July 11, 2018, 12:38 p.m.
E501 line too long (84 > 79 characters)	reviewbot	July 11, 2018, 12:38 p.m.
E126 continuation line over-indented for hanging indent	reviewbot	July 24, 2018, 5:42 p.m.
Can we include the error details in this? That'd help with debugging.	chipx86	July 31, 2018, 2:27 p.m.
Trailing comma.	chipx86	July 31, 2018, 2:27 p.m.
Can we use keyword arguments for all calls to this, here and everywhere else in the change? It's a complex …	chipx86	July 31, 2018, 2:31 p.m.
This will create a tuple of strings.	chipx86	July 31, 2018, 2:32 p.m.
This creates a tuple.	chipx86	July 31, 2018, 2:32 p.m.
This too.	chipx86	July 31, 2018, 2:32 p.m.
Can you use references throughout the docstrings whenever you're referencing another resource? They're in the form of webapi2.0-<blah>-resource or webapi2.0-<blah>-list-resource.	chipx86	Aug. 13, 2018, 4:17 p.m.
It doesn't really return "OK." I feel that can be misleading. We probably can remove the whole "either OK or …	chipx86	Aug. 13, 2018, 4:16 p.m.
Double backticks for validation_info. Also, "field" and not "parameter."	chipx86	Aug. 13, 2018, 4:17 p.m.
Can you switch to a named logger for the module?	chipx86	Aug. 13, 2018, 4:18 p.m.
Missing trailing comma.	chipx86	Aug. 13, 2018, 4:18 p.m.
F841 local variable 'validation_info' is assigned to but never used	reviewbot	Aug. 13, 2018, 4:30 p.m.
F821 undefined name 'on_info'	reviewbot	Aug. 13, 2018, 4:30 p.m.
E501 line too long (120 > 79 characters)	reviewbot	Aug. 13, 2018, 4:30 p.m.
F841 local variable 'validation_info' is assigned to but never used	reviewbot	Aug. 13, 2018, 4:30 p.m.
F821 undefined name 'on_info'	reviewbot	Aug. 13, 2018, 4:30 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Aug. 16, 2018, 8:14 p.m.
E501 line too long (91 > 79 characters)	reviewbot	Aug. 16, 2018, 8:14 p.m.
W391 blank line at end of file	reviewbot	Aug. 16, 2018, 8:14 p.m.
F841 local variable 'parent_diff' is assigned to but never used	reviewbot	Aug. 17, 2018, 2:57 p.m.
Newlines in exceptions are generally not going to be handled well, either in HTML or in logs. Maybe: Could not …	chipx86	Aug. 17, 2018, 7:05 p.m.
Missing trailing comma.	chipx86	Aug. 17, 2018, 7:06 p.m.
You can consolidate these.	chipx86	Aug. 17, 2018, 7:13 p.m.
Missing a trailing period.	chipx86	Aug. 17, 2018, 7:07 p.m.
Name should be GetFileExistsInHistoryTests.	chipx86	Aug. 17, 2018, 7:09 p.m.
For here and any others, can we use called_with() and check the arguments?	chipx86	Aug. 17, 2018, 7:08 p.m.
Extra blank line here.	chipx86	Aug. 17, 2018, 7:09 p.m.
Missing a period.	chipx86	Aug. 17, 2018, 7:09 p.m.
Extra blank line.	chipx86	Aug. 17, 2018, 7:09 p.m.
You can align these starting on the initial call. There should be plenty of room.	chipx86	Aug. 17, 2018, 7:09 p.m.
Can you include the repository ID? That'll help with RBCommons.	chipx86	Aug. 17, 2018, 7:10 p.m.
This line's too long. You can break at a .	chipx86	Aug. 17, 2018, 7:10 p.m.
Can you explain why here?	chipx86	Aug. 17, 2018, 7:11 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Aug. 17, 2018, 7:33 p.m.
F821 undefined name 'revision'	reviewbot	Aug. 17, 2018, 7:33 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Aug. 17, 2018, 7:39 p.m.
This sounds confusing. Can you say: The "%s" repository does not support review requests with commit history.	chipx86	Aug. 27, 2018, 5:18 p.m.
Blank line between these.	chipx86	Aug. 27, 2018, 5:19 p.m.

flake8 passed.

JSHint passed.

Change Summary:

Fix unit test failures.

Depends On:

+	~~9905 - Add a DVCS capability gated behind the DVCS feature~~

Commit:

-	c73a8395ee5a8c463733f9933cd1693ef85a84a2
+	26ef4844d2fe1de202a5e98cb3e484ea1f1e2bd2

Diff:

Revision 2 (+1703 -9)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commitutils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/testing/testcase.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_validate_diff_commit.py (Diff revision 2)
Show all issues
```
F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused
```

Commit:

-	26ef4844d2fe1de202a5e98cb3e484ea1f1e2bd2
+	90b94c870b0149f30290da2bd19efffcf43bead4

Diff:

Revision 3 (+1714 -9)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commitutils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/testing/testcase.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

diff_commit_count -> diffcommit_count

Commit:

-	90b94c870b0149f30290da2bd19efffcf43bead4
+	ab0115dee1837598f148d9a5cb3ebc3e39611400

Diff:

Revision 4 (+1714 -9)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commitutils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/testing/testcase.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/commitutils.py (Diff revision 4)
Show all issues
```
This can wrap.
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
Extra period on this line.
```
reviewboard/diffviewer/managers.py (Diff revision 4)
Show all issues
```
This can be wrapped.
```

Commit:

-	ab0115dee1837598f148d9a5cb3ebc3e39611400
+	217c9d617955dd76067c65ba86faf201bc86c73f

Diff:

Revision 5 (+1717 -10)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commitutils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/testing/testcase.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Apparently I never published this review, which I made a while back. My apologies if comments are pretty stale now. I'll do another as I get through the chain back to this.

Show all issues

Can you rename the file to "commit_utils.py"?

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

Show all issues

This is way too generic a name. I'd prefer get_file_exists_for_validation.

Not sure this file is the right place, actually. This is about validation and not commits themselves.

brennie July 10, 2018, 12:02 p.m.

Except this isnt only used for validation. This is used for reviewboard/diffviewer/managers.py as well when we create the actual commits.

reviewboard/diffviewer/commitutils.py (Diff revision 4)
Show all issues
```
You can now split this across lines at a .
```
reviewboard/diffviewer/commitutils.py (Diff revision 4)
Show all issues
```
bool
```

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

Show all issues

Did you mean to unindent the line? Doesn't seem like it would particularly matter in this change.

brennie July 10, 2018, 12:02 p.m.

Yes, I did. In the old behaviour, we only appended to filediffs when we weren't validating and therefore would return an empty list. This makes it so we append to the list so we can instantiate them during validation without creating them in teh db.

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

Show all issues

Can you import specifically waht you need from the module, instead of the module itself?

brennie July 10, 2018, 12:09 p.m.

So we are using get_file_exists from that module (and re: my other comment, I don't believe renaming it to what you suggested is suitable), but the code in this module does:

        get_file_exists = partial(commit_utils.get_file_exists,
                                  validation_info or {},
                                  self.repository,
                                  self.cleaned_data['parent_id'])

        # ...

        return create_filediffs(
            # ...
            get_file_exists=get_file_exists,
            # ...
        )


which would override the global name.
I personally don't see a big deal in importing the module vs importing bits of it in this case.

chipx86 July 10, 2018, 1:38 p.m.

It could be _get_file_exists in this case. It's not a big deal, but it's worth noting that we used to import this way from diffutils all over the place and it made it a bit more annoying to maintain. We eventually did a cleanup. I feel like as commit_utils grows, we're likely going to do the same thing.

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

Show all issues

I imagine this should be a HiddenInput? We don't want users mucking about in it if we display this anywhere.

reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
"Validation metadata"
```

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

Show all issues

The text should specifically be self.fields['validation_info'].error_messages['required']. No need for ugettext.

brennie July 10, 2018, 12:02 p.m.
```
TIL.
```

reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
Shouldn't include literals in summaries.
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
Too many periods.
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
"exceptions"
```

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

Show all issues

Maybe better as:

if not validation_info:
    return {}

# .. rest of the method

reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
b64decode can also raise TypeError.
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
The return can be inline.
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
Maybe "A form for validating DiffCommits."
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
"Parent diff"
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
"Validation metadata"
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
"Base commit ID"
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
No literals in summaries.
```
reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
Too many periods.
```

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

Show all issues

Same comments as above.

Seems we have some stuff we could easily pull out into a base class?

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

Show all issues

This seems like code we have elsewhere. I'd like to deduplicate this if possible.

reviewboard/diffviewer/managers.py (Diff revision 4)
Show all issues
```
We should pull out the specific functions we need.
```
1. brennie July 10, 2018, 12:24 p.m.
  Again, see previous reply.
reviewboard/diffviewer/managers.py (Diff revision 4)
Show all issues
```
This can be broken into multiple lines now at any .
```
reviewboard/diffviewer/tests/test_commitutils.py (Diff revision 4)
Show all issues
```
No literals in summaries.
```
reviewboard/diffviewer/tests/test_forms.py (Diff revision 4)
Show all issues
```
Mixing import groups here.
```
reviewboard/testing/testcase.py (Diff revision 4)
Show all issues
```
Shouldn't DiffCommit be handling this?
```
reviewboard/webapi/resources/validate_diff_commit.py (Diff revision 4)
Show all issues
```
Ordering issues.
```
reviewboard/webapi/resources/validate_diff_commit.py (Diff revision 4)
Show all issues
```
The DIFF_* should be in alphabetical order.
```

Change Summary:

Addressed the majority of open issues.

Commit:

-	217c9d617955dd76067c65ba86faf201bc86c73f
+	efd916a5a0e84bc2a7904c7970ecbf576d1933e0

Diff:

Revision 6 (+1711 -29)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Addressed remaining feedback

Commit:

-	efd916a5a0e84bc2a7904c7970ecbf576d1933e0
+	1ac95ff3d8d386703ac12ca8aaadc9fce9e515b3

Diff:

Revision 7 (+1711 -29)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 7)
Show all issues
```
E501 line too long (90 > 79 characters)
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 7)
Show all issues
```
E501 line too long (84 > 79 characters)
```

Change Summary:

Fixup Flake8 issues

Commit:

-	1ac95ff3d8d386703ac12ca8aaadc9fce9e515b3
+	b91eaa8d612bfcedd7a453a5e531e36964349ce0

Diff:

Revision 8 (+1713 -29)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Keep track of add/modify/remove in parent diff

Commit:

-	b91eaa8d612bfcedd7a453a5e531e36964349ce0
+	8f0ea175b4fa6f17d0516a4bb5d62ef7a5ef968a

Diff:

Revision 9 (+1923 -29)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_validate_diffcommit.py (Diff revision 9)
Show all issues
```
E126 continuation line over-indented for hanging indent
```

Change Summary:

flake8

Commit:

-	8f0ea175b4fa6f17d0516a4bb5d62ef7a5ef968a
+	255222c19ecfa14618aba5461f65a599aacac551

Diff:

Revision 10 (+1923 -29)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

Can we include the error details in this? That'd help with debugging.

reviewboard/diffviewer/forms.py (Diff revision 10)
Show all issues
```
Trailing comma.
```

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 10)

Show all issues

Can we use keyword arguments for all calls to this, here and everywhere else in the change? It's a complex enough function where we should ensure calls are self-documenting.

brennie July 31, 2018, 2:32 p.m.

We can't do it but can't use kwargs in partial() because otherwise they will not be used in the correct positions.

reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
Show all issues
```
This will create a tuple of strings.
```
reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
Show all issues
```
This creates a tuple.
```
reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
Show all issues
```
This too.
```

reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)

Show all issues

Can you use references throughout the docstrings whenever you're referencing another resource? They're in the form of webapi2.0-<blah>-resource or webapi2.0-<blah>-list-resource.

reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)

Show all issues

It doesn't really return "OK." I feel that can be misleading. We probably can remove the whole "either OK or an error" and just say "a result representing whether ..."

brennie Aug. 13, 2018, 4:20 p.m.
```
I mean, it does return 200 OK.
```

reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
Show all issues
```
Double backticks for validation_info.
Also, "field" and not "parameter."
```
reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
Show all issues
```
Can you switch to a named logger for the module?
```
reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 10)
Show all issues
```
Missing trailing comma.
```

Change Summary:

Addressed issues, added docs.

Commit:

-	255222c19ecfa14618aba5461f65a599aacac551
+	74eab7d1571527aba49415f4f887f065dd0900a8

Diff:

Revision 11 (+1924 -29)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_forms.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 11)
Show all issues
```
F841 local variable 'validation_info' is assigned to but never used
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 11)
Show all issues
```
F821 undefined name 'on_info'
```
reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 11)
Show all issues
```
E501 line too long (120 > 79 characters)
```

Change Summary:

actually commit changes >.>

Commit:

-	74eab7d1571527aba49415f4f887f065dd0900a8
+	55fd26fa7172fbfc4137b0c2dc9190add0a6831a

Diff:

Revision 12 (+1934 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 12)
Show all issues
```
F841 local variable 'validation_info' is assigned to but never used
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 12)
Show all issues
```
F821 undefined name 'on_info'
```

Change Summary:

fixups

Commit:

-	55fd26fa7172fbfc4137b0c2dc9190add0a6831a
+	564905b15f0b53120c34a7f55874c6da7de28496

Diff:

Revision 13 (+1934 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

fix unit tests

Commit:

-	564905b15f0b53120c34a7f55874c6da7de28496
+	46b58240dddcd476a501b02e14637bfcf0ef8625

Diff:

Revision 14 (+1938 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Depends On:

+	~~10119 - Compute pre-patch files for FileDiffs in commit histories~~

Change Summary:


Do not include parent diff into in validation_info
Allow parent_diff in subsequent commit validation

Commit:

-	46b58240dddcd476a501b02e14637bfcf0ef8625
+	25c580136748405934fa83a92cd8a2ad5763f26b

Diff:

Revision 15 (+1905 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 15)
Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_validate_diffcommit.py (Diff revision 15)
Show all issues
```
E501 line too long (91 > 79 characters)
```
reviewboard/webapi/tests/test_validate_diffcommit.py (Diff revision 15)
Show all issues
```
W391 blank line at end of file
```

Commit:

-	25c580136748405934fa83a92cd8a2ad5763f26b
+	2c175870b6f9daf20059e7c2ce98d2e601ac6b78

Diff:

Revision 16 (+1906 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Clean up unit tests due to validation changes. No longer process parent filediffs in validator

Commit:

-	2c175870b6f9daf20059e7c2ce98d2e601ac6b78
+	4ca0eb376ddd68f25e24767e89afa03a40dfd336

Diff:

Revision 17 (+1895 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/forms.py (Diff revision 17)

Show all issues

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

Commit:

-	4ca0eb376ddd68f25e24767e89afa03a40dfd336
+	858f8d77098b2ad76f1e06d3d518111923b05905

Diff:

Revision 18 (+1899 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/forms.py (Diff revision 18)

Show all issues

Newlines in exceptions are generally not going to be handled well, either in HTML or in logs. Maybe:

Could not parse validation info %(validation_info)s: %(exc)s

reviewboard/diffviewer/forms.py (Diff revision 18)
Show all issues
```
Missing trailing comma.
```

reviewboard/diffviewer/forms.py (Diff revision 18)

Show all issues

You can consolidate these.

reviewboard/diffviewer/managers.py (Diff revision 18)
Show all issues
```
Missing a trailing period.
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 18)
Show all issues
```
Name should be GetFileExistsInHistoryTests.
```

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 18)

Show all issues

For here and any others, can we use called_with() and check the arguments?

reviewboard/diffviewer/tests/test_forms.py (Diff revision 18)
Show all issues
```
Extra blank line here.
```
reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 18)
Show all issues
```
Missing a period.
```
reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 18)
Show all issues
```
Extra blank line.
```

reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 18)

Show all issues

You can align these starting on the initial call. There should be plenty of room.

reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 18)
Show all issues
```
Can you include the repository ID? That'll help with RBCommons.
```
reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 18)
Show all issues
```
This line's too long. You can break at a .
```
reviewboard/webapi/tests/test_validate_diffcommit.py (Diff revision 18)
Show all issues
```
Can you explain why here?
```

Commit:

-	858f8d77098b2ad76f1e06d3d518111923b05905
+	5973997c843000d9dd417f41feb5e88f12bae3eb

Diff:

Revision 19 (+1898 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/forms.py (Diff revision 19)
Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 19)
Show all issues
```
F821 undefined name 'revision'
```

Change Summary:

Fix issues introduced with git rerere

Commit:

-	5973997c843000d9dd417f41feb5e88f12bae3eb
+	c23ee9cf286fbca7de21d4b0a45fa3d68ecb0a0c

Diff:

Revision 20 (+1886 -33)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models/filediff.py
	11 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_forms.py (Diff revision 20)
Show all issues
```
E501 line too long (80 > 79 characters)
```

Commit:

-	c23ee9cf286fbca7de21d4b0a45fa3d68ecb0a0c
+	6e738dc6bdd3c1b89c5fb09f1ccae8112908fbc2

Diff:

Revision 21 (+1886 -33)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models/filediff.py
	11 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Small issues.
```

reviewboard/webapi/resources/validate_diffcommit.py (Diff revision 21)

Show all issues

This sounds confusing. Can you say:

The "%s" repository does not support review requests with commit history.

reviewboard/webapi/tests/test_validate_diffcommit.py (Diff revision 21)
Show all issues
```
Blank line between these.
```

Change Summary:

Addressed Christian's feedback.

Commit:

-	6e738dc6bdd3c1b89c5fb09f1ccae8112908fbc2
+	4dd62e1f5b24d4afae452697b6192c443f62ad02

Diff:

Revision 22 (+1882 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Addressed Christian's feedback.

Commit:

-	4dd62e1f5b24d4afae452697b6192c443f62ad02
+	5646814021cf8c50db323eb606610908cdc34d04

Diff:

Revision 23 (+1886 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Unit test fixes

Commit:

-	5646814021cf8c50db323eb606610908cdc34d04
+	1e5bd7dd0edc08635c571706bf50defad3b54fae

Diff:

Revision 24 (+1888 -29)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/validate-diff-commit.rst
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (36b5970)

You have a pending review.

Require validation for DiffCommits when posting

Change Summary:

Depends On:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Depends On:

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary: