Summary

post-review: support perforce pending changelists

Review Request #804 — Created April 3, 2009 and discarded

Information

Owner

mw_triad

Repository

Review Board SVN (deprecated)

Branch

Bugs

1020

Depends On

Reviewers

Groups

reviewboard

People

Description

This adds support for pending changelists, including the default changelist, to post-review.

Testing Done

This patch is being used internally in my $DAYJOB group; works great from all indications

/trunk/rbtools/scripts/post-review (Diff revision 1)
```
Indentation looks wrong.
```
/trunk/rbtools/scripts/post-review (Diff revision 1)
```
Blank line after blocks.
```

/trunk/rbtools/scripts/post-review (Diff revision 1)

Is there a chance we'll get a \r\n? Wondering if splitlines() is a better idea.

mw_triad April 23, 2009, 1:47 a.m.

Not unless 'd += "...\n"' is magically adding a "\r" as well?

Note that this is splitting the string built by the previous line. (I'd omit this entirely, except I'm a Python newbie and didn't know how to build it as an array directly :-).)

/trunk/rbtools/scripts/post-review (Diff revision 1)
```
This blank line should stay.
```

/trunk/rbtools/scripts/post-review (Diff revision 1)

Can we just roll this up into the previous cl_is_pending check?

mw_triad April 23, 2009, 1:38 a.m.

Not if you want to omit it entirely ;-).

/trunk/rbtools/scripts/post-review (Diff revision 1)

Not wild about this. It's one thing to call a new method, and another to change expectations for return values.

I don't feel diff is the right place to really transform changenum, so maybe another function is in order. Perhaps a function that returns whether or not a changenum is valid for upload to a server. Then new_review_request could call this before determining whether to upload it, like:

   if changenum and tool.is_uploadable_changenum(changenum):

Not sure about that name, but something along those lines.

mw_triad April 23, 2009, 1:39 a.m.

Makes sense (even if it's duplicated code). At the time, getting it to work was higher on my priority list than doing it elegantly.

Change Summary:

Rebase to r1929 and improve based on feedback.

This adds a new function submitted_changenum to get the changenum to feed RB from the changenum being posted, instead of overloading diff().

Diff:

Revision 2 (+43 -11)

Show changes

/trunk/rbtools/scripts/post-review

I'm not so hot about this change, though I'm having trouble describing exactly why.

Some of the terminology and the statements in issue #1020 are what confuse me.  post-review handles pending changelists just fine.  It just doesn't handle the *default* changelist.  When I first started using reviewboard I thought it was a little odd that I had to run "p4 change" in order to post a review, but I got used to it and it ends up being convenient to keep the changelist description in sync with the reviewboard description.

The only thing I can see about this change is that it clobbers the change number for pending changelists (for non-default changes).  I think that would cause problems when you try to repost a review without specifying -r.

mw_triad April 28, 2009, 2:01 a.m.

That's... interesting. It most assuredly doesn't work over here; 'p4 describe' doesn't give a list of open files for pending changelists (besides not working with 'default'). And the server (we are running 1.0beta2) rejects review requests where the changenum is set and is a pending changelist (possibly for the same reason?).

That said, our p4d is ancient; maybe the behavior of 'describe' changed.

The change to generating the file list is (or should be) harmless (and adds support for non-numbered pending changelists, i.e. "default"). If I change stripping the changenum to do so only with p4d < 2003.x, would that be satisfactory?

As for stripping the change number causing problems, it most certainly does not, except so far as the request description isn't pulled from the pending changelist. Which as mentioned, doesn't work for us anyway, and also doesn't apply to "default" changelists. It is not necessary to specify -r; RB handles perforce requests with no change number just fine (I can say that with confidence since none of the many-dozen requests on our server against the perforce repo are problematic in any way, issue #1021 notwithstanding).

Change Summary:

Don't strip change number unless p4d version is "old". Also fix breakage with recent no-change-number support.

NOTE: I have no way of knowing what is the correct threshold for p4d version to strip vs. not strip, so I'm arbitrarily picking "any p4d prior to 2003.x" (which includes our p4d) as the cutoff. If anyone knows the exact version where "describe -s" started listing files for pending changelists, the test should be changed to reflect that...

Diff:

Revision 3 (+47 -11)

Show changes

/trunk/rbtools/scripts/post-review

Change Summary:

According to the p4d release notes, the behavior of 'p4 describe' changed in 2002.2 (perforce issue #35391); update the strip-change-number test to reflect this.

Diff:

Revision 4 (+47 -11)

Show changes

/trunk/rbtools/scripts/post-review