Add watchlist support to git-cl.

Add watchlist support to git-cl. 
Add test for said support. 
Update some git-gl tests to work on OSX
(hopefully without breaking the tests for Linux).

[John Grabowski, 2009-12-02 01:32:50]

rietveld seems a little unhappy at the moment; you might have to "view raw patch
set" to see all the chaneges, since some links come up empty.  (The "View" links
in particular seem problematic.)

[Paweł Hajdan Jr., 2009-12-02 07:14:43]

I'd prefer that --no-hooks doesn't disable watchlists, and it is possible in
this patch, as watchlists are implemented in git-cl and not as a presubmit hook.
However, I remember Evan saying it's preferred to do it as a presubmit hook
instead.

For me, LGTM.

[M-A Ruel, 2009-12-02 15:43:04]

I have no opinion, as long as Evan and Mandeep are fine with the patch.

[John Grabowski, 2009-12-02 18:49:04]

Moved condition so disabling hooks does not disable watchlists; new diffs
up.

One problem with making watchlist processing a presubmit hook is that it's
run in a subprocess.  Thus I can't easily modify the cc list of git-cl, it's
parent process.  This need for non-boolean feedback (e.g. more than "did you
hit the try server") is unique for the hooks.

There are, or course, alternatives.  For example, I could add a
branch.<branchname>.watchlist_cc "git config" property, write to it in a
hook, then read from it in git-cl.  I tried to keep things simple but am
open to change if Evan (or anyone) really thinks this should be a hook.

jrg


On Tue, Dec 1, 2009 at 11:14 PM, <phajdan.jr@chromium.org> wrote:

> I'd prefer that --no-hooks doesn't disable watchlists, and it is possible
> in
> this patch, as watchlists are implemented in git-cl and not as a presubmit
> hook.
> However, I remember Evan saying it's preferred to do it as a presubmit hook
> instead.
>
> For me, LGTM.
>
>
> http://codereview.chromium.org/461001
>

[Paweł Hajdan Jr., 2009-12-02 18:57:28]

After fixing the nit, LGTM!

http://codereview.chromium.org/461001/diff/5001/2008
File git-cl (right):

http://codereview.chromium.org/461001/diff/5001/2008#newcode58
git-cl:58: self.watchlist_cc = None  # str; comma seperated string of email
addrs
nit: Shouldn't comments be proper sentences, without such abbreviations?

[Evan Martin, 2009-12-02 19:04:54]

Historically I've been against adding this to git-cl since it is used by
projects that are not Chrome.  However, I am kind of exhausted about arguing
about it so I can relent.  But I'll ask it anyway: is it not possible to hook
this into the Chrome repo's precommit stuff?

[John Grabowski, 2009-12-02 20:47:44]

We want this to be part of the upload stage, not the commit stage, so that
watchers see all the CL comments go by and (in theory) have a chance to
comment themselves.

Can you clarify your question?  E.g. are you suggesting I mod this into a
pre-cl-upload hook?

jrg

On Wed, Dec 2, 2009 at 11:04 AM, <evan@chromium.org> wrote:

> Historically I've been against adding this to git-cl since it is used by
> projects that are not Chrome.  However, I am kind of exhausted about
> arguing
> about it so I can relent.  But I'll ask it anyway: is it not possible to
> hook
> this into the Chrome repo's precommit stuff?
>
>
> http://codereview.chromium.org/461001
>

[M-A Ruel, 2009-12-02 20:53:17]

On 2009/12/02 20:47:44, John Grabowski wrote:
> We want this to be part of the upload stage, not the commit stage, so that
> watchers see all the CL comments go by and (in theory) have a chance to
> comment themselves.
> 
> Can you clarify your question?  E.g. are you suggesting I mod this into a
> pre-cl-upload hook?

I don't think anyone use --bypass-hooks for git cl upload. At worst, it could be
implemented the same way try server selection is done with a different
PRESUBMIT.py function. See trychange.py.

[John Grabowski, 2009-12-02 21:06:36]

trychange.py is imported directly by gcl.  This is very much like how
watchlists.py is directly imported by git-cl.  So I'm not sure what you are
suggesting here, unless you are refering to how GetCodeReviewSetting() uses
a data file.

Evan, can you clarify what I can do to this CL to make it more acceptable to
you?  I'd much rather get an <3 than an "I am exhausted".

jrg

On Wed, Dec 2, 2009 at 12:53 PM, <maruel@chromium.org> wrote:

> On 2009/12/02 20:47:44, John Grabowski wrote:
>
>> We want this to be part of the upload stage, not the commit stage, so that
>> watchers see all the CL comments go by and (in theory) have a chance to
>> comment themselves.
>>
>
>  Can you clarify your question?  E.g. are you suggesting I mod this into a
>> pre-cl-upload hook?
>>
>
> I don't think anyone use --bypass-hooks for git cl upload. At worst, it
> could be
> implemented the same way try server selection is done with a different
> PRESUBMIT.py function. See trychange.py.
>
>
> http://codereview.chromium.org/461001
>

[chase, 2009-12-02 21:06:40]

On 2009/12/02 20:47:44, John Grabowski wrote:
> We want this to be part of the upload stage, not the commit stage, so that
> watchers see all the CL comments go by and (in theory) have a chance to
> comment themselves.
> 
> Can you clarify your question?  E.g. are you suggesting I mod this into a
> pre-cl-upload hook?

Easier to modify git_cl_hooks.py to support this than editing our user's
pre-cl-upload hook file directly.

As to whether it should be a hook, if it's Chromium-specific, it seems right for
it to be in the Chromium hooks.  Presubmit checks were Chromium-specific and we
went without them in git-cl until someone (me!) wrote git-cl hook support.  So
I'm definitely biased, but I'm also definitely considering where we're heading.

Otherwise, if we make more local changes to our copy of git-cl, we'll continue
to deviate from upstream. (At least I care that we don't do that, am I the only
one?)

[John Grabowski, 2009-12-02 23:28:48]

Simplified git-cl change based on feedback; most of the guts moved into a hook.
Also see http://codereview.chromium.org/463010

[M-A Ruel, 2009-12-03 00:27:05]

Can you explain the difference between rietveld.cc and branch.master.cc?

[John Grabowski, 2009-12-03 00:58:15]

rietveld.cc comes from http://src.chromium.org/svn/codereview.settings and
is fixed at tree creation time.  It does not change.  branch.master.cc is
specific to this CL (branch) and changes frequently.  We don't want the
latter to stomp on the former, ever.

jrg

On Wed, Dec 2, 2009 at 4:27 PM, <maruel@chromium.org> wrote:

> Can you explain the difference between rietveld.cc and branch.master.cc?
>
>
> http://codereview.chromium.org/461001
>

[M-A Ruel, 2009-12-03 01:30:47]

This change is now fairly trivial so lgtm to me. I guess it should be fine to
Evan now.

[Evan Martin, 2009-12-03 01:45:13]

Doesn't branch.master.* get deleted if you delete your master branch?  I don't
exactly understand this.

[John Grabowski, 2009-12-03 01:49:59]

I don't know; I am new to git.  Do you have an idea for a better config
name?

jrg

On Wed, Dec 2, 2009 at 5:45 PM, <evan@chromium.org> wrote:

> Doesn't branch.master.* get deleted if you delete your master branch?  I
> don't
> exactly understand this.
>
>
> http://codereview.chromium.org/461001
>

[John Grabowski, 2009-12-03 02:45:18]

Final data name negotiated with Evan.

[Evan Martin, 2009-12-03 20:15:01]

thanks, all in

[Mandeep Singh Baines, 2009-12-04 19:28:16]

http://codereview.chromium.org/461001/diff/3005/6004
File git-cl (right):

http://codereview.chromium.org/461001/diff/3005/6004#newcode82
git-cl:82: more_cc = self._GetConfig('rietveld.extra_cc', error_ok=True)
What is the use case for rietveld.extra_cc? I'm guessing its to setup a CC_LIST
specific to the current issue which you want to persist for the duration of the
issue. In that case, you'd want to store extra_cc in the branch config. See
_IssueSetting() for an example.

Here, we are storing extra_cc as a repo config which persists across branches.
Not sure why someone would want to do that.

[John Grabowski, 2009-12-04 23:18:18]

The use case is a value dumped in for cc by the upload hook processing a
watchlist.  The mirror side of this change is in depot_tools;
http://codereview.chromium.org/460038

Watchlists are defined in
http://sites.google.com/a/chromium.org/dev/developers/contributing-code/watch...
.

Since hooks are run as a subprocess we need a way to save the watchlist cc
list and read it later.  This variable is the chosen spot.

jrg


On Fri, Dec 4, 2009 at 11:28 AM, <msb@chromium.org> wrote:

>
> http://codereview.chromium.org/461001/diff/3005/6004
> File git-cl (right):
>
> http://codereview.chromium.org/461001/diff/3005/6004#newcode82
> git-cl:82: more_cc = self._GetConfig('rietveld.extra_cc', error_ok=True)
> What is the use case for rietveld.extra_cc? I'm guessing its to setup a
> CC_LIST specific to the current issue which you want to persist for the
> duration of the issue. In that case, you'd want to store extra_cc in the
> branch config. See _IssueSetting() for an example.
>
> Here, we are storing extra_cc as a repo config which persists across
> branches. Not sure why someone would want to do that.
>
>
> http://codereview.chromium.org/461001
>

[chase, 2009-12-07 21:50:16]

Haven't had time to troubleshoot but saw this on my vista system the first time
I ran an updated git-cl to upload a patch today:

error: invalid key: rietveld.extra_cc

My guess is some code path is trying to read the config key when it's not yet
been written.  'git config -l' in my git checkout after the git-cl upload
completed doesn't show a rietveld.extra_cc config entry.