Modify gcl to use suggested_reviewer output from presubmit_support.

Modify gcl to use suggested_reviewer output from presubmit_support.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78329

[M-A Ruel, 2011-03-15 19:11:57]

http://codereview.chromium.org/6696010/diff/1/gcl.py
File gcl.py (right):

http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode41
gcl.py:41: import breakpad  # pylint: disable=W0611
breakpad _must_ be the first.

http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode328
gcl.py:328: if subject.find("\r\n") != -1:
http://docs.python.org/library/stdtypes.html#str.splitlines

you can in fact:
assert not ('\r' in subject or '\n' in subject)

http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode332
gcl.py:332: if len(subject) > 77:
The limit is in fact 100.

if len(subject) > 100

http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode333
gcl.py:333: subject = subject + "..."
subject[:97]

http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode339
gcl.py:339: description = property(_get_description, _set_description)
Use python 2.2 format instead:
http://docs.python.org/library/functions.html#property

http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode1107
gcl.py:1107: if not any(reviewers_re.match(l) for
if not any(
    reviewers_re.match(l) for l in description.splitlines()):

http://codereview.chromium.org/6696010/diff/1/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/6696010/diff/1/git_cl/git_cl.py#newcode835
git_cl/git_cl.py:835: if not options.bypass_hooks and not options.force:
What's the difference between --force and --bypass_hooks?

[chase, 2011-03-15 20:48:58]

http://codereview.chromium.org/6696010/diff/1/gcl.py
File gcl.py (right):

http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode328
gcl.py:328: if subject.find("\r\n") != -1:
single quotes here and below, please

http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode1044
gcl.py:1044: "\nCommitted: " + viewvc_url + revision)
can this be reverted?  seems like there's no difference between pre- and post-
except post- doesn't use +=

http://codereview.chromium.org/6696010/diff/1/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/6696010/diff/1/git_cl/git_cl.py#newcode835
git_cl/git_cl.py:835: if not options.bypass_hooks and not options.force:
On 2011/03/15 19:11:57, Marc-Antoine Ruel wrote:
> What's the difference between --force and --bypass_hooks?

IIRC, originally --bypass_hooks was to not run things like presubmit checks and
to not check the status of the tree before committing.  --force would run
presubmit checks and not check the status of the tree.

This change makes the two flags equivalent.  I would think we could ditch one
flag if we're willing to make them equivalent.  Can we make git-cl and gcl match
here?  (I think gcl only has a force flag equivalent.)  If we make them
equivalent, the deprecated flag can be supported for a while with a message
('--bypass_hooks is going away, please use --force in the future').

[Dirk Pranke, 2011-03-15 23:36:39]

On 2011/03/15 20:48:58, chase wrote:
> http://codereview.chromium.org/6696010/diff/1/gcl.py
> File gcl.py (right):
> 
> http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode328
> gcl.py:328: if subject.find("\r\n") != -1:
> single quotes here and below, please
> 
> http://codereview.chromium.org/6696010/diff/1/gcl.py#newcode1044
> gcl.py:1044: "\nCommitted: " + viewvc_url + revision)
> can this be reverted?  seems like there's no difference between pre- and post-
> except post- doesn't use +=

The post- is using property overrides, and I'm not actually sure if the python
interpreter is smart enough to do the right thing. I'll test it and find out,
and if so, yes, I'll revert it.

> 
> http://codereview.chromium.org/6696010/diff/1/git_cl/git_cl.py
> File git_cl/git_cl.py (right):
> 
> http://codereview.chromium.org/6696010/diff/1/git_cl/git_cl.py#newcode835
> git_cl/git_cl.py:835: if not options.bypass_hooks and not options.force:
> On 2011/03/15 19:11:57, Marc-Antoine Ruel wrote:
> > What's the difference between --force and --bypass_hooks?
> 
> IIRC, originally --bypass_hooks was to not run things like presubmit checks
and
> to not check the status of the tree before committing.  --force would run
> presubmit checks and not check the status of the tree.
> 
> This change makes the two flags equivalent.  I would think we could ditch one
> flag if we're willing to make them equivalent.  Can we make git-cl and gcl
match
> here?  (I think gcl only has a force flag equivalent.)  If we make them
> equivalent, the deprecated flag can be supported for a while with a message
> ('--bypass_hooks is going away, please use --force in the future').

In git-cl, there is an "about to commit, are you sure you want to continue?"
prompt that happens even if you --bypass-hooks. --force overrides that. I think
the two switches are equivalent in gcl.

That said, I tend to think of "--force" as "--don't prompt me, just fail if
there are issues", and "--bypass-hooks" as "--bypass the hooks" (but wouldn't,
for example, necessarily skip the closed-tree check, since it's not technically
a hook). 

One could argue that force should be "ignore the errors".

This patch is just reverting to match the behavior we had from a few days ago. I
think they're good questions to ask, but I would slightly prefer to address them
in a different patch if that's okay.

[Dirk Pranke, 2011-03-16 00:59:52]

New version uploaded. I made all of the suggested changes, with one exception.

M-A, you said that I should use the "Python 2.2" version of the property()
method. AFAIK, I am using the 2.2 version. I can't use the @property decorator
version, because I need a setter as well, and the @description.setter decorator
syntax isn't available until 2.6 (and I'm assuming we still need to support
2.5).

Does that make sense?

Also, I back-ported the git_cl changes to the previous refactoring patch,
because I needed them there, not here :).

[chase, 2011-03-16 02:01:15]

lgtm

[William Hesse, 2011-03-16 11:50:24]

This change is causing gcl to fail on my linux system, and the systems of other
V8 developers in Aarhus.
Filing issue 
http://code.google.com/p/chromium/issues/detail?id=76356.

We may revert this change, if the change author is not available.