You mean, use 'rbt post -r 7153'? Yes, of course.
I did notice that if I just use 'rbt post' on git without specifying the review ID, rbt throws an exception rather than automatically re-using the same review. I would have expected it to automatically find the same review request, as it does for perforce, but rbt threw the exception before my change, so I'm considering that to be out of the scope of this code review.

Perforce works a bit differently, since there's a non-changing ID involved. With Git, we don't have that luxury, unfortunately. That's why the behavior is a bit unexpected (when used to the Perforce behavior).
However, if you use -u, it will try to find the matching review request. That's basically the equivalent of the Perforce behavior, and works for everything that's not Perforce.
What exception did it throw? We definitely need to take a look at that.
Thanks!

It's API Error 204: "The commit ID specified has already been used."
Can a user use -u when uploading the request for the first time as well? If so, then it would be fine, and maybe the error is expected behavior. But it would be nice if ReviewBoard realizes it's the same commit and updates the review anyway.

You're right, we should do that. I'll make a note on that.
-u doesn't work the first time. That's only used when trying to match an existing review request. We have to basically look up review requests from the user that look like the commit message. The reason we can't use commit SHAs is because they'll change every time you modify the commit, or add a new commit to the branch. There's no identifying information to key the review request off of.

Right, that makes sense. As a user, I would like to set up a command line alias for rbt post with the options I need, and be able to use that all the time, whether or not I've posted a review before.
Another thing I wanted to do, after I push this change, is to have rbt be able to guess the review ID based on the stamped URL, which might be one way to solve the issue in the git case. But since this talk is going the way of enhancements, perhaps we should take this conversation into another forum :-)

We have command aliases as of RBTools 0.7, so you can check out http://blog.beanbaginc.com/2015/01/19/composing-workflows-using-aliases-in-rbtools/ for more info.
With regards to feature enhancements, can you file them in the issue tracker on https://code.google.com/p/reviewboard/issues/list ? Thanks!

I'm not sure what the original behavior was, because these arguments were not being used previously; the invocation of this method did not send those parameters, so it's dead code as far as I can tell. It looks like it originally might have just been copy/pasted from create_commit(). The arguments make sense for create_commit(), but not so much for amend_commit() if it's decided that the message is the only thing that can be amended.
From recent edits to this file, it looks like it was decided that stamping shouldn't work at all if there are local uncommitted changes (at least as far as Git is concerned), so it doesn't make sense to me why we would ever want to add files while amending.

Ok since this isn't being used anywhere, you can ignore this.

See above.

Same as above.

Cool, that's good to know.

Do you have a suggestion for the type of exception to raise? It doesn't look like any of the existing ones are applicable for a generic perforce exception. Should I just make "PerforceError", or maybe "ScmError"?

I think a general SCMError would be fine in both this and the above case.

From the various SCM files, I inferred that a colon is the delimiter between a prefix and the actual change number, so I would be surprised/confused if there were more than one. If there are no colons, this statement returns the original string. I changed it here because I wanted to strip prefixes uniformly wherever they occurred.

Is it possible someone could be using colons in their own changesets (so it is something like dev:mychangeset) and then this will come along and do the wrong thing?
Maybe we should do tip.split(':', 1)[1] ?

Perforce changelists are always numeric (with the exception of the "default" changelist). I'm not aware of a feature for users to be able to name changelists on top of this. There are labels/tags (which I've never used) http://www.perforce.com/perforce/r14.2/manuals/p4guide/chapter.labels.html, but it looks like rbt won't work with these anyway. So I think it's safe to say that the change number will never have a colon, unless Perforce makes radical changes to its overall design.
In the perforce case, I think it's more likely that a prefix would contain another colon, but given that the colon is the de facto delimiter, I find that quite unlikely as well. If we want to encourage a standard way of stripping prefixes across SCMs though, then your suggestion will be equivalent in this case and more reliable in the general case.

0 or 1 (see above)

See my above comment.

It makes more sense to me as a reader to keep it this way. I was conceptualizing that there are two "modes" of scanning the document: either copying line-for-line, or ignoring the part that is the old description. What do you think?

Its just a style thing and it doesn't really matter. it is up to you.

I'll leave it this way. I did add a couple more comments to make it easier for readers to follow (and fixed an edge case in the logic as well).

You know, it's used so often that I never realized it's not an official acronym. I think it means changelist number, but as is the case with most acronyms, half the time people just mean changelist. I've changed this instance, but note that this is far from the only time the term is used in this file.

Will do. Based on the other comments, it seems like there might be more subtleties in getting this to behave nicely with the different SCMs.

Can you suggest a better name? This confused me when I was reading the code, because it's doing more than merely "checking" (which, to me, implies a read-only thing). It's actually overwriting some values, and it would be nice if that's readily apparent from the method name.

I'd leave it alone for now. If you want, you can make changes like this in another review request.

Sure, that's reasonable.

See above.

Since get_user and get_repository_id involve API calls, I wanted to avoid doing them multiple times. My implementation of determine_review_request_id (inside stamp.py) calls the API's get_review_requests multiple times (some inside guess_existing_review_request and some not). Moving these functions out of guess_existing_review_request removed the "need" for them to be invoked twice.

In this case, rbt stamp supports only git and perforce, so there's no need to have the options for other SCMs available until someone extends the command's functionality to those SCMs. 
Regarding the specific options to supply, they appear to be global options pertaining to the setup of the perforce client. If users want a clear usage pattern among the different commands in rbt, it would make sense to make all the perforce options available in all the commands that interact with perforce. Since they are accessed in the run_p4 method itself, it seems likely that some users will want/need to have them set up for all their interactions with perforce.

Okay, sounds good to me since they are required for interacting with p4.

Sure. I'll still have to insert changenum using bracket notation though, since it's not there in all cases.

Yes thats fine.

According to my reading of the ReviewBoard docs, there will never be more than one review request associated with a particular commit ID. So if there's not one, there's zero, and we can fall back to the next guessing strategy.
Since I only use rbt with perforce, I wasn't acquainted with the -u option until very recently. More importantly, at my workplace, we invoke rbt directly from P4V (the perforce GUI), so there's no command line to let us choose the correct review anyway. The addition of this guessing strategy might actually break the git behavior; maybe it should be attempted only if the repository supports changesets?
I'd really like to see "search for the stamp URL in the change description" added as a guessing strategy, but that could be a significant behavioral change so I didn't do it here.

Ah yes. I missed the commit_id bit. There definitely is only going to be one review request at most.
However, I think its a good idea to make our assumptions explicit with an assert, e.g.

if review_requests:
    assert len(review_requests) == 1

    # ...


We definitely don't want to break any existing behaviour. Updates to the guessing should probably be done in another review request, since they're a big change on their own.

I want to keep the change-based guessing for perforce, because it keeps the behavior consistent with rbt post for perforce (not using -u). ReviewBoard finds the review ID corresponding to the specific changelist, and then automatically updates it. In the same way, rbt stamp will automatically find the same review request and automatically stamp.
I've placed this feature behind a "repository_info.supports_changesets" flag, however, so the git behavior will remain the same.

That seems OK.

I took that out on purpose, because in Perforce there can be multiple pending changesets and they don't have a relative ordering. A changelist number MUST be specified, otherwise we can't infer which one is "latest".
I'm also curious about the git implementation, which concatenates the messages from multiple commits and writes them (along with the review URL) all to the latest commit. I could see this getting confusing when rebasing/squashing.

In this case the docstring has to be reworded. Perhaps "a commit message" instead of "the commit message" ?

"a commit message" is vague, but this is a one-liner and it's well-documented at the class level, so I think we can get away with it.

Yep. I was using it in two places before, but then I wasn't. Note that I changed the way it checks to see if the stamp is already there. Before, it looked for the stamp string, but now it looks for the URL. This has the benefit (in my opinion) of allowing the new URL to be stamped if the code review is changed for some reason.

If more functionality is added in the future surrounding the stamp string, such as guessing the review request based on the stamped URL, it would make sense to pull this back out into a constant. It also makes it more apparent that it's special and shouldn't ever be modified (then old commits would have a different stamp string than new ones, which could cause compatibility issues).

Yeah. In stamp, I'm returning the ID and the URL, otherwise I would have to call reviewboard again separately to get the URL.
I am somewhat uncertain about returning the entire "review request" object though, when not all the fields will actually be filled out (callers might want to grab more info out of it and not be able to). Do you think it's good enough to mention the limitation in the docstring?

Yeah this is some good info for it!
Perhaps this should fill out all fields by default and you can request fields via an extra request_fields paramter that defaults to None ?

I'm worried about that too: the method requires a certain base set (id, summary, description, draft) to do its own internal guessing, so we shouldn't let the caller specify a smaller set of fields than that. We could have guess_existing_review_request() take the union of those specified by the caller and those it always needs, but that leads to a more confusing implementation/behavior that I'd rather avoid. In this case I'd prefer to just make a note in the documentation that only the specific fields will be present.
I'm specifically not mentioning in the docstring which fields are returned, because it offers the opportunity for the documentation to become out of sync with the code, which isn't much a concern in this case because it's an internal function.

Good catch; thanks.

Aha. I was trying to match them to the order they appeared in errors.py.

I was trying to keep this under 80 characters :) To that end, I've changed it to """Ask ReviewBoard for the review request ID for the tip revision."""

Yeah, this would be best as a comment maybe in the form of:

#: One-line summary
#:
#: Multi-line description


This is, for Sphinx (our doc builder), the equivalent of a docstring below, but is a lot less confusing (I've always despised this about Python).
Along with that, the wording needs some changes. We don't prefix summaries with "This is the ..." or anything like that, as that part is implied. rbt stamp is just one of the users of this; rbt patch and rbt land are others.
Also, the RBTools compatibility isn't the issue, so much as the Review Board compatibility (Review Board's post-commit hooks for repositories expect "Reviewed at").
Instead, how about:

#: The format string used to specify a URL to a review request in commits.
#:
#: Commands that prepare a commit message for pushing, such as rbt stamp,
#: rbt patch, and rbt land, must use this format to indicate the URL to the
#: matching review request. Review Board will parse the commit messages when
#: executing any post-receive hooks, looking for this string and a valid URL.


Also, constants should go at the top, before classes and functions.

Thanks for the explanation. I wanted to keep this constant here next to the function in which it's used, but over time that could diverge. I've replaced with your wording and moved to the top.

There is some wonkiness when viewing an interdiff between two revisions where the newer revision has been rebased. It is a known bug.

I like taking revisions because it makes it clear (by convention of the revisions variable being used throughout the project) what format we're expecting. Otherwise it would have to be explained that change_id is derived from revisions.tip, so why don't we just take revisions? Also, if we ever do want to look at base, we already have it.

What is a reST list?
I would like to list the fields, but I would also not like to have the documentation risk becoming out of sync with the implementation. I thought of saying "will contain only some fields, specified by the only_fields variable", and then perhaps putting only_fields clearly at the top of the code.

A re-structured list is formatted as follows:

* item
* item
* item

I've done the same for guess_existing_review_request and find_review_request_by_change_id, defining only_fields prominently at the top of the function and updating the documentation to note that this variable specifies the fields that will be returned.
If you would prefer I duplicate the list in the documentation, let me know and I'll change it.

I thought the docstring was supposed to be written in imperative voice?

Only the summary line, as far as I know.

Changed to "None will be returned instead."

Sure.

Removed the second one, and changed the first to this:

logging.debug('Found review request %s with status %s' % (review_request.id, status))
I'm assuming we're not ever going to have a review request without an ID, unless something is wrong enough that we don't care if the program doesn't work anymore.

Is that not apparent enough from the previous debug message that prints out the identity of the tip revision?

Changed the log messages to make things more obvious, when viewed together.

Good call. Changed to determine_review_request.

Removed the word "ID" from the single-line description. The multiline description details the tuple that is returned.

Yeah, good idea.

I was hoping for you to comment on the two old ones I left open. If you're okay with my changes, then you can mark them as you feel appropriate.

I changed the two in my function. There are a lot of other functions that still execute(['git']).

It was helpful to me to know that communicate sets the returncode, which is something that's not necessarily intuitive if you're not familiar with Popen.

Would that actively solve a problem in the scope of this change?