Summary:

`rbt land` command for landing changes in upstream

Review Request #6509 — Created Oct. 26, 2014 and submitted Dec. 16, 2014, 5:43 a.m.

Information
Owner:	asalahli
Repository:	RBTools
Branch:	master
Bugs:
Depends On:
Commit:	2f08592...
Reviewers
Groups:	rbtools, students
People:

Description

rbt land command for landing changes in upstream
See https://reviewboard.hackpad.com/rbt-land-YrgGuhTuseU
for detailed description of the command.

What works:

Merging/rebasing a branch with/on destination branch.
Adding a commit message based on the review request.
Pushing to upstream
A wrapper around rbt patch in order to allow landing
others' changes.

Testing Done

Successfully merged and rebased a feature branch to/on master branch.
Successfully patched a review request from another user and landed it
on upstream.
Tested with:
- empty repo
- unclean working directory
- unapproved review request
- same target and destination branches
- incorrect target/destination branch
- incorrect upstream repository

and got appropriate error messages, as expected.

Issues

Description	From	Last Updated
'APIError' imported but unused	reviewbot	Nov. 9, 2014, 12:40 a.m.
'execute' imported but unused	reviewbot	Nov. 9, 2014, 12:40 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Nov. 9, 2014, 12:40 a.m.
Col: 1 W391 blank line at end of file	reviewbot	Nov. 9, 2014, 12:40 a.m.
'json' imported but unused	reviewbot	Nov. 9, 2014, 12:40 a.m.
local variable 'remote' is assigned to but never used	reviewbot	Nov. 9, 2014, 12:45 a.m.
'APIError' imported but unused	reviewbot	Nov. 9, 2014, 12:45 a.m.
'InvalidRevisionSpecError' imported but unused	reviewbot	Nov. 9, 2014, 12:45 a.m.
'execute' imported but unused	reviewbot	Nov. 9, 2014, 12:45 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Nov. 9, 2014, 12:45 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Nov. 9, 2014, 12:45 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Nov. 9, 2014, 12:45 a.m.
Col: 1 W391 blank line at end of file	reviewbot	Nov. 9, 2014, 12:45 a.m.
'json' imported but unused	reviewbot	Nov. 9, 2014, 12:45 a.m.
'json' imported but unused	reviewbot	Nov. 9, 2014, 12:47 a.m.
'json' imported but unused	reviewbot	Nov. 16, 2014, 11:16 a.m.
Col: 1 W391 blank line at end of file	reviewbot	Nov. 16, 2014, 1:48 p.m.
'json' imported but unused	reviewbot	Nov. 16, 2014, 1:48 p.m.
Let's use "squash" instead of "flatten". Also, this function and push_upstream need to have NotImplemented stubs in SCMClient.	chipx86	Nov. 23, 2014, 8:59 a.m.
Blank line between these.	chipx86	Nov. 23, 2014, 8:46 a.m.
Say you do this on 'master', and your commit is already there. Doing a pull is going to pull down …	chipx86	Nov. 23, 2014, 8:45 a.m.
Missing a trailing period.	chipx86	Nov. 23, 2014, 8:45 a.m.
This doesn't really describe what the command does. We should have a lot more detail about what this is and …	chipx86	Nov. 23, 2014, 11:52 a.m.
The """ should be on the next lin.	chipx86	Nov. 23, 2014, 10:17 a.m.
Merging shouldn't squash. Or are you saying it'd be the inverse?	chipx86	Nov. 23, 2014, 10:17 a.m.
Can this be done now?	chipx86	Nov. 23, 2014, 3:05 a.m.
Just from the common ones, right? How about doing that as part of this change?	chipx86	Nov. 23, 2014, 8:59 a.m.
'json' imported but unused	reviewbot	Nov. 16, 2014, 1:48 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Nov. 23, 2014, 9:21 a.m.
undefined name 'branch'	reviewbot	Nov. 23, 2014, 9:21 a.m.
undefined name 'branch'	reviewbot	Nov. 23, 2014, 9:21 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 23, 2014, 9:21 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 23, 2014, 9:21 a.m.
'json' imported but unused	reviewbot	Nov. 23, 2014, 9:21 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Nov. 23, 2014, 9:24 a.m.
'json' imported but unused	reviewbot	Nov. 23, 2014, 9:24 a.m.
Col: 69 W291 trailing whitespace	reviewbot	Nov. 23, 2014, 9:26 a.m.
'json' imported but unused	reviewbot	Nov. 23, 2014, 9:26 a.m.
'json' imported but unused	reviewbot	Nov. 23, 2014, 9:27 a.m.
Use a double quote and put the branch name in single quotes, e.g. raise MergeError("Could not checkout branch '%s' ...) …	brennie	Dec. 3, 2014, 10:45 a.m.
Quote branchname in single quotes.	brennie	Dec. 3, 2014, 10:46 a.m.
"to the remote repository"	chipx86	Dec. 3, 2014, 10:43 a.m.
.reviewboardrc shoudn't be required. It hsould be possible to specify on the command line.	chipx86	Dec. 3, 2014, 10:43 a.m.
I prefer full sentences that sound more natural, like "A review request ID is required."	chipx86	Dec. 3, 2014, 10:43 a.m.
This change I think predates the discussion on this, but "BRANCH" can't be used for any kind of real determinations …	chipx86	Dec. 3, 2014, 11:23 a.m.
This may not be very clear when the user hits it and doesn't realize that they chose the same branch. …	chipx86	Dec. 3, 2014, 10:44 a.m.
We can't depend on this attribute existing on the review request. It's new in 2.0. You can check with hasattr(). …	chipx86	Dec. 3, 2014, 10:44 a.m.
"The specified review request ..." This should also include the review_request.approval_failure text.	chipx86	Dec. 3, 2014, 10:44 a.m.
This should now be MergeError as e. Same in all other cases.	chipx86	Dec. 3, 2014, 10:45 a.m.
How about "... has landed on <branch>"	chipx86	Dec. 3, 2014, 10:45 a.m.
Since we're moving to Py2.5+, you'll want to use the new print function. At the top of your file: from …	brennie	Dec. 3, 2014, 10:46 a.m.
This was changed in https://reviews.reviewboard.org/r/6578/ to use __future__.print_function. You should pull master and rebase your changes off of it. Your …	brennie	Dec. 3, 2014, 10:46 a.m.
I had to read the code and think through it to realize what this is doing and why it's there. …	chipx86	Dec. 4, 2014, 4:56 a.m.
There's a way to simplify this and make the function in general a lot more useful. Instead of hard-coding all …	chipx86	Dec. 4, 2014, 4:56 a.m.
'json' imported but unused	reviewbot	Nov. 23, 2014, 10:16 a.m.
Col: 36 E111 indentation is not a multiple of four	reviewbot	Dec. 3, 2014, 10:42 a.m.
Col: 36 E113 unexpected indentation	reviewbot	Dec. 3, 2014, 10:42 a.m.
Col: 17 E113 unexpected indentation	reviewbot	Dec. 3, 2014, 10:42 a.m.
Col: 17 E112 expected an indented block	reviewbot	Dec. 3, 2014, 10:42 a.m.
Col: 25 W601 .has_key() is deprecated, use 'in'	reviewbot	Dec. 3, 2014, 11:29 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Dec. 4, 2014, 5 a.m.
"are" considered optional.	chipx86	Dec. 16, 2014, 4:02 a.m.
"an upstream repository."	chipx86	Dec. 16, 2014, 4:02 a.m.
"the patching step."	chipx86	Dec. 16, 2014, 4:02 a.m.
Instead of hard-coding here, it's best to identify what's missing in SCMClients that this function needs to support. We can …	chipx86	Dec. 16, 2014, 4:28 a.m.
This needs to raise a CommandError for the output to be properly shown. Otherwise it'll appear as a crash. Also, …	chipx86	Dec. 16, 2014, 4:48 a.m.
I just pushed a change that adds this. See what commands/stamp.py and commands/post.py do now.	chipx86	Dec. 16, 2014, 4:54 a.m.
"Review Board". Can you say: "... an old version (pre-2.0)" that ..."	chipx86	Dec. 16, 2014, 4:05 a.m.
"support the `approved` field."	chipx86	Dec. 16, 2014, 4:50 a.m.
Blank line between these.	chipx86	Dec. 16, 2014, 4:05 a.m.
If we have approved and approval_failure, we have issue_open_count. They were introduced in the same release.	chipx86	Dec. 16, 2014, 4:05 a.m.
The ending paren should be on the self.options.squash line.	chipx86	Dec. 16, 2014, 4:05 a.m.
We don't put parens around individual conditions.	chipx86	Dec. 16, 2014, 4:08 a.m.
Same here.	chipx86	Dec. 16, 2014, 4:08 a.m.
This looks left over from something.	chipx86	Dec. 16, 2014, 4:29 a.m.
You can probably make this one line by doing: approval_failure = \ 'The review request ...'	chipx86	Dec. 16, 2014, 4:50 a.m.

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/clients/git.py

rbtools/commands/land.py (Diff revision 1)
Show all issues
```
 'APIError' imported but unused
```
rbtools/commands/land.py (Diff revision 1)
Show all issues
```
 'execute' imported but unused
```

rbtools/commands/land.py (Diff revision 1)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

rbtools/commands/land.py (Diff revision 1)
Show all issues
```
Col: 1
 W391 blank line at end of file
```
setup.py (Diff revision 1)
Show all issues
```
 'json' imported but unused
```

Description:

		`rbt land` command will be used for
~		1.Applying a specific review request to local source tree and pushing it to the main repository.
	~	1.Applying a specific review request to local source tree
	+	and pushing it to the main repository.
		2.Pushing users local branch to the main repository.

		It is not working yet.

		Once it works, the command usage will be:

~		`rbt land <request-id> [--patch] [--remote-branch <branch-name>]
	~	`rbt land <request-id>`
	+
	+	Options:
	+
	+	--patch Apply the review request to local
	+	repository (i.e. RR belongs to someone else)
	+	--history (preserve\|squash)
	+	Either keep commit history, or squash it into
	+	a single commit
	+	--remote-branch BRANCH_NAME
	+	Push changes to BRANCH_NAME branch (overrides
	+	the entry in .reviewboardrc)
	+
	+
	+	Eventually, the command should be able to guess review request id
	+	from past commits. But that code is embedded in `rbt post` and needs
	+	to be refactored before it can be used. So, for now `--path` option
	+	can be used.

Summary:

-	[WIP] `rbt land` command for landing changes
+	[WIP] `rbt land` command for landing changes in upstream

Description:

~		`rbt land` command will be used for
~		1.Applying a specific review request to local source tree
~		and pushing it to the main repository.
	~	`rbt land` command for landing changes in upstream
	~	See https://reviewboard.hackpad.com/rbt-land-YrgGuhTuseU
	~	for detailed description of the command.
-		2.Pushing users local branch to the main repository.

~		It is not working yet.
	~	What works:
	+	+ Merging/rebasing a branch with/on destination branch.
	+	+ Adding a commit message based on the review request.
	+	+ Pushing to upstream

~		Once it works, the command usage will be:
	~

~		`rbt land <request-id>`
~
~		Options:
~
~		--patch Apply the review request to local
	~	What needs to be done:
	~	+ A wrapper around `rbt patch` in order to allow landing
	~	others' changes.
	~	+ Lots of error handling. Currently, no error handling is
	~	being performed if commit/merge/push is unsuccessful.
-		repository (i.e. RR belongs to someone else)
-		--history (preserve\|squash)
-		Either keep commit history, or squash it into
-		a single commit
-		--remote-branch BRANCH_NAME
-		Push changes to BRANCH_NAME branch (overrides
-		the entry in .reviewboardrc)
-
-
-		Eventually, the command should be able to guess review request id
-		from past commits. But that code is embedded in `rbt post` and needs
-		to be refactored before it can be used. So, for now `--path` option
-		can be used.

Testing Done:

	+	Successfully merged and rebased a feature branch to/on master branch.
	+	Tested with
	+	empty repo
	+	unclean working directory
	+	unapproved review request
	+
	+	and got appropriate error messages, as expected.

Commit:

-	2580d773ef57860360adbe729694a213d9555816
+	41c142e6018b935ce14384ecaf5ccdb7a89e7140

Diff:

Revision 2 (+142)

Show changes

	setup.py
	rbtools/clients/git.py
	rbtools/commands/land.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 2)

Show all issues

 local variable 'remote' is assigned to but never used

rbtools/commands/land.py (Diff revision 2)
Show all issues
```
 'APIError' imported but unused
```

rbtools/commands/land.py (Diff revision 2)

Show all issues

 'InvalidRevisionSpecError' imported but unused

rbtools/commands/land.py (Diff revision 2)
Show all issues
```
 'execute' imported but unused
```

rbtools/commands/land.py (Diff revision 2)

Show all issues

Col: 80
 E501 line too long (85 > 79 characters)

rbtools/commands/land.py (Diff revision 2)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

rbtools/commands/land.py (Diff revision 2)

Show all issues

Col: 80
 E501 line too long (83 > 79 characters)

rbtools/commands/land.py (Diff revision 2)
Show all issues
```
Col: 1
 W391 blank line at end of file
```
setup.py (Diff revision 2)
Show all issues
```
 'json' imported but unused
```

Change Summary:

Fixed style issues

Commit:

-	41c142e6018b935ce14384ecaf5ccdb7a89e7140
+	1299b17f4cce826d1859168c7ce836e8efa474fb

Diff:

Revision 3 (+139)

Show changes

	setup.py
	rbtools/clients/git.py
	rbtools/commands/land.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/clients/git.py

setup.py (Diff revision 3)
Show all issues
```
 'json' imported but unused
```

Groups:

+	students

Description:

		`rbt land` command for landing changes in upstream
		See https://reviewboard.hackpad.com/rbt-land-YrgGuhTuseU
		for detailed description of the command.

		What works:
		+ Merging/rebasing a branch with/on destination branch.
		+ Adding a commit message based on the review request.
~		+ Pushing to upstream
~
~
	~	+ Pushing to upstream
	~	+ A wrapper around `rbt patch` in order to allow landing
	~	others' changes.

		What needs to be done:
-		+ A wrapper around `rbt patch` in order to allow landing
-		others' changes.
		+ Lots of error handling. Currently, no error handling is
		being performed if commit/merge/push is unsuccessful.

Commit:

-	1299b17f4cce826d1859168c7ce836e8efa474fb
+	b7992fbe36fd18f9d21aa6de8423d08b194c1134

Diff:

Revision 4 (+179)

Show changes

	setup.py
	rbtools/clients/git.py
	rbtools/commands/land.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/clients/git.py

setup.py (Diff revision 4)
Show all issues
```
 'json' imported but unused
```

Description:

		`rbt land` command for landing changes in upstream
		See https://reviewboard.hackpad.com/rbt-land-YrgGuhTuseU
		for detailed description of the command.

~		What works:
	~	What works:
-		+ Merging/rebasing a branch with/on destination branch.
-		+ Adding a commit message based on the review request.
-		+ Pushing to upstream
-		+ A wrapper around `rbt patch` in order to allow landing
-		others' changes.

~		What needs to be done:
~		+ Lots of error handling. Currently, no error handling is
~		being performed if commit/merge/push is unsuccessful.
	~	Merging/rebasing a branch with/on destination branch.
	~	Adding a commit message based on the review request.
	~	Pushing to upstream
	+	A wrapper around `rbt patch` in order to allow landing others' changes.
	+
	+
	+
	+	NOTE: I've added a new parameter to `rbtools.utils.process.execute()`
	+	that makes the function throw `ExecuteError` exception when the
	+	command fails. That exception is then cathed and more user-friendly,
	+	message is printed. However I'm not sure if this is the desired
	+	behaviour, because I could not figure out how the errors are handled
	+	right now.
	+	Also, since `rbtools.clients.git.GitClient.create_commit()` is being
	+	used from other places as well as the new code I've added, I could
	+	not use the new exception throwing option on this case, so if commit
	+	fails, the command terminates the old way.

Testing Done:

~		Successfully merged and rebased a feature branch to/on master branch.
~		Tested with
~		empty repo
	~	Successfully merged and rebased a feature branch to/on master branch.
	~	Successfully patched a review request from another user and landed it on upstream.
	~	Tested with: empty repo unclean working directory unapproved review request same target and destination branches incorrect target/destination branch incorrect upstream repository
-		unclean working directory
-		unapproved review request

		and got appropriate error messages, as expected.

Commit:

-	b7992fbe36fd18f9d21aa6de8423d08b194c1134
+	7c124acfe14cc450dcbabd75e06b3af1df4b1f53

Diff:

Revision 5 (+199 -1)

Show changes

	setup.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/utils/process.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/utils/process.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/utils/process.py
    rbtools/clients/git.py

rbtools/commands/land.py (Diff revision 5)
Show all issues
```
Col: 1
 W391 blank line at end of file
```
setup.py (Diff revision 5)
Show all issues
```
 'json' imported but unused
```

Commit:

-	7c124acfe14cc450dcbabd75e06b3af1df4b1f53
+	fc062a4201fe362d84752296900f8a3757b1bb75

Diff:

Revision 6 (+198 -1)

Show changes

	setup.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/utils/process.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/utils/process.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    setup.py
    rbtools/utils/process.py
    rbtools/clients/git.py

setup.py (Diff revision 6)
Show all issues
```
 'json' imported but unused
```

rbtools/clients/git.py (Diff revision 6)

Show all issues

Let's use "squash" instead of "flatten".
Also, this function and push_upstream need to have NotImplemented stubs in SCMClient.

rbtools/clients/git.py (Diff revision 6)
Show all issues
```
Blank line between these.
```

rbtools/clients/git.py (Diff revision 6)

Show all issues

Say you do this on 'master', and your commit is already there. Doing a pull is going to pull down new content and merge it in, creating a bit of an annoying merge graph, before pushing.

What you probably want here is 'git pull --rebase'.

Also, pull and push will, depending on settings/version of git, pull/push all branches. I'd be explicit with taking a branch name and passing it in this function.

rbtools/clients/git.py (Diff revision 6)
Show all issues
```
Missing a trailing period.
```

rbtools/commands/land.py (Diff revision 6)

Show all issues

This doesn't really describe what the command does. We should have a lot more detail about what this is and why/how it's used.

rbtools/commands/land.py (Diff revision 6)

Show all issues

The """ should be on the next lin.

asalahli Nov. 21, 2014, 6:56 a.m.

In Patch, """ is placed as in this one. Should I change it too, or should I keep this?

rbtools/commands/land.py (Diff revision 6)

Show all issues

Merging shouldn't squash. Or are you saying it'd be the inverse?

asalahli Nov. 21, 2014, 6:56 a.m.

Yes, since this is more about squashing the history vs. keeping it, rather than merging vs. rebasing. So, --squash would squash everything into one commit, while default would preserve the history.

rbtools/commands/land.py (Diff revision 6)
Show all issues
```
Can this be done now?
```

rbtools/commands/land.py (Diff revision 6)

Show all issues

Just from the common ones, right?

How about doing that as part of this change?

Summary:

-	[WIP] `rbt land` command for landing changes in upstream
+	`rbt land` command for landing changes in upstream

Description:

		`rbt land` command for landing changes in upstream
		See https://reviewboard.hackpad.com/rbt-land-YrgGuhTuseU
		for detailed description of the command.

		What works:

		Merging/rebasing a branch with/on destination branch.
		Adding a commit message based on the review request.
		Pushing to upstream
		A wrapper around `rbt patch` in order to allow landing others' changes.
-
-
-
-		NOTE: I've added a new parameter to `rbtools.utils.process.execute()`
-		that makes the function throw `ExecuteError` exception when the
-		command fails. That exception is then cathed and more user-friendly,
-		message is printed. However I'm not sure if this is the desired
-		behaviour, because I could not figure out how the errors are handled
-		right now.
-		Also, since `rbtools.clients.git.GitClient.create_commit()` is being
-		used from other places as well as the new code I've added, I could
-		not use the new exception throwing option on this case, so if commit
-		fails, the command terminates the old way.

Commit:

-	fc062a4201fe362d84752296900f8a3757b1bb75
+	97bc388de9953325d3ea6ab2997651d3d39d04e0

Diff:

Revision 7 (+252 -31)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

rbtools/clients/git.py (Diff revision 7)

Show all issues

Col: 80
 E501 line too long (84 > 79 characters)

rbtools/clients/git.py (Diff revision 7)
Show all issues
```
 undefined name 'branch'
```
rbtools/clients/git.py (Diff revision 7)
Show all issues
```
 undefined name 'branch'
```
rbtools/utils/commands.py (Diff revision 7)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
rbtools/utils/commands.py (Diff revision 7)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
setup.py (Diff revision 7)
Show all issues
```
 'json' imported but unused
```

Change Summary:

Fixed style issues

Commit:

-	97bc388de9953325d3ea6ab2997651d3d39d04e0
+	ba9559378aab4e43d7b524dd9d041bb27653577c

Diff:

Revision 8 (+254 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/utils/commands.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

rbtools/clients/git.py (Diff revision 8)

Show all issues

Col: 80
 E501 line too long (83 > 79 characters)

setup.py (Diff revision 8)
Show all issues
```
 'json' imported but unused
```

Commit:

-	ba9559378aab4e43d7b524dd9d041bb27653577c
+	2279d047252ba841c937ee8ec8bf802b63754d59

Diff:

Revision 9 (+255 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

rbtools/clients/git.py (Diff revision 9)
Show all issues
```
Col: 69
 W291 trailing whitespace
```
setup.py (Diff revision 9)
Show all issues
```
 'json' imported but unused
```

Commit:

-	2279d047252ba841c937ee8ec8bf802b63754d59
+	41183d56fc229db1c212bde520be6bf33b461284

Diff:

Revision 10 (+255 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

setup.py (Diff revision 10)
Show all issues
```
 'json' imported but unused
```

Change Summary:

Better docstring for commands.land.Land

Commit:

-	41183d56fc229db1c212bde520be6bf33b461284
+	139a4b4c0f18775fac796272bb6e493340335e7f

Diff:

Revision 11 (+255 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

setup.py (Diff revision 11)
Show all issues
```
 'json' imported but unused
```

rbtools/commands/land.py (Diff revision 11)
Show all issues
```
"to the remote repository"
```

rbtools/commands/land.py (Diff revision 11)

Show all issues

.reviewboardrc shoudn't be required. It hsould be possible to specify on the command line.

rbtools/commands/land.py (Diff revision 11)

Show all issues

I prefer full sentences that sound more natural, like "A review request ID is required."

rbtools/commands/land.py (Diff revision 11)

Show all issues

This change I think predates the discussion on this, but "BRANCH" can't be used for any kind of real determinations like this. That option purely handles the visual display of a branch, and can contain any text.

rbtools/commands/land.py (Diff revision 11)

Show all issues

This may not be very clear when the user hits it and doesn't realize that they chose the same branch. How about:

"The local branch cannot be merged onto itself. Try a different local branch or destination branch."

rbtools/commands/land.py (Diff revision 11)

Show all issues

We can't depend on this attribute existing on the review request. It's new in 2.0.

You can check with hasattr().

If it doesn't exist, we can check review_request.ship_it_count.

rbtools/commands/land.py (Diff revision 11)

Show all issues

"The specified review request ..."

This should also include the review_request.approval_failure text.

asalahli Dec. 3, 2014, 10:45 a.m.

I changed this line to only print approval_failure text, since it clearly explains the problem.

rbtools/commands/land.py (Diff revision 11)

Show all issues

This should now be MergeError as e.

Same in all other cases.

rbtools/commands/land.py (Diff revision 11)
Show all issues
```
How about "... has landed on <branch>"
```

rbtools/utils/commands.py (Diff revision 11)

Show all issues

I had to read the code and think through it to realize what this is doing and why it's there.

How about calling this build_rbtools_cmd_argv, and having the docs be clear that this is used to take common options passed on the command line or in .reviewboardrc and convert them back into a set of arguments for calling another RBTools command?

rbtools/utils/commands.py (Diff revision 11)

Show all issues

There's a way to simplify this and make the function in general a lot more useful.
Instead of hard-coding all these here, this function can take a dictionary (defaulting to a predefiend dictionary constant in this module) and build the command lines from that. It can do this with:
```python

def build_rbtools_cmd_argv(options, options_map=DEFAULT_OPTIONS_MAP):

    argv = []
for option_key, arg_name in six.iteritems(options_map):
    option_value = getattr(options, option_key, None)

    if option_value is True:
        argv.append(arg_name)
    elif option_value not in (False, None):
        argv.extend([arg_name, option_value])

return argv

rbtools/clients/git.py (Diff revision 11)

Show all issues

Use a double quote and put the branch name in single quotes, e.g.

raise MergeError("Could not checkout branch '%s' ...)

It also might read better if the error is on the same line, seperated by a colon instead of newlines.

Same below.

asalahli Dec. 3, 2014, 10:46 a.m.

Added single quotes as you suggested. But newlines are there to seperate error message from SCM command output.

rbtools/clients/git.py (Diff revision 11)
Show all issues
```
Quote branchname in single quotes.
```

rbtools/commands/land.py (Diff revision 11)

Show all issues

Since we're moving to Py2.5+, you'll want to use the new print function. At the top of your file:
from __future__ import print_function

and then here you call print as a function with parentheses: print('Review request %s ...' % request_id)
Likewise elsewhere you use print.

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

Show all issues

This was changed in https://reviews.reviewboard.org/r/6578/ to use __future__.print_function.

You should pull master and rebase your changes off of it. Your patch may not cleanly apply, otherwise.

Commit:

-	139a4b4c0f18775fac796272bb6e493340335e7f
+	d9c015d835e79451305899f59f4d6429a3dea1b2

Diff:

Revision 12 (+275 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/utils/commands.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

rbtools/commands/land.py (Diff revision 12)

Show all issues

Col: 36
 E111 indentation is not a multiple of four

rbtools/commands/land.py (Diff revision 12)
Show all issues
```
Col: 36
 E113 unexpected indentation
```
rbtools/commands/land.py (Diff revision 12)
Show all issues
```
Col: 17
 E113 unexpected indentation
```
rbtools/commands/land.py (Diff revision 12)
Show all issues
```
Col: 17
 E112 expected an indented block
```

Commit:

-	d9c015d835e79451305899f59f4d6429a3dea1b2
+	6c676b89c5b1cd8284bb97a1730bea4d585e7bc0

Diff:

Revision 13 (+275 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

Change Summary:

Changed branch related arguments and options to reflect the discussion in the last group meeting.

Commit:

-	6c676b89c5b1cd8284bb97a1730bea4d585e7bc0
+	d5e45e68edab59a4c7912523df8fd0c6ebb45498

Diff:

Revision 14 (+286 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/__init__.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	1 more

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

rbtools/commands/land.py (Diff revision 14)

Show all issues

Col: 25
 W601 .has_key() is deprecated, use 'in'

Commit:

-	d5e45e68edab59a4c7912523df8fd0c6ebb45498
+	91869c08f74449cdcb2b3cbfdc3dba91f02513cb

Diff:

Revision 15 (+286 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/__init__.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	1 more

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

Commit:

-	91869c08f74449cdcb2b3cbfdc3dba91f02513cb
+	9eab566c9daacd4e29f0306f7cf6bd4c3ffeda92

Diff:

Revision 16 (+288 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/__init__.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	1 more

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

rbtools/commands/land.py (Diff revision 16)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Commit:

-	9eab566c9daacd4e29f0306f7cf6bd4c3ffeda92
+	57fd082c4199e48ca293aa5cd3a53a518800d106

Diff:

Revision 17 (+289 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/__init__.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	1 more

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

Just a few things left. Some small wording changes, and a couple things we can now implement based on a recent commit.

Looking great! Can't WAIT to land this :)

rbtools/commands/__init__.py (Diff revision 17)
Show all issues
```
"are" considered optional.
```
rbtools/commands/land.py (Diff revision 17)
Show all issues
```
"an upstream repository."
```
rbtools/commands/land.py (Diff revision 17)
Show all issues
```
"the patching step."
```

rbtools/commands/land.py (Diff revision 17)

Show all issues

Instead of hard-coding here, it's best to identify what's missing in SCMClients that this function needs to support. We can then add a can_<something> boolean attribute to SCMClient, defaulting to False, and add an equivalent to the supported SCMClients, set to True.
This would then check for those attributes instead. See the new rbt stamp for another case where this is done.
I imagine we'd want can_merge and can_push_upstream.
This way, other SCMClients will get support for this when those functions are implemented.

rbtools/commands/land.py (Diff revision 17)

Show all issues

I just pushed a change that adds this. See what commands/stamp.py and commands/post.py do now.

asalahli Dec. 16, 2014, 4:54 a.m.

I will do this as a follow up, in a seperate review request.

rbtools/commands/land.py (Diff revision 17)

Show all issues

"Review Board".
Can you say: "... an old version (pre-2.0)" that ..."

rbtools/commands/land.py (Diff revision 17)
Show all issues
```
Blank line between these.
```

rbtools/commands/land.py (Diff revision 17)

Show all issues

If we have approved and approval_failure, we have issue_open_count. They were introduced in the same release.

rbtools/commands/land.py (Diff revision 17)

Show all issues

The ending paren should be on the self.options.squash line.

rbtools/utils/commands.py (Diff revision 17)
Show all issues
```
We don't put parens around individual conditions.
```
rbtools/utils/commands.py (Diff revision 17)
Show all issues
```
Same here.
```
rbtools/utils/commands.py (Diff revision 17)
Show all issues
```
This looks left over from something.
```

Commit:

-	57fd082c4199e48ca293aa5cd3a53a518800d106
+	d7caf5f04fec93ceee1e3e2ed9799cfcf3635a10

Diff:

Revision 18 (+290 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/__init__.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	1 more

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

rbtools/commands/land.py (Diff revisions 17 - 18)

Show all issues

This needs to raise a CommandError for the output to be properly shown. Otherwise it'll appear as a crash.

Also, can you put the space after the %s instead of on the next line, and the % self.tool.name on the next line? Like:

raise CommandError('This command ... %s '
                   'repositories.'
                   % self.tool.name)

chipx86 Dec. 16, 2014, 4:45 a.m.

Actually, even better would be:

raise CommandError(
    'This command ... repositories.'
    % self.tool.name)

If that fits.

rbtools/commands/land.py (Diff revisions 17 - 18)
Show all issues
```
"support the `approved` field."
```

rbtools/commands/land.py (Diff revision 18)

Show all issues

You can probably make this one line by doing:

approval_failure = \
    'The review request ...'

Commit:

-	d7caf5f04fec93ceee1e3e2ed9799cfcf3635a10
+	84a1e68123f8d76ef56d01ef257ea43d02f145d7

Diff:

Revision 19 (+291 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/__init__.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	1 more

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

Commit:

-	84a1e68123f8d76ef56d01ef257ea43d02f145d7
+	2f085921e115a76329f34f52f8e49983cb96abf3

Diff:

Revision 20 (+289 -30)

Show changes

	AUTHORS
	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/commands/__init__.py
	rbtools/commands/land.py
	rbtools/commands/patch.py
	1 more

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/commands/__init__.py
    rbtools/commands/patch.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py
    setup.py
    rbtools/clients/errors.py

Ignored Files:
    AUTHORS

Status: Closed (submitted)

Change Summary:

Pushed to master (e11deb8)