Summary

Support for multi commit review requests in patch

Review Request #7175 — Created April 6, 2015 and submitted April 9, 2015, 10:05 p.m.

Information

Owner

brennie

Repository

RBTools

Branch

dvcs

Bugs

Depends On

~~7181~~

Commit

afbd97f...

Reviewers

Groups

rbtools

People

Description

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.

Testing Done

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 diff was correct.
Ran rbt patch -C. The commits were all created client side. The
output of git diff was correct.
Ran rbt patch -S. The commits were not committed. The output of
git diff was correct. Verified via the --debug flag that we only
request the condensed diff from the server.
Ran rbt patch -SC against 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 of git diff
was correct.
Ran rbt patch -C. One commit was created and it was of the old
format (as specified above). The output of git diff was correct.
Ran rbt patch -SC. Same behaviour of rbt patch -C.

Issues

Description	From	Last Updated
This should use the new template stuff, no?	david	April 7, 2015, 2:50 p.m.
This should probably not say "yields" (since that implies a generator). How about something like a list of history entry …	david	April 7, 2015, 3:34 p.m.
This is assuming a single page of results. You should look at how we iterate over repositories in order to …	david	April 7, 2015, 6:14 p.m.
I don't think we need to use &= here (or even define success outside of the for loop) because when …	david	April 7, 2015, 3:34 p.m.
I don't think you need to use the quotes in here, since the shell isn't parsing arguments.	david	April 8, 2015, 5:53 p.m.
Shouldn't -u and the author be separate arguments?	david	April 8, 2015, 5:53 p.m.
There's no reason for this format operation now. Just pass in author.	david	April 8, 2015, 6:11 p.m.
How about wrapping this as: with_history = (should_commit and self.should_use_history(tool, server_url))	david	April 8, 2015, 5:54 p.m.
Indentation here is crazy. The arguments should just be 4 spaces from the previous indent level, not 20.	david	April 8, 2015, 6:11 p.m.
How about doing: if not success: raise CommandError(...) if should_commit: ... That way you can un-indent everything one level.	david	April 8, 2015, 5:54 p.m.

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

Change Summary:

Minor fixups

Commit:

487ba1fb6af899889e3a604a411e1088447ecf42

d2cf74032d5511a97cc20786bcdd02c50ecd4d9e

Diff:

Revision 2 (+146 -45)

Show changes

	rbtools/api/resource.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/land.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



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:

d2cf74032d5511a97cc20786bcdd02c50ecd4d9e

3c483ea01558bcea08857c1f939ce58ff9d7ecb6

Diff:

Revision 3 (+148 -45)

Show changes

	rbtools/api/resource.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/land.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: 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/api/resource.py (Diff revision 3)

The issue has been dropped. Show all issues

This should use the new template stuff, no?

brennie April 7, 2015, 2:50 p.m.

Once that lands, I'll put up a new patch to reconcile this.

rbtools/commands/patch.py (Diff revision 3)

The issue has been resolved. Show all issues

This should probably not say "yields" (since that implies a generator). How about something like a list of history entry (diff, metadata) tuples?

brennie April 7, 2015, 2:50 p.m.

Oops! This originally was a generator so that is how it crept through.

rbtools/commands/patch.py (Diff revision 3)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

I don't think we need to use &= here (or even define success outside of the for loop) because when the result of this is False, you raise a CommandError.

Depends On:: ~~7181 - Add pagination helpers to API ListResources~~

Change Summary:

Address David's issues. Use the new pagination stuff.

Commit:

3c483ea01558bcea08857c1f939ce58ff9d7ecb6

733b78b29b91ede838fdc7a2c3dc5c0685d21807

Diff:

Revision 4 (+146 -45)

Show changes

	rbtools/api/resource.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/land.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: 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)

The issue has been resolved. Show all issues

I don't think you need to use the quotes in here, since the shell isn't parsing arguments.

rbtools/clients/mercurial.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Shouldn't -u and the author be separate arguments?
```

rbtools/commands/patch.py (Diff revision 4)

The issue has been resolved. Show all issues

How about wrapping this as:

with_history = (should_commit and
                self.should_use_history(tool, server_url))

rbtools/commands/patch.py (Diff revision 4)

The issue has been resolved. Show all issues

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 APIError that wasn't being caught.

Reorder code so that fetching review_request happens outside the loop.

Commit:

733b78b29b91ede838fdc7a2c3dc5c0685d21807

7281293a617ea0f9ab84753d3e791db6974bf47b

Diff:

Revision 5 (+153 -42)

Show changes

	rbtools/api/resource.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/land.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: 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)
The issue has been resolved. Show all issues
```
There's no reason for this format operation now. Just pass in author.
```

rbtools/commands/patch.py (Diff revisions 4 - 5)

The issue has been resolved. Show all issues

Indentation here is crazy. The arguments should just be 4 spaces from the previous indent level, not 20.

Commit:

7281293a617ea0f9ab84753d3e791db6974bf47b

afbd97f0f605779a1a97567b745f532df71b50ac

Diff:

Revision 6 (+150 -40)

Show changes

	rbtools/api/resource.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/commands/land.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



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/clients/mercurial.py
    rbtools/api/resource.py
    rbtools/clients/git.py
    rbtools/commands/patch.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to dvcs (80debfe)