Blank line between these.

Make sure this wraps to 80 characters.

Also, why not just rstrip() for all these?

Both rows of values inside the [...] should be aligned.

'is' shouldn't be used for string comparisons.

This would be better as: if not self.upstream_branch:

Parameters should be aligned.

Blank line between these.

We seem to do this logic twice. I'd prefer an inline utility function like:


def get_origin(default_upstream_branch=None, ignore_errors=False):
    self.upstream_branch = default_upstream_branch or options.tracking or 'origin/master'
    upstream_remote = ...
    origin = execute(..., ignore_errors=ignore_errors)


Then you can just do:

origin = get_origin(self.upstream_branch, True)

if origin.startswith("fatal:"):
    origin = get_origin()


Or something like that.

Alignment and rstrip.

Just do:

if parent_branch:

Those two parameters can probably fit on the same line in under 80 chars.

Since we do the "%s..%s" % (ancestor, commit) twice, it'd be nice to just do this before the self.type checks and save it as revision_range or something.

I'd prefer --tracking-branch.

Also, can you update the rbtools docs (reviewboard/docs/manual/users/tools/post-review.txt) with this parameter?

I love that you're introducing unit tests for rbtools. We've needed this badly for a while :)

Some small fixes (coding style) and a handful of docstring stuff.

No need for parens around returned tuples. Should do:

self.upstream_branch, origin = \
    self.get_origin(....)

Same here with the parens.

Doc blocks in Python should describe what the function does first and foremost. This should then describe what it does with the parameters, and then what's returned. 

Blank line between these.

Should be alphabetized.

Should be one import with \ for multiple lines.

Two blank lines between classes.

Same here.

Should have something, but if not, just remove the docstring.

Make sure this wraps to 80 chars.

Blank line between these.

The alignment is off. Make sure all params align.

Maybe call this self.client, so we can reuse that name in all tests?

Really should have a docstring for the unit tests that says "Testing <some description here>"

Same with all other test_* functions.

We repeat this so much. Might be nice to just have a utility function that takes the filename, contents, and a commit message, and just does all this.

Blank line between these. We're testing another thing, so it should be its own block.

You likely couldn't post due to the domain name change. I bet it doesn't handle redirects correctly. Try modifying the .reviewboardrc on the server to point to reviews.reviewboard.org. I'll push updates later to fix this.

Almost done, a few more things.

Out of curiosity, do you know what the minimum version of Git is that supports this? I mostly only care if it's recent, as we'll need to doc/work around it.

Indentation issues.

Missing trailing period.

ignore_errors, should be on the same indentation level as [

Still would prefer we not repeat ourselves with this. Can just store it in a variable before the if statements.

The second line should be on the same indentation level as the """, not as the contents.

We reference this server many times in the file. Maybe store it as a class-devel variable?

Thanks Dan. It's committed to r5985a31.

lstrip isn't good here, for example it will change string
refs/heads/dev
to
v
insead of "dev"
From doc of lstrip
"The chars argument is a string specifying the set of characters to be removed"
>>> 'www.example.com'.lstrip('cmowz.')
'example.com'
replace('refs/heads/','',1)
or something similar would work better

fasdfasfasd