Remove support for generic "hook files" in git-cl, require depot_tools

Remove support for generic "hook files" in git-cl, and just call into
the git_cl_hooks library in depot_tools. Also remove support for
post-commit hooks. This binds git-cl tighter to depot_tools, but also
makes things a lot simpler and cleaner.

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

[Dirk Pranke, 2011-03-11 02:51:40]

Looks like everything gets easier w/o the standalone hooks files. 

WDYT?

[M-A Ruel, 2011-03-11 14:42:52]

http://codereview.chromium.org/6665018/diff/8/git-cl-upload-hook
File git-cl-upload-hook (left):

http://codereview.chromium.org/6665018/diff/8/git-cl-upload-hook#oldcode1
git-cl-upload-hook:1: #!/usr/bin/python
You need to delete this file later, as older clients will still try to fetch it.

http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py#newcode24
git_cl/git_cl.py:24: # TODO(dpranke): We wrap this in a try block for a limited
form of
My take is to move git_cl up a directory first and them do more severe hacking
on it. But I don't mind, do as you prefer.

http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py#newcode666
git_cl/git_cl.py:666: def ConvertToInteger(inputval):
(random idea)
def safe_convert(inputval, outputtype):
try:
  return outputtype(inputval):
...

:D
But I don't mind.

http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py#newcode712
git_cl/git_cl.py:712: return presubmit_support.DoPresubmitChecks(change,
committing,
It changes from running out of process to in-process. There is known issues I
want to get rid of. :/

But running out of process is a matter of integration inside presubmit_support
than git-cl or gcl. It's just sad to have a temporary regression on that side in
the meantime.

http://codereview.chromium.org/6665018/diff/8/git_cl_hooks.py
File git_cl_hooks.py (left):

http://codereview.chromium.org/6665018/diff/8/git_cl_hooks.py#oldcode1
git_cl_hooks.py:1: # Copyright (c) 2010 The Chromium Authors. All rights
reserved.
Also need o not be removed right away.

[Dirk Pranke, 2011-03-11 20:58:54]

On 2011/03/11 14:42:52, Marc-Antoine Ruel wrote:
> http://codereview.chromium.org/6665018/diff/8/git-cl-upload-hook
> File git-cl-upload-hook (left):
> 
> http://codereview.chromium.org/6665018/diff/8/git-cl-upload-hook#oldcode1
> git-cl-upload-hook:1: #!/usr/bin/python
> You need to delete this file later, as older clients will still try to fetch
it.
> 

It seems like older clients will only try to fetch it once per-repo, though,
right? So it would likely only be an issue if someone has downloaded depot_tools
but not updated, then checks out a new repo using git, and then for some reason
git-cl fails to self-update to a new version prior to actually running in the
new repo?

That said, there's little harm in leaving it around for now, so I will.

> http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py
> File git_cl/git_cl.py (right):
> 
> http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py#newcode24
> git_cl/git_cl.py:24: # TODO(dpranke): We wrap this in a try block for a
limited
> form of
> My take is to move git_cl up a directory first and them do more severe hacking
> on it. But I don't mind, do as you prefer.

I want to be minimally invasive until I can get the OWNERS code done. We can
clean up more later.

> 
> http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py#newcode666
> git_cl/git_cl.py:666: def ConvertToInteger(inputval):
> (random idea)
> def safe_convert(inputval, outputtype):
> try:
>   return outputtype(inputval):
> ...
> 
> :D
> But I don't mind.
> 

Will leave as is for now.

> http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py#newcode712
> git_cl/git_cl.py:712: return presubmit_support.DoPresubmitChecks(change,
> committing,
> It changes from running out of process to in-process. There is known issues I
> want to get rid of. :/
> 

I'm not sure I follow you here. I think you're saying that this is okay for now,
but there are things we should fix at some point. Is that right? I'd be curious
to know what those things are ...

> But running out of process is a matter of integration inside presubmit_support
> than git-cl or gcl. It's just sad to have a temporary regression on that side
in
> the meantime.
> 
> http://codereview.chromium.org/6665018/diff/8/git_cl_hooks.py
> File git_cl_hooks.py (left):
> 
> http://codereview.chromium.org/6665018/diff/8/git_cl_hooks.py#oldcode1
> git_cl_hooks.py:1: # Copyright (c) 2010 The Chromium Authors. All rights
> reserved.
> Also need o not be removed right away.

Will leave around for now as per above.

[M-A Ruel, 2011-03-11 21:15:17]

On 2011/03/11 20:58:54, dpranke wrote:
> On 2011/03/11 14:42:52, Marc-Antoine Ruel wrote:
> > git-cl-upload-hook:1: #!/usr/bin/python
> > You need to delete this file later, as older clients will still try to fetch
> it.
> > 
> 
> It seems like older clients will only try to fetch it once per-repo, though,
> right? So it would likely only be an issue if someone has downloaded
depot_tools
> but not updated, then checks out a new repo using git, and then for some
reason
> git-cl fails to self-update to a new version prior to actually running in the
> new repo?
> 
> That said, there's little harm in leaving it around for now, so I will.

Yes. So I'd update depot_tools.tar.gz/.zip first before deleting the file.


> > http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py#newcode712
> > git_cl/git_cl.py:712: return presubmit_support.DoPresubmitChecks(change,
> > committing,
> > It changes from running out of process to in-process. There is known issues
I
> > want to get rid of. :/
> > 
> 
> I'm not sure I follow you here. I think you're saying that this is okay for
now,
> but there are things we should fix at some point. Is that right? I'd be
curious
> to know what those things are ...

Some PRESUBMIT.py files have side effects like modifying sys.path or more. So
it's safer to run them out of process, which is not done yet. But it was done
for git-cl hooks because of the design but you're making the hook run inprocess
now. In any case, it's presubmit_support's job to do that.


lgtm but I just want to make sure Anush (or someone else from ChromiumOS) is
fine with the fact that we are removing support for the hooks.

[Dirk Pranke, 2011-03-11 21:23:49]

On Fri, Mar 11, 2011 at 1:15 PM,  <maruel@chromium.org> wrote:
> On 2011/03/11 20:58:54, dpranke wrote:
>>
>> On 2011/03/11 14:42:52, Marc-Antoine Ruel wrote:
>> > git-cl-upload-hook:1: #!/usr/bin/python
>> > You need to delete this file later, as older clients will still try to
>> > fetch
>> it.
>> >
>
>> It seems like older clients will only try to fetch it once per-repo,
>> though,
>> right? So it would likely only be an issue if someone has downloaded
>
> depot_tools
>>
>> but not updated, then checks out a new repo using git, and then for some
>
> reason
>>
>> git-cl fails to self-update to a new version prior to actually running in
>> the
>> new repo?
>
>> That said, there's little harm in leaving it around for now, so I will.
>
> Yes. So I'd update depot_tools.tar.gz/.zip first before deleting the file.
>

Good point. I hadn't thought about the tarballs yet.

>
>> >
>> > http://codereview.chromium.org/6665018/diff/8/git_cl/git_cl.py#newcode712
>> > git_cl/git_cl.py:712: return presubmit_support.DoPresubmitChecks(change,
>> > committing,
>> > It changes from running out of process to in-process. There is known
>> > issues
>
> I
>>
>> > want to get rid of. :/
>> >
>
>> I'm not sure I follow you here. I think you're saying that this is okay
>> for
>
> now,
>>
>> but there are things we should fix at some point. Is that right? I'd be
>
> curious
>>
>> to know what those things are ...
>
> Some PRESUBMIT.py files have side effects like modifying sys.path or more.
> So
> it's safer to run them out of process, which is not done yet. But it was
> done
> for git-cl hooks because of the design but you're making the hook run
> inprocess
> now. In any case, it's presubmit_support's job to do that.
>

Hm. Yeah, I had been thinking of something along those lines. I'll
keep mulling it over myself
as well. I agree that it's better enforced under presubmit_support.

-- Dirk

>
> lgtm but I just want to make sure Anush (or someone else from ChromiumOS) is
> fine with the fact that we are removing support for the hooks.
>
> http://codereview.chromium.org/6665018/
>

[Mandeep Singh Baines, 2011-03-11 23:04:01]

LGTM

[chase, 2011-03-14 21:05:47]

lgtm with some nits/notes

Thanks for making the change, today I was looking at making this simpler after
the outage last week and you did it already in a way that's as straightforward
as it gets. :)  Strange to go from supporting separate hook files in git-cl to
moving away from them completely.  If you're interested,
http://code.google.com/p/chromium/issues/detail?id=5339 for the original
history.

http://codereview.chromium.org/6665018/diff/7003/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/6665018/diff/7003/git_cl/git_cl.py#newcode682
git_cl/git_cl.py:682: root = "."
my original lines in git_cl_hooks.py should have used ', feel free to fix if you
want

http://codereview.chromium.org/6665018/diff/7003/git_cl/git_cl.py#newcode685
git_cl/git_cl.py:685: raise Exception("Could not get root directory.")
my original lines in git_cl_hooks.py should have used ', feel free to fix if you
want

http://codereview.chromium.org/6665018/diff/7003/git_cl/git_cl.py#newcode701
git_cl/git_cl.py:701: '%s...' % (upstream_branch)]).strip()
indent one char

http://codereview.chromium.org/6665018/diff/7003/git_cl/git_cl.py#newcode711
git_cl/git_cl.py:711: 'rietveld.extracc', ','.join(watchers)])
indent one char

http://codereview.chromium.org/6665018/diff/7003/git_cl/git_cl.py#newcode938
git_cl/git_cl.py:938: RunHook(committing=False, upstream_branch=base_branch)
<strike>committing=True to match PREDCOMMIT_HOOK</strike>
looks like you fixed this in r77898, thanks