Support for multi commit review requests in patch
Review Request #7175 — Created April 6, 2015 and submitted
The patch command now supports applying review requests with commit
history. If requested via command line arguments, each commit in the
review request will be applied in sequence on the current branch,
optionally committing each patch. The default behaviour is to replicate
what ever format of review request was uploaded, but this can be
overridden via flags.
Ran unit tests.
Manually tested the following against review requests with multiple
commits:
- Ran
rbt patch. The commits were not committed. The output of
git diffwas correct. - Ran
rbt patch -C. The commits were all created client side. The
output ofgit diffwas correct. - Ran
rbt patch -S. The commits were not committed. The output of
git diffwas correct. Verified via the--debugflag that we only
request the condensed diff from the server. - Ran
rbt patch -SCagainst a review request with multiple commits.
The resulting commit was of the old format (description, testing done,
bugs fixed, etc.).
Manually tested the following against review request without multiple
commits:
- Ran
rbt patch. No commits were created. The output ofgit diff
was correct. - Ran
rbt patch -C. One commit was created and it was of the old
format (as specified above). The output ofgit diffwas correct. - Ran
rbt patch -SC. Same behaviour ofrbt patch -C.
| Description | From | Last Updated |
|---|---|---|
|
This should use the new template stuff, no? |
 david
|
|
|
This should probably not say "yields" (since that implies a generator). How about something like a list of history entry … |
david
|
|
|
This is assuming a single page of results. You should look at how we iterate over repositories in order to … |
david
|
|
|
I don't think we need to use &= here (or even define success outside of the for loop) because when … |
david
|
|
|
I don't think you need to use the quotes in here, since the shell isn't parsing arguments. |
david
|
|
|
Shouldn't -u and the author be separate arguments? |
david
|
|
|
There's no reason for this format operation now. Just pass in author. |
david
|
|
|
How about wrapping this as: with_history = (should_commit and self.should_use_history(tool, server_url)) |
david
|
|
|
Indentation here is crazy. The arguments should just be 4 spaces from the previous indent level, not 20. |
david
|
|
|
How about doing: if not success: raise CommandError(...) if should_commit: ... That way you can un-indent everything one level. |
david
|
Change Summary:
Minor fixups
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+146 -45) |
-
Tool: PEP8 Style Checker Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py Tool: Pyflakes Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py
Change Summary:
Save a round trip in some cases.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+148 -45) |
-
Tool: Pyflakes Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py Tool: PEP8 Style Checker Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py
-
-
-
rbtools/commands/patch.py (Diff revision 3) This should probably not say "yields" (since that implies a generator). How about something like
a list of history entry (diff, metadata) tuples? -
rbtools/commands/patch.py (Diff revision 3) This is assuming a single page of results. You should look at how we iterate over repositories in order to fetch all pages.
-
rbtools/commands/patch.py (Diff revision 3) I don't think we need to use
&=here (or even definesuccessoutside of the for loop) because when the result of this isFalse, you raise a CommandError.
Change Summary:
Address David's issues. Use the new pagination stuff.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+146 -45) |
-
Tool: Pyflakes Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py Tool: PEP8 Style Checker Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py
-
-
rbtools/clients/git.py (Diff revision 4) I don't think you need to use the quotes in here, since the shell isn't parsing arguments.
-
-
rbtools/commands/patch.py (Diff revision 4) How about wrapping this as:
with_history = (should_commit and self.should_use_history(tool, server_url)) -
rbtools/commands/patch.py (Diff revision 4) How about doing:
if not success: raise CommandError(...) if should_commit: ...That way you can un-indent everything one level.
Change Summary:
Address David's issues.
Fix a typo.
Catch one more
APIErrorthat wasn't being caught.Reorder code so that fetching
review_requesthappens outside the loop.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+153 -42) |
-
Tool: Pyflakes Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py Tool: PEP8 Style Checker Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py
-
-
rbtools/clients/mercurial.py (Diff revisions 4 - 5) There's no reason for this format operation now. Just pass in
author. -
rbtools/commands/patch.py (Diff revisions 4 - 5) Indentation here is crazy. The arguments should just be 4 spaces from the previous indent level, not 20.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+150 -40) |
-
Tool: PEP8 Style Checker Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py Tool: Pyflakes Processed Files: rbtools/commands/land.py rbtools/clients/mercurial.py rbtools/api/resource.py rbtools/clients/git.py rbtools/commands/patch.py
Tool: Pyflakes Processed Files: rbtools/commands/patch.py rbtools/api/resource.py rbtools/clients/git.py Tool: PEP8 Style Checker Processed Files: rbtools/commands/patch.py rbtools/api/resource.py rbtools/clients/git.py