Infer CL author and reviewer list from local state if the issue has not previously been uploaded.

Infer CL author and reviewer list from local state if the issue has not previously been uploaded.

R=agable@chromium.org
BUG=none

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

[Ilya Sherman, 2014-03-14 00:44:52]

This CL currently has failing unit tests.  I spent a while trying to understand
the mox testing framework.  While I think I understand conceptually what it's
trying to do, in practice, I wasn't able to figure out how to mock the
appropriate things to get these tests to pass (never mind to test the current
change in a useful/meaningful way).  Would you be willing to pair program with
me to figure that out and/or otherwise help me land this change?  Thanks :)

[Ilya Sherman, 2014-03-14 01:45:32]

Hmm, I realized that the author of the CL is already set correctly, so it's just
the "--reviewers" flag that wasn't being handled as well as it could be.  That
makes the CL simpler, though there's still one issue that the tests complain
about:

https://codereview.chromium.org/195793021/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/195793021/diff/20001/git_cl.py#newcode1699
git_cl.py:1699: setattr(change, 'R', ','.join(options.reviewers))
There's probably a better way to do this.  Thoughts?

[agable, 2014-03-14 15:11:03]

https://codereview.chromium.org/195793021/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/195793021/diff/20001/git_cl.py#newcode1699
git_cl.py:1699: setattr(change, 'R', ','.join(options.reviewers))
On 2014/03/14 01:45:33, Ilya Sherman wrote:
> There's probably a better way to do this.  Thoughts?

presubmit_support.py has a Change class which is capable of handling "tag
lines", i.e. lines at the end of a change description of the form R=, TBR=, and
BUG=. It saves the values of these tags into Change.tags['<tagname>']. It's also
capable of modifying those tags -- it automatically writes the R= line on upload
based on the --reviewers flags, and it automatically cleans up the R= and TBR=
lines based on who actually LGTM'd the change. You should reuse the logic there
to handle this.

https://codereview.chromium.org/195793021/diff/20001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/195793021/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:873: return None,
_ReviewersFromChange(input_api.change)
This doesn't coincide with the docstring, which says that reviewers will only be
returned if they've given approval when approval_needed==True.

[agable, 2014-03-14 15:11:44]

On 2014/03/14 15:11:03, agable wrote:
> https://codereview.chromium.org/195793021/diff/20001/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/195793021/diff/20001/git_cl.py#newcode1699
> git_cl.py:1699: setattr(change, 'R', ','.join(options.reviewers))
> On 2014/03/14 01:45:33, Ilya Sherman wrote:
> > There's probably a better way to do this.  Thoughts?
> 
> presubmit_support.py has a Change class which is capable of handling "tag
> lines", i.e. lines at the end of a change description of the form R=, TBR=,
and
> BUG=. It saves the values of these tags into Change.tags['<tagname>']. It's
also
> capable of modifying those tags -- it automatically writes the R= line on
upload
> based on the --reviewers flags, and it automatically cleans up the R= and TBR=
> lines based on who actually LGTM'd the change. You should reuse the logic
there
> to handle this.
> 
>
https://codereview.chromium.org/195793021/diff/20001/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
> 
>
https://codereview.chromium.org/195793021/diff/20001/presubmit_canned_checks....
> presubmit_canned_checks.py:873: return None,
> _ReviewersFromChange(input_api.change)
> This doesn't coincide with the docstring, which says that reviewers will only
be
> returned if they've given approval when approval_needed==True.

Damn it, Rietveld picked up the lg2m from my inline comment. Just in case -- not
lgtm.

[Ilya Sherman, 2014-03-18 01:18:34]

https://codereview.chromium.org/195793021/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/195793021/diff/20001/git_cl.py#newcode1699
git_cl.py:1699: setattr(change, 'R', ','.join(options.reviewers))
On 2014/03/14 15:11:03, agable wrote:
> On 2014/03/14 01:45:33, Ilya Sherman wrote:
> > There's probably a better way to do this.  Thoughts?
> 
> presubmit_support.py has a Change class which is capable of handling "tag
> lines", i.e. lines at the end of a change description of the form R=, TBR=,
and
> BUG=. It saves the values of these tags into Change.tags['<tagname>']. It's
also
> capable of modifying those tags -- it automatically writes the R= line on
upload
> based on the --reviewers flags, and it automatically cleans up the R= and TBR=
> lines based on who actually LGTM'd the change. You should reuse the logic
there
> to handle this.

The Change class doesn't actually ever update the R= line.  That's handled by
the ChangeDescription class in this file.  I've added a method to the Change
class to update and re-parse the description.  This changes the class from an
immutable one to a mutable one.  Is that ok?

https://codereview.chromium.org/195793021/diff/20001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/195793021/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:873: return None,
_ReviewersFromChange(input_api.change)
On 2014/03/14 15:11:03, agable wrote:
> This doesn't coincide with the docstring, which says that reviewers will only
be
> returned if they've given approval when approval_needed==True.

Good catch.  Fixed.

[agable, 2014-03-18 17:56:52]

LGTM.

https://codereview.chromium.org/195793021/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/195793021/diff/20001/git_cl.py#newcode1699
git_cl.py:1699: setattr(change, 'R', ','.join(options.reviewers))
On 2014/03/18 01:18:35, Ilya Sherman wrote:
> On 2014/03/14 15:11:03, agable wrote:
> > On 2014/03/14 01:45:33, Ilya Sherman wrote:
> > > There's probably a better way to do this.  Thoughts?
> > 
> > presubmit_support.py has a Change class which is capable of handling "tag
> > lines", i.e. lines at the end of a change description of the form R=, TBR=,
> and
> > BUG=. It saves the values of these tags into Change.tags['<tagname>']. It's
> also
> > capable of modifying those tags -- it automatically writes the R= line on
> upload
> > based on the --reviewers flags, and it automatically cleans up the R= and
TBR=
> > lines based on who actually LGTM'd the change. You should reuse the logic
> there
> > to handle this.
> 
> The Change class doesn't actually ever update the R= line.  That's handled by
> the ChangeDescription class in this file.  I've added a method to the Change
> class to update and re-parse the description.  This changes the class from an
> immutable one to a mutable one.  Is that ok?

You're right, I was working from faulty memory :) Your change is the right thing
to do.

[Ilya Sherman, 2014-03-21 01:48:02]

Thanks for the review.  Please take another look -- I've now fixed all the
presubmit failures, including adding some unit tests.

[Ilya Sherman, 2014-03-24 20:55:09]

Aaron, did you want to take another look?  I've made changes to the unit tests
since your last review.

Also, for whatever reason, Rietveld is picking up your "not lgtm" over your
"lgtm".  Is that a bug, or did you maybe send the "lgtm" from a different
account?

[agable, 2014-03-25 17:42:26]

On 2014/03/24 20:55:09, Ilya Sherman wrote:
> Aaron, did you want to take another look?  I've made changes to the unit tests
> since your last review.
> 
> Also, for whatever reason, Rietveld is picking up your "not lgtm" over your
> "lgtm".  Is that a bug, or did you maybe send the "lgtm" from a different
> account?

Was OOO. The test fixes LGTM. I have no clue why Rietveld still thinks that the
anti-lg2m takes precedence. If it still does after I send this message, we
should file a bug.

[Ilya Sherman, 2014-03-25 17:45:55]

On 2014/03/25 17:42:26, agable wrote:
> On 2014/03/24 20:55:09, Ilya Sherman wrote:
> > Aaron, did you want to take another look?  I've made changes to the unit
tests
> > since your last review.
> > 
> > Also, for whatever reason, Rietveld is picking up your "not lgtm" over your
> > "lgtm".  Is that a bug, or did you maybe send the "lgtm" from a different
> > account?
> 
> Was OOO. The test fixes LGTM. I have no clue why Rietveld still thinks that
the
> anti-lg2m takes precedence. If it still does after I send this message, we
> should file a bug.

Sorry, didn't think to check your calendar.  Thanks for the review -- Rietveld
seems to have accepted it this time :)

[Ilya Sherman, 2014-03-25 17:45:59]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-25 17:46:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/195793021/80001

[commit-bot: I haz the power, 2014-03-25 17:47:59]

Change committed as 259250