Summary

post-review: Support older p4d, pending changelists

Review Request #1031 — Created Sept. 9, 2009 and submitted

Information

Owner

mw_triad

Repository

RBTools

Branch

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This fixes post-review to work on older p4d (which I am stuck with at $DAYJOB); without this change post-review can only be used on submitted changelists. This also adds support for the default changelist (for all p4/p4d versions).

See also http://reviews.review-board.org/r/804/ (replacing the diffs there failed, hence the new request).

Testing Done

rbtools/postreview.py (Diff revision 1)
```
Blank line between these.
```

rbtools/postreview.py (Diff revision 1)

Please add a doc string explaining what this does.

Also, this should probably be get_submitted_changenum. Is that really what this is, though? It doesn't actually return a submitted version of a changenum. It's more that it determines whether it's pending or not and returns None if it's not...

This seems like it would break existing Perforce setups where you have a pending changeset that isn't yet submitted. It would take the perfectly valid changenum and return None, which is not what we want.

The *pending* check here and elsewhere in the code seems to really change the existing behavior, and we need to make sure it doesn't.

mw_triad Sept. 10, 2009, 1:53 a.m.

...except that (IIRC) RB flat out breaks if you try to feed it a pending cl number. Maybe that's because the server has the same problem with older p4d as post-review? But how to deal with it, then? I suppose this function could do the same version check, and only return None for default if p4d is newer. That would be suboptimal in some weird case where the RB server is different from the one seen by post-review, but I don't know if that's even possible, and anyway the alternative is to change Review Board itself.

As for the name... hmm... what about "sanitize_changenum"? As you say it isn't "get the submitted changenum", the actual purpose of the function is to return a changenum that is safe to be fed to Review Board.

chipx86 Sept. 10, 2009, 7:39 p.m.

I want to make sure we have the same definition of pending here.

You p4 change, describe the changeset, add the files, but don't commit. You then post this change number for review. Right?

This works for us and has for a long time. These are registered on the server, so we can pull the change description down. Does that not work on the 2002 version?

As for the function, sanitize_changenum would be fine.

mw_triad Sept. 11, 2009, 2:16 a.m.

"pending": yes, we're on the same page. That *doesn't* work (and AFAIK, never has) for us. My guess is because p4d < 2002.2 doesn't give the same information via 'p4 describe <pending-cl>' that 2002.2 and later do, i.e. the same reason I had to change post-review to go through extra hoops to get the list of files.

It's possible to work around that (and I do just that, in fact, in this patch), so I suppose Review Board could be similarly fixed. But I'm inclined to just make the same version check in sanitize_changenum. Would that be acceptable?

chipx86 Sept. 11, 2009, 5:59 p.m.

Yep, that sounds fine. I just wanted to make sure :)

rbtools/postreview.py (Diff revision 1)

It would be nice to make a function for returning the server version and then calling it here. It should return the version in a tuple, or some such, for easy comparison.

mw_triad Sept. 10, 2009, 1:50 a.m.

Good idea, will do that in the next version of the patch.

Change Summary:

Add documentation, move server version lookup to its own function, unbreak _depot_to_local.

Diff:

Revision 2 (+72 -12)

Show changes

rbtools/postreview.py

Change Summary:

Store p4d_version when we do get_repository_info (saves some operations), and don't do extra work when we have the file list from 'p4 describe'.

Diff:

Revision 3 (+71 -12)

Show changes

rbtools/postreview.py

Ship it!

```
Committed with minor changes. Thanks!
```