1345: post-review does not process a *pending* Perforce changeset in v0.2 beta3
- Fixed
- Review Board
| lon***@gmai***** (Google Code) (Is this you? Claim this profile.) | |
| Feb. 6, 2010 |
What version are you running?
RBTools v0.2 beta3 dev
What's the URL of the page containing the problem?
post-review command line tool
What steps will reproduce the problem?
1.have a pending changeset in Perforce, with some Affected files
2.submit a review using post-review
3.
What is the expected output? What do you see instead?
The review should be submitted.
Here is fails with:
>>> Generating diff for changenum 969273
>>> p4 describe -s 969273
Couldn't find any affected files for this change.
The same scenarios works with v0.2 beta2
What operating system are you using? What browser?
Linux
Please provide any additional information below.
I believe the problem comes from the addition of:
+ description = []
under if '*pending*'
for Support older p4d, pending changelists on 9/21.
debug("Generating diff for changenum %s" % changenum)
- description = execute(["p4", "describe", "-s", changenum],
- split_lines=True)
+ description = []
- if '*pending*' in description[0]:
+ if changenum == "default":
cl_is_pending = True
+ else:
+ description = execute(["p4", "describe", "-s", changenum],
+ split_lines=True)
+
+ if '*pending*' in description[0]:
+ cl_is_pending = True
+ description = []
+
+ v = self.p4d_version
+
+ if cl_is_pending and (v[1] < 2002 or (v[1] == "2002" and v[2] < 2)):
+ # Pre-2002.2 doesn't give file list in pending changelists, so we
+ # have to get it a different way
I should have gone a bit further.
The last else is misplaced in the diffs below. It should be at the same level as the
for.
Thank you
Laurent
- # Get the file list
- for line_num, line in enumerate(description):
- if 'Affected files ...' in line:
- break
else:
- # Got to the end of all the description lines and didn't find
- # what we were looking for.
- die("Couldn't find any affected files for this change.")
+ # Get the file list
+ for line_num, line in enumerate(description):
+ if 'Affected files ...' in line:
+ break
+ else:
+ # Got to the end of all the description lines and didn't find
+ # what we were looking for.
+ die("Couldn't find any affected files for this change.")
Since I am at it, 2 additional issues:
1) wrong index offset when testing for Perforce version
+ # if v[1] < 2002 or (v[1] == "2002" and v[2] < 2):
+ if v[0] < 2002 or (v[0] == "2002" and v[1] < 2):
+ # if cl_is_pending and (v[1] < 2002 or (v[1] == "2002" and v[2] < 2)):
+ if cl_is_pending and (v[0] < 2002 or (v[0] == "2002" and v[1] < 2)):
2) Specific to our installation maybe. The test in:
if '*pending*' in description[0]:
is fragile. In our installation, our wrapper prints an extra line, so the test fails.
Thanks for looking into this, but please post patches on http://reviews.review-board.org/
-
+ PendingReview + Component-RBTools
I was able to reproduce using the latest version of postreview.py (
http://github.com/reviewboard/rbtools/blob/7c2e3186477c529008fc69921e39509c1c93ae06/r
btools/postreview.py ) but I believe the original steps to reproduce in this bug are
actually incorrect. Instead of a pending changeset, you start with a submitted
changeset:
What steps will reproduce the problem?
1.have a *submitted* changeset in Perforce, with some Affected files
2.submit a review using post-review
Using blame, I tracked down the issue to a commit done on September 21, 2009 :
http://github.com/reviewboard/rbtools/commit/864077d8502c856dfc8428477fc59265040a8df6
The fix was pretty simple as follows:
~/bin $ diff -c3 post-review.latest post-review.latest.fixed
*** post-review.latest Tue Jan 26 16:12:20 2010
--- post-review.latest.fixed Tue Jan 26 16:14:12 2010
***************
*** 1623,1632 ****
for line_num, line in enumerate(description):
if 'Affected files ...' in line:
break
! else:
! # Got to the end of all the description lines and didn't find
! # what we were looking for.
! die("Couldn't find any affected files for this change.")
description = description[line_num+2:]
--- 1623,1632 ----
for line_num, line in enumerate(description):
if 'Affected files ...' in line:
break
! else:
! # Got to the end of all the description lines and didn't find
! # what we were looking for.
! die("Couldn't find any affected files for this change.")
description = description[line_num+2:]
The issue is that the else was accidentally matched with the previous “if” when in
fact it was originally for the “for” loop (as it existed in the February 16, 2009
version that we were using
http://github.com/reviewboard/reviewboard/blob/1b5c22c4f70f4fb530c81655c12bd694bf4b75
25/contrib/tools/post-review ).
To fix, I just had to move the 4 lines for the else statement back just 4 spaces.
Python defines the “for” statement to optionally have an “else” statement.
Specifically, the doc says: “A break statement executed in the first suite terminates
the loop without executing the else clause’s suite.”
http://docs.python.org/reference/compound_stmts.html#for
Easy link for the curious: http://github.com/reviewboard/rbtools/commit/84f17d2c858213f726ab0a2f33bfcb3c4d765009