Have drover ask for additional reviewers if its reverting a change made by the user

Have drover ask for additional reviewers if its reverting a change made by the user


BUG=161704, 161840
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=170588

[Ryan Tseng, 2012-11-21 00:17:01]

Not well tested, but its looking hairy so asking for early feedback now.  What
do you think of this approach?

1. Fetch current user from ~/.subversion/auth/svn.simple folder
2. Fetch rietveld url and issue from revision log
3. Import the Rietveld module and fetch the original reviewers that way.

[Ryan Tseng, 2012-11-21 22:45:25]

Okay I think its reviewable now.

[nsylvain, 2012-11-21 23:10:51]

The code is good, but I don't feel comfortable giving a L_G_T_M on this patch
right now.  This adds the rietveld dependency to drover, which is not
insignificant, this also goes deep into the internal data files of svn, which is
crazy, and fragile!

In other circumstances this would not be a good thing. We gotta get the job done
no matter how hard it is, but in this case I don't personally agree with the
premise of the bug.   I do believe that the best person to review a revert is
the owner of the code. That persons knows best all the code that got checked in
and it's pretty much the only person who can actually find mistakes in a revert.
  If you happen to revert your own patch, I still think you are the right person
to double-check your work. 

And.. who really reviews reverts? They are pretty much all rubber-stamped
anyway.

You could make the patch more simple by just keeping :
     author = text_prompt("Enter reviewer(s) to TBR", author)

But turns out that it's already there, under "PROMPT_FOR_AUTHOR" so it might
just be a matter of setting PROMPT_FOR_AUTHOR to True and calling it done.

https://codereview.chromium.org/11414098/diff/6001/drover.py
File drover.py (right):

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode607
drover.py:607: out.write('> %s\n' % line)
we should probably just use spaces instead of ">".

[Isaac (away), 2012-11-22 01:36:05]

The indentation code looks good to me; maybe we should split it into a different
CL because it will be easy to review.

As far as the more grungy svn code: can't we just run svn info in the drover
checkout to get the current svn user?

I reviewed the whole thing in case we still need the code, let me know what you
think.

https://codereview.chromium.org/11414098/diff/6001/drover.py
File drover.py (right):

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode434
drover.py:434: folder = os.path.expandvars(folder)
if you use ~ instead of $HOME, you can call expandvars for the windows path, and
use realpath for the != windows case

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode437
drover.py:437: os.path.exists(svn_simple_folder) else [])
This is a race condition on the folder or svn files getting removed. 
Recommended style is to try to list the directory (i.e. directly in the for
loop) without calling exists and catch exceptions if they occur, as you can
never prevent all OS exceptions with if blocks.

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode454
drover.py:454: and netloc in auth_info['svn:realmstring']):
better to parse the netloc from the svn url and compare for equality.

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode581
drover.py:581: current_users = getCurrentSVNUsers(BASE_URL)
can't we just run svn info in the drover checkout to get the current svn user?

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode585
drover.py:585: review_url_match = re.search("Review URL:\s*(.*)", revision_log)
the rietveld integration is nice, but it might be preferable to prompt.  And in
fact, it might be better to always prompt (and give a list of the original
reviewers), because sometimes the original committer is not a full committer,
and they should not be LGTMing the revert (they should be CCed)

Definitely if we depend on rietveld we need a timeout, and the behavior there
would be to prompt so we'll need to implement the prompt behavior either way.

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode593
drover.py:593: cl_reviewers = (",".join(issue_info["reviewers"]) if "reviewers"
in
See the code for CheckOwners in the presubmit_canned_checks.  Maybe move the
LGTM checking code into rietveld.py, and then drover and the presubmit checks
can both depend on it.  Note that their original CL might not have been LGTMed
(as is disproportionately the case with reverted changes)

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode597
drover.py:597: additional_authors = text_prompt("You appear to be reverting "
appear might be too weak, how about

"Is this a self-revert?  Please enter a TBR reviewer.

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode599
drover.py:599: cl_reviewers)
Reviewer lists can be quite long, are you sure we want this as a default?

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode601
drover.py:601: author += "," + additional_authors
author of a self-revert should not be in the TBR list

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode606
drover.py:606: for line in getRevisionLog(url, revision).split('\n'):
use splitlines()

[Isaac (away), 2012-11-22 01:43:24]

https://codereview.chromium.org/11414098/diff/6001/drover.py
File drover.py (right):

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode585
drover.py:585: review_url_match = re.search("Review URL:\s*(.*)", revision_log)
Oh, this comment is slightly stale.  I wrote it and then read the prompting code
below and forgot to modify the comment.

But still think we should change drover to always prompt (defaulting to TBRing
the original author in non-self-reverts, perhaps)

[Ryan Tseng, 2012-11-29 23:27:35]

Removed the rietveld dependency, so it doesn't query for past reviewers.

https://codereview.chromium.org/11414098/diff/6001/drover.py
File drover.py (right):

https://codereview.chromium.org/11414098/diff/6001/drover.py#newcode581
drover.py:581: current_users = getCurrentSVNUsers(BASE_URL)
AFAIK SVN user cache information isn't given in any svn commands, including svn
info
On 2012/11/22 01:36:05, Isaac wrote:
> can't we just run svn info in the drover checkout to get the current svn user?

[Isaac (away), 2012-11-29 23:50:24]

On 2012/11/29 23:27:35, Ryan T. wrote:
> AFAIK SVN user cache information isn't given in any svn commands, including
svn info

What information are you trying to get?  When I run svn info I get my username.

[Ryan Tseng, 2012-11-29 23:52:51]

On 2012/11/29 23:50:24, Isaac wrote:
> On 2012/11/29 23:27:35, Ryan T. wrote:
> > AFAIK SVN user cache information isn't given in any svn commands, including
> svn info
> 
> What information are you trying to get?  When I run svn info I get my
username.

I'm trying to get that piece of information, for some reason it doesn't show up
on my svn info.  What version of svn do you have? I have 1.6.17

[Isaac (away), 2012-11-30 00:03:51]

My mistake, I looked at a time when I was the most recent author.  It might not
be possible.

[cmp, 2012-11-30 21:08:57]

lgtm with a fix

But it would be great if this had tests.  I'm guessing it doesn't have tests
because drover doesn't have tests.  If that's true, eek!

https://codereview.chromium.org/11414098/diff/5002/drover.py
File drover.py (right):

https://codereview.chromium.org/11414098/diff/5002/drover.py#newcode583
drover.py:583: PROMPT_FOR_AUTHOR = True
PROMPT_FOR_AUTHOR is a constant, and it's incorrect to change its value after
it's set.  Instead, you should use a lowercase 'prompt_for_author'.  I assume
PROMPT_FOR_AUTHOR is not referred to in some other scope, if so, you'll need
some other changes as well.

[commit-bot: I haz the power, 2012-11-30 23:06:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hinoka@google.com/11414098/11001

[commit-bot: I haz the power, 2012-11-30 23:09:14]

Change committed as 170588