For determining OWNERS, drop reviewers specified via '-r' that aren't in email address format.

For determining OWNERS, drop reviewers specified via '-r' that aren't in email address format.

BUG=none
R=agable@chromium.org

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

[Ilya Sherman, 2014-03-27 21:03:10]

apavlov@ emailed me today informing me that my previous change broke his
workflow.  He's accustomed to running a command like "$ git cl upload -r
pfeldman,aandrey -s" and having the tools infer the email addresses he means for
pfeldman and aandrey.  Seems safe to do that here as well.

[agable, 2014-03-27 21:23:59]

On 2014/03/27 21:03:10, Ilya Sherman wrote:
> apavlov@ emailed me today informing me that my previous change broke his
> workflow.  He's accustomed to running a command like "$ git cl upload -r
> pfeldman,aandrey -s" and having the tools infer the email addresses he means
for
> pfeldman and aandrey.  Seems safe to do that here as well.

I think this is a Bad Idea. Currently, you can specify reviewers by name, as
long as that name is the one they have set in Rietveld, and it Just Works. For
example, I could do "git cl upload -r 'Ilya Sherman'" and you would show up as a
reviewer on the CL. In addition, this breaks for users of
chromereviews.googleplex.com, who need to use @google.com addresses for their
reviewers.

[Ilya Sherman, 2014-03-27 21:28:24]

On 2014/03/27 21:23:59, agable wrote:
> On 2014/03/27 21:03:10, Ilya Sherman wrote:
> > apavlov@ emailed me today informing me that my previous change broke his
> > workflow.  He's accustomed to running a command like "$ git cl upload -r
> > pfeldman,aandrey -s" and having the tools infer the email addresses he means
> for
> > pfeldman and aandrey.  Seems safe to do that here as well.
> 
> I think this is a Bad Idea. Currently, you can specify reviewers by name, as
> long as that name is the one they have set in Rietveld, and it Just Works. For
> example, I could do "git cl upload -r 'Ilya Sherman'" and you would show up as
a
> reviewer on the CL. In addition, this breaks for users of
> http://chromereviews.googleplex.com, who need to use @google.com addresses for
their
> reviewers.

I see.  I think in that case, we have two options:
(1) Query Rietveld for an email address associated with a name.  If the query
fails or returns no results, the presubmit check fails.
(2) Drop non-email address reviewer names for this presubmit check improvement. 
The presubmit still issues a warning when it could be smart enough not to, but
nobody's previously-working flow is broken.

Which do you think is preferable?  Is there an even better solution that I'm
overlooking?

[M-A Ruel, 2014-03-28 00:19:35]

https://codereview.chromium.org/215153004/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/215153004/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:869: return set([_EnsureEmailFormat(reviewer) for
reviewer in reviewers])
FTR;
return set(_EnsureEmailFormat(reviewer) for reviewer in reviewers)

[Ilya Sherman, 2014-04-09 01:34:59]

Ok, went with just dropping non-email addresses instead.  PTAL.

https://codereview.chromium.org/215153004/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/215153004/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:869: return set([_EnsureEmailFormat(reviewer) for
reviewer in reviewers])
On 2014/03/28 00:19:35, M-A Ruel wrote:
> FTR;
> return set(_EnsureEmailFormat(reviewer) for reviewer in reviewers)

Done.

[M-A Ruel, 2014-04-09 01:48:36]

lgtm

[Ilya Sherman, 2014-04-09 06:17:52]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-09 09:02:16]

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

[commit-bot: I haz the power, 2014-04-09 09:06:19]

Change committed as 262641