Add watchlists to 'gcl upload'...

Add watchlists to 'gcl upload'

Watchlists provides an opportunity for someone interested in a particular
code section to make comments before commit. One can subscribe to watchlists
depending on filepath (regular expressions) and she'll automatically get
cc-ed to changes when the watchlist is triggered.

Additional examples of watchlist_rules in diff repo:
http://codereview.chromium.org/118431
http://codereview.chromium.org/119356

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

[John Grabowski, 2009-06-09 17:55:46]

Glad to see this coming together!

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

http://codereview.chromium.org/118432/diff/1/4#newcode777
Line 777: # Watchlist processing
Add URL in a comment here explaining what watchlists are.  Document on
dev.chromium.org

http://codereview.chromium.org/118432/diff/1/4#newcode778
Line 778: watchlist = watchlists.Watchlists(GetRepositoryRoot())
Might be nice to add a --no_watchlists arg in case watchlists are broken (or
your tree is busted and it's an emergency or whatever) and you temporarily want
to bypass them

http://codereview.chromium.org/118432/diff/1/4#newcode786
Line 786: cc_list += ','.join(watchers)
Simplify prev 6 lines into 1.  Trailing commas are OK, right?
cc_list = ','.join(watchers) + ',' + GetCodeReviewSetting("CC_LIST")

http://codereview.chromium.org/118432/diff/1/3
File watchlists.py (right):

http://codereview.chromium.org/118432/diff/1/3#newcode8
Line 8: TODO: Create helper doc on dev.chromium.org
In addition to helper doc, brief explanation here would be good.

http://codereview.chromium.org/118432/diff/1/3#newcode30
Line 30: return None
print error here (perhaps logging.error()) so people know right away if a
watchlist file has bad syntax

http://codereview.chromium.org/118432/diff/1/3#newcode35
Line 35: """Manage Watchlists."""
More info please.

http://codereview.chromium.org/118432/diff/1/3#newcode58
Line 58: def _LoadWatchlistRules(self):
Something to think for the future: we might want >1 watchlist file (e.g. one in
"WebKit", one in "O3D" directory, etc.  Might be good to have a list of
filenames to load.

http://codereview.chromium.org/118432/diff/1/3#newcode94
Line 94: return list(watchers)
Might be nice to have a:
if __name__ == '__main__':
  # confirm watchlists parse and look good

So people can easily test

[Nirnimesh, 2009-06-10 17:24:07]

Thanks John for your comments. Uploaded new changeset.

Marc-Antoine, please suggest if you'd like this to be reviewed by someone else
too.

Thanks

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

http://codereview.chromium.org/118432/diff/1/4#newcode777
Line 777: # Watchlist processing
On 2009/06/09 17:55:46, John Grabowski wrote:
> Add URL in a comment here explaining what watchlists are.  Document on
> dev.chromium.org
> 

Done.

http://codereview.chromium.org/118432/diff/1/4#newcode778
Line 778: watchlist = watchlists.Watchlists(GetRepositoryRoot())
On 2009/06/09 17:55:46, John Grabowski wrote:
> Might be nice to add a --no_watchlists arg in case watchlists are broken (or
> your tree is busted and it's an emergency or whatever) and you temporarily
want
> to bypass them
> 

Good idea.
Done.

http://codereview.chromium.org/118432/diff/1/4#newcode786
Line 786: cc_list += ','.join(watchers)
On 2009/06/09 17:55:46, John Grabowski wrote:
> Simplify prev 6 lines into 1.  Trailing commas are OK, right?
> cc_list = ','.join(watchers) + ',' + GetCodeReviewSetting("CC_LIST")
> 
>   

Done.

http://codereview.chromium.org/118432/diff/1/3
File watchlists.py (right):

http://codereview.chromium.org/118432/diff/1/3#newcode8
Line 8: TODO: Create helper doc on dev.chromium.org
On 2009/06/09 17:55:46, John Grabowski wrote:
> In addition to helper doc, brief explanation here would be good.

Done.

http://codereview.chromium.org/118432/diff/1/3#newcode30
Line 30: return None
On 2009/06/09 17:55:46, John Grabowski wrote:
> print error here (perhaps logging.error()) so people know right away if a
> watchlist file has bad syntax
> 

Done.

http://codereview.chromium.org/118432/diff/1/3#newcode35
Line 35: """Manage Watchlists."""
On 2009/06/09 17:55:46, John Grabowski wrote:
> More info please.

Done.

http://codereview.chromium.org/118432/diff/1/3#newcode94
Line 94: return list(watchers)
On 2009/06/09 17:55:46, John Grabowski wrote:
> Might be nice to have a:
> if __name__ == '__main__':
>   # confirm watchlists parse and look good
> 
> So people can easily test
> 

Good idea.
Added a way to verify that a watchlist works.

[M-A Ruel, 2009-06-10 17:38:21]

General comments;

1) Could you rename watchlist_rules.py to WATCHLIST to be clear it's not
actually a source file or meant to be executed.

2) I think you implementation covers most use case except that it'll fail with
git-cl.

3) I have a strange feeling it should be implemented as a presubmit check. 
Something like:

def CheckChangeOnUpload(input_api, output_api):
  for item in input_api.AbsoluteLocalPaths():
    if re.match(r"bleh/.*", item):
      return [output_api.AddReviewer('me@chromium.org')]
  return []

But it may not be the right place after all. It's just that I want to keep gcl
as small as possible.

[John Grabowski, 2009-06-10 18:23:34]

> 2) I think you implementation covers most use case except that it'll fail with
> git-cl.

Can you provide info on how nirnimesh can hook into git-cl?
 
> 3) I have a strange feeling it should be implemented as a presubmit check. 
> Something like:
> 
> def CheckChangeOnUpload(input_api, output_api):
>   for item in input_api.AbsoluteLocalPaths():
>     if re.match(r"bleh/.*", item):
>       return [output_api.AddReviewer('me@chromium.org')]
>   return []
> 
> But it may not be the right place after all. It's just that I want to keep gcl
> as small as possible.

I understand the need to keep gcl small.  There are other potential places to
hook in (e.g. in rietveld itself) that we were warned against.  

It is unclear to me if presubmit is better than inside the 'gcl upload' command.
 Logically I would think no since presubmit checks the code whereas upload deals
with email et al.  But I am open to ideas.

[John Grabowski, 2009-06-10 18:36:18]

LGTM but please wait for an LGTM from at least one other person.
How about Randall?

http://codereview.chromium.org/118432/diff/7/10
File gcl.py (right):

http://codereview.chromium.org/118432/diff/7/10#newcode785
Line 785: if not no_watchlists:
double negative... might be noce to singlify.  E.g. make the variable named
"use_watchlists" and only 'notify' on arg processing.  But that's minor.

http://codereview.chromium.org/118432/diff/7/9
File watchlists.py (right):

http://codereview.chromium.org/118432/diff/7/9#newcode46
Line 46: 
extra blank line

http://codereview.chromium.org/118432/diff/7/9#newcode121
Line 121: def main(argv):
Comment: used to conform watchlists can be parsed

[M-A Ruel, 2009-06-10 18:37:34]

On 2009/06/10 18:23:34, John Grabowski wrote:
> > 2) I think you implementation covers most use case except that it'll fail
with
> > git-cl.
> 
> Can you provide info on how nirnimesh can hook into git-cl?

depot tools now have a git-cl stub. Just execute it and its git repository will
be checkout. Needs cygwin on Windows.

  
> > 3) I have a strange feeling it should be implemented as a presubmit check. 
> > Something like:
> > 
> > def CheckChangeOnUpload(input_api, output_api):
> >   for item in input_api.AbsoluteLocalPaths():
> >     if re.match(r"bleh/.*", item):
> >       return [output_api.AddReviewer('me@chromium.org')]
> >   return []
> > 
> > But it may not be the right place after all. It's just that I want to keep
gcl
> > as small as possible.
> 
> I understand the need to keep gcl small.  There are other potential places to
> hook in (e.g. in rietveld itself) that we were warned against.  
> 
> It is unclear to me if presubmit is better than inside the 'gcl upload'
command.
>  Logically I would think no since presubmit checks the code whereas upload
deals
> with email et al.  But I am open to ideas.

Not necessarily "better", just "no need to write the same code 2 times".

Presubmit checks many things like the moon phase. :) It's already run on gcl
upload unless --no_presubmit is given.

No strong opinion but I feel that it'll fail in some circumstances; git svn
dcommit, svn commit, raw diff upload to rietveld (unlikely). I guess it's a
non-goal but makes the whole idea moot to me (hence my initial objection).

Why implement the --no_watchlists flag? It seems unnecessary to me.

http://codereview.chromium.org/118432/diff/7/9
File watchlists.py (right):

http://codereview.chromium.org/118432/diff/7/9#newcode46
Line 46: 
unneeded vertical space

http://codereview.chromium.org/118432/diff/7/9#newcode66
Line 66: repo_root: root to respository
repository. I feel this is self-descriptive code and doesn't warrant a comment
at all. At worst, rename the argument 'repository_root' so you'll be set.

http://codereview.chromium.org/118432/diff/7/9#newcode94
Line 94: self._defns, self._watchlists = defns, watchlists
I don't see why the two statements are concatenated in one line.

http://codereview.chromium.org/118432/diff/7/9#newcode117
Line 117: [watchers.add(x) for x in self._watchlists[name]]
The issue with this line is that you create a temporary list for nothing and I
usually don't expect side-effect from this form of statement.

[Randall Spangler, 2009-06-10 19:55:49]

http://codereview.chromium.org/118432/diff/7/10
File gcl.py (right):

http://codereview.chromium.org/118432/diff/7/10#newcode791
Line 791: cc_list += ',' + ','.join(watchers)
Does this work if cc_list is blank?

What about:

cc_list = ','.join([cc_list] + watchers)

http://codereview.chromium.org/118432/diff/7/9
File watchlists.py (right):

http://codereview.chromium.org/118432/diff/7/9#newcode97
Line 97: for name in watchlists.keys():
.keys() is redundant.  Use:

   for name in watchlists:

http://codereview.chromium.org/118432/diff/7/9#newcode98
Line 98: if name not in defns.keys():
(here too)

[Nirnimesh, 2009-06-11 19:27:24]

>> 1) Could you rename watchlist_rules.py to WATCHLIST to be clear it's not
actually a source file or meant to be executed.

Done. Converted to "WATCHLISTS" -- now loaded like *.gyp

>> 2) I think you implementation covers most use case except that it'll fail
with git-cl.

Can I worry about git-cl after I check this in?

>> 3) I have a strange feeling it should be implemented as a presubmit check. 

I would side with John on this. I agree that it could be implemented in the
presubmit check and I did consider this, but it just didn't seem like the right
place to me. In my opinion it's closer to gcl & codereview rather than
presubmit.

I've addressed all the other concerns.
Thanks

http://codereview.chromium.org/118432/diff/7/10
File gcl.py (right):

http://codereview.chromium.org/118432/diff/7/10#newcode785
Line 785: if not no_watchlists:
On 2009/06/10 18:36:19, John Grabowski wrote:
> double negative... might be noce to singlify.  E.g. make the variable named
> "use_watchlists" and only 'notify' on arg processing.  But that's minor.
> 
--no_watchlists falls in line with the other args like --no_try, --no_presubmit,
etc.
Also, it makes sense for the user to use --no_watchlists to avoid watchlists.

http://codereview.chromium.org/118432/diff/7/10#newcode791
Line 791: cc_list += ',' + ','.join(watchers)
On 2009/06/10 19:55:50, Randall Spangler wrote:
> Does this work if cc_list is blank?
> 
> What about:
> 
> cc_list = ','.join([cc_list] + watchers)

If cc_list is empty (''), the above will still be prefixed by a ','
Changed to filter out empty elements.

http://codereview.chromium.org/118432/diff/7/9
File watchlists.py (right):

http://codereview.chromium.org/118432/diff/7/9#newcode46
Line 46: 
On 2009/06/10 18:36:19, John Grabowski wrote:
> extra blank line

Done.

http://codereview.chromium.org/118432/diff/7/9#newcode46
Line 46: 
On 2009/06/10 18:37:34, Marc-Antoine Ruel wrote:
> unneeded vertical space

Done.

http://codereview.chromium.org/118432/diff/7/9#newcode66
Line 66: repo_root: root to respository
On 2009/06/10 18:37:34, Marc-Antoine Ruel wrote:
> repository. I feel this is self-descriptive code and doesn't warrant a comment
> at all. At worst, rename the argument 'repository_root' so you'll be set.

Done.

http://codereview.chromium.org/118432/diff/7/9#newcode94
Line 94: self._defns, self._watchlists = defns, watchlists
On 2009/06/10 18:37:34, Marc-Antoine Ruel wrote:
> I don't see why the two statements are concatenated in one line.

Done.

http://codereview.chromium.org/118432/diff/7/9#newcode97
Line 97: for name in watchlists.keys():
On 2009/06/10 19:55:50, Randall Spangler wrote:
> .keys() is redundant.  Use:
> 
>    for name in watchlists:

Done.

http://codereview.chromium.org/118432/diff/7/9#newcode98
Line 98: if name not in defns.keys():
On 2009/06/10 19:55:50, Randall Spangler wrote:
> (here too)

Done.

http://codereview.chromium.org/118432/diff/7/9#newcode117
Line 117: [watchers.add(x) for x in self._watchlists[name]]
On 2009/06/10 18:37:34, Marc-Antoine Ruel wrote:
> The issue with this line is that you create a temporary list for nothing and I
> usually don't expect side-effect from this form of statement.

I did this mostly to save lines. A for block would take 2 lines.

Converted to map

http://codereview.chromium.org/118432/diff/7/9#newcode121
Line 121: def main(argv):
On 2009/06/10 18:36:19, John Grabowski wrote:
> Comment: used to conform watchlists can be parsed

Done.

[M-A Ruel, 2009-06-11 19:32:20]

lgtm with nits.

Without a unit test, there's no guarantee it'll still work in a year. (Not that
gcl has good coverage...)

http://codereview.chromium.org/118432/diff/2001/2004
File gcl.py (right):

http://codereview.chromium.org/118432/diff/2001/2004#newcode745
Line 745: import watchlists
You can defer the import at line 786

http://codereview.chromium.org/118432/diff/2001/2004#newcode792
Line 792: cc_list = ','.join(map(lambda x: x, [cc_list] + watchers))
Note that filter(None, [cc_list] + watchers) would work too instead of map(...).

[Randall Spangler, 2009-06-11 20:55:03]

lgtm

[Nirnimesh, 2009-06-11 21:05:14]

http://codereview.chromium.org/118432/diff/2001/2004
File gcl.py (right):

http://codereview.chromium.org/118432/diff/2001/2004#newcode745
Line 745: import watchlists
On 2009/06/11 19:32:21, Marc-Antoine Ruel wrote:
> You can defer the import at line 786

Done.

http://codereview.chromium.org/118432/diff/2001/2004#newcode792
Line 792: cc_list = ','.join(map(lambda x: x, [cc_list] + watchers))
On 2009/06/11 19:32:21, Marc-Antoine Ruel wrote:
> Note that filter(None, [cc_list] + watchers) would work too instead of
map(...).

Done.

[M-A Ruel, 2009-06-11 21:15:17]

On 2009/06/11 21:05:14, Nirnimesh wrote:
> http://codereview.chromium.org/118432/diff/2001/2004
> File gcl.py (right):
> 
> http://codereview.chromium.org/118432/diff/2001/2004#newcode745
> Line 745: import watchlists
> On 2009/06/11 19:32:21, Marc-Antoine Ruel wrote:
> > You can defer the import at line 786
> 
> Done.
> 
> http://codereview.chromium.org/118432/diff/2001/2004#newcode792
> Line 792: cc_list = ','.join(map(lambda x: x, [cc_list] + watchers))
> On 2009/06/11 19:32:21, Marc-Antoine Ruel wrote:
> > Note that filter(None, [cc_list] + watchers) would work too instead of
> map(...).
> 
> Done.

you already have my lgtm