New issue
Advanced search Search tips

Issue 791489 link

Starred by 2 users

Issue metadata

Status: Untriaged
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

check_reviewers can't deal with list of TBR emails

Project Member Reported by mattcary@chromium.org, Dec 4 2017

Issue description

If I TBR multiple people (eg TBR: a@chromium.org, b@chromium.org), depot_tools fails while doing git cl upload:

  File "/usr/local/google/home/mattcary/lib/depot_tools/owners.py", line 188, in _check_reviewers
    assert all(self.email_regexp.match(r) for r in reviewers), reviewers
AssertionError: set(['mattcary@chromium.org', 'twellington@chromium.org, mdjones@chromium.org'])


I think that email_regexp is too fussy and expects only single email, not a list.
 
Can you provide the full commit message and/or command line you're running when this error shows up?

The problem is that it is somehow failing to split "twellington@chromium.org, mdjones@chromium.org" into two email addresses, and trying to run the email_regexp against the pair instead of each individually.

It'll be a lot easier to fix if I know where that pair is coming from.
I'm not 100% sure the following is the original commit message, but it's been failing consistently enough that I'm pretty sure this will reproduce the issue (it's coming up a lot while sheriffing and it really slows down my flow disabling tests ;)  Take the text between >>> and <<<:
>>>
Disable flaking WebRtcEventLogManagerTest.LocalLogMayNotBeStartedTwice

Bug:  787809 
TBR: eladalon@chromium.org, guidou@chromium.org
<<<

More precisely, the above is the text of my local git commit, and then I run the following command and get this full stack trace (of which the error mentioned above is the last line):

mattcary@mattcary:/clank/sheriff/src$ git cl upload                                                                    
Running presubmit upload checks ...                                                                                    
Traceback (most recent call last):                                                                                     
  File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 6193, in <module>                             
    sys.exit(main(sys.argv[1:]))                                                                                       
  File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 6175, in main                                 
    return dispatcher.execute(OptionParser(), argv)                                                                    
  File "/usr/local/google/home/mattcary/lib/depot_tools/subcommand.py", line 252, in execute                           
    return command(parser, args[1:])                                                                                   
  File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 5036, in CMDupload                            
    return cl.CMDUpload(options, args, orig_args)                                                                      
  File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 1642, in CMDUpload                            
    change=change)                                                                                                     
  File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 1574, in RunHook                              
    gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit())                                                       
  File "/usr/local/google/home/mattcary/lib/depot_tools/presubmit_support.py", line 1412, in DoPresubmitChecks         
    results += executer.ExecPresubmitScript(presubmit_script, filename)                                                
  File "/usr/local/google/home/mattcary/lib/depot_tools/presubmit_support.py", line 1319, in ExecPresubmitScript       
    result = eval(function_name + '(*__args)', context)                                                                
  File "<string>", line 1, in <module>                                                                                 
  File "<string>", line 2764, in CheckChangeOnUpload                                                                   
  File "<string>", line 2480, in _CommonChecks                                                                         
  File "/usr/local/google/home/mattcary/lib/depot_tools/presubmit_canned_checks.py", line 1161, in PanProjectChecks    
    input_api, output_api, source_file_filter=None))                                                                   
  File "/usr/local/google/home/mattcary/lib/depot_tools/presubmit_canned_checks.py", line 974, in CheckOwners          
    reviewers_plus_owner)                                                                                              
  File "/usr/local/google/home/mattcary/lib/depot_tools/owners.py", line 174, in files_not_covered_by                  
    self._check_reviewers(reviewers)                                                                                   
  File "/usr/local/google/home/mattcary/lib/depot_tools/owners.py", line 188, in _check_reviewers                      
    assert all(self.email_regexp.match(r) for r in reviewers), reviewers                                               
AssertionError: set(['mattcary@chromium.org', 'eladalon@chromium.org, guidou@chromium.org'])    
Ah. Hmm, no tooling should be producing commit messages which look like that.

This is clearly a bad thing and I should work on improving this interaction but for now, you can get around this in one of two ways:
1) Format the commit message like this:
"""
Disable flaking SomeSuite.ThatOneAnnoyingTest

TBR=someone@foo.tld, someone@bar.tld

Bug:  123456 
"""

or 2) Don't put the TBR in the commit message at all, and upload the change with "git cl upload --tbrs=someone@foo.tld,someone@bar.tld".
No tooling created that, I wrote the commit message myself.

Ah, so TBR uses "=", but Bug uses ":" now? With rietveld, Bug used "=" but I noticed it switched to ":" with gerrit. I assumed the other annotations used a colon as well.

IMHO, I shouldn't have to think too hard about what punctuation to use. It's not too hard to make the parsing intelligent; I'd be happy to make the changes to depot_tools if you all our resource constrainted.
You're absolutely right, which is why I said I need to work on fixing this and didn't close the bug :)

I do suggest using "--tbrs=foo,bar" in the mean time, however.
(And unfortunately, it *is* hard to make the parsing cleaner in a sane way. :()
Sorry! I misread your comment. Thank you for staying on the bug, it's very much appreciated, creating reliable input processing is very fiddly.

Sign in to add a comment