Yes, I think it is. We just haven't had time in the last few days to take a look.

Oops. My editor was set to two spaces for some reason.

The only SCMs I'm really familiar with are Perforce and Git; I was coming at this from a Perforce standpoint, where if the user includes files on an ad hoc basis, rbt won't know which changelist to stamp. It's a question of practicality: if a user has decided to include a single file from each of two different changelists, which one should be stamped?
Ideally, rbt post -s should behave [almost] exactly the same as rbt post && rbt stamp. So what should happen if someone tries to stamp onto individual files in Perforce (since -I is a global option)? I didn't actually test that out when I added stamping for Perforce, but to me the only somewhat-reasonable thing to do is to stamp every changelist that contains a file in the reviewed set, which I don't particularly like. Maybe it should stamp, but only if the file set is all under a single changelist?
In Git, where we can just stamp HEAD no matter what, this isn't so much an issue because there's only one "active" commit at any given time, so we can probably safely let stamping work in that scenario.

I just tested this out with Perforce. Here's what happens:

- If I include a changelist and an ad-hoc file, rbt can't create a diff and exits.

- If I include an ad-hoc file that's not in a changelist, rbt fails to stamp (because we can't stamp the default changelist), but posts the review anyway.

- If I include an ad-hoc file that is in a changelist, rbt can't create a diff and exits.
In Git, I can post a review with just the ad-hoc files, and rbt stamps my latest commit message. This seems like reasonable behavior.
I guess I'm okay with letting the stamp go through, because it will fail anyway when it doesn't make sense.

My motivation here was to smooth out the perhaps-unintended functionality of rbt post changelist_num under Perforce. When we want to update an existing review, we don't have to use -u, because rbt self-recovers from the "review request already exists for this change ID" error by changing the create to an update. Since the tool works reliably this way, it's inconvenient and unnecessary for developers to have to think about whether or not it's the first time they're submitting the code for review, and thus whether or not to add -u. If we just leave it off all the time, everything works fine (in my current organization, we have a GUI control in our tools to post a review from a changelist, so it's rather impossible -- or at least greatly cumbersome -- to try to figure out whether we should be creating or updating).
I understand that rbt doesn't follow that paradigm with Git, but it does with Perforce whether you like it or not, and I suspect there are many people/organizations who currently use the tool in that fashion and would mourn the loss of convenience if it were to change. So I don't want to break it.

Back to your question: No, I don't particularly think it's necessary to include the stamp in the review request description. However:


It makes sense that, if we're guessing the review description from the commit description, that they match, and if I post and stamp in the same action, I would thus expect both things to be updated.


The more important consideration: given the way I can "update" the review under perforce, if I post and stamp the review, then post it again, rbt will re-parse the changelist description and try to add the URL into the description on ReviewBoard. This means that ReviewBoard shows the description was changed in r2 even if the developer didn't manually modify the changelist description, consequently wasting space on the review page and confusing the reviewers by showing them a "changed description" that didn't really change in any meaningful way. So that's really what I'm trying to avoid.



I can think of three potential ways to mitigate the second issue:


Do what I did in this review


Change the guessing logic to ignore the stamp string when parsing the summary and description (I don't like this because it might lead users to wonder why their entire commit message isn't getting parsed)


Change the auto-recover logic so that when rbt finds out about the existing review for the given commit ID, it treats everything like an update (exactly as if the user had given -u) from that point forward, and would then not try to re-guess the summary/description unless the user explicitly specified those options when running the command.

The more I think about it, I see option 3 as the best thing to do (it will make things work more nicely even if there's no stamping involved). But you guys should be the ones who ultimately decide that behavior.

I tried to implement option 3, but it's somehow pulling the commit description anyway if I run rbt post cln again after stamping (looks like it's being caused by the review_request.update on 417-419, but I can't figure out why). I think it's going to be too risky to get this working and cover the edge cases correctly, so I'm sticking with my original approach (stamp, and then guess/parse/update the description fields).
It looks like this Perforce stuff was originally implemented in this commit: https://github.com/reviewboard/rbtools/commit/5c889c93

Thanks! Is there any documentation I should add in any other places, or will the doc gen tools get everything it needs from the option description?

The docs should pick it up from the option description. The only thing I can think of that might be useful is a note in the rbt stamp docs about this feature.

That does seem like a good idea. I'll add it.