update git-cl for OWNERS file support via .git/hooks/pre-cl-*

UPDATE: closing this for now to reduce confusion, I'm assuming we'll go the http://codereview.chromium.org/6674014/ route and remove the extra hooks step.


update git-cl for changes to hook output needed to support OWNERS files and to propagate --tbr and --host-url (the host+scheme+port of the rietveld server to use) into presubmit_support and the presubmit hooks.

This patch propagates all of the data through the .git/hooks/pre-cl-* mechanism. However, it has not addressed how to ensure that the hooks are updated properly so it is not safe to land as-is.

We do not need both this patch and http://codereview.chromium.org/6674014/.

[Dirk Pranke, 2011-03-09 02:29:14]

This was split off from http://codereview.chromium.org/6623074/ .

There are no changes since version that was reviewed there. 

It still needs unit tests (coming up next).

[Dirk Pranke, 2011-03-11 00:03:16]

Okay, this change now depends on http://codereview.chromium.org/6657028/ to
really work, but it should handle the OWNERS checks as well as propagating the
Rietveld server address through and handle the --tbr flag from git-cl properly.

[Dirk Pranke, 2011-03-11 00:22:44]

On 2011/03/11 00:03:16, dpranke wrote:
> Okay, this change now depends on http://codereview.chromium.org/6657028/ to
> really work, but it should handle the OWNERS checks as well as propagating the
> Rietveld server address through and handle the --tbr flag from git-cl
properly.

Running the git-cl PRESUBMIT hooks on this warns me to not forget to "fix git-cl
to download the newest version of git-cl-upload-hook". I'm a bit worried about
what this might entail, as I can imagine it but find no obvious code to support
it in the logs. Help?

-- Dirk

[M-A Ruel, 2011-03-11 00:48:41]

On 2011/03/11 00:22:44, dpranke wrote:
> Running the git-cl PRESUBMIT hooks on this warns me to not forget to "fix
git-cl
> to download the newest version of git-cl-upload-hook". I'm a bit worried about
> what this might entail, as I can imagine it but find no obvious code to
support
> it in the logs. Help?

Right now, once the hook has been copied into .git/hooks/, it's never updated.
It means your change would not be propagated automatically to existing
checkouts. I don't have a great answer to this except fixing the code to update
it automatically (?) I must say I'm not 100% happy with the way hooks are set up
since it's hard to pass data from and to the hook, like how you are processing
the stdout for directives.

http://codereview.chromium.org/6646009/diff/10006/git-cl-upload-hook
File git-cl-upload-hook (right):

http://codereview.chromium.org/6646009/diff/10006/git-cl-upload-hook#newcode50
git-cl-upload-hook:50: parser.add_option('--host-url', default=None,
Actually, it is safe to get this value from git config since it's always the
reference value.

http://codereview.chromium.org/6646009/diff/10006/git_cl_hooks.py
File git_cl_hooks.py (right):

http://codereview.chromium.org/6646009/diff/10006/git_cl_hooks.py#newcode35
git_cl_hooks.py:35: class ChangeOptions:
While at it make it a new style class:
class ChangeOptions(object):

http://codereview.chromium.org/6646009/diff/10006/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6646009/diff/10006/presubmit_support.py#newcod...
presubmit_support.py:246: self.host_url = host_url or
'http://codereview.chromium.org'
Bad idea. You don't want reviews on the private instance to be silently
redirected to the public one.

[Dirk Pranke, 2011-03-11 01:21:48]

On 2011/03/11 00:48:41, Marc-Antoine Ruel wrote:
> On 2011/03/11 00:22:44, dpranke wrote:
> > Running the git-cl PRESUBMIT hooks on this warns me to not forget to "fix
> git-cl
> > to download the newest version of git-cl-upload-hook". I'm a bit worried
about
> > what this might entail, as I can imagine it but find no obvious code to
> support
> > it in the logs. Help?
> 
> Right now, once the hook has been copied into .git/hooks/, it's never updated.
> It means your change would not be propagated automatically to existing
> checkouts. I don't have a great answer to this except fixing the code to
update
> it automatically (?) I must say I'm not 100% happy with the way hooks are set
up
> since it's hard to pass data from and to the hook, like how you are processing
> the stdout for directives.
> 

Okay, per the other note I sent you and Evan, it looks to me like we can
probably get rid of git-cl-upload-hook completely. Anyone object? I'll see if I
can do that as a separate patch first.

I agree that the hooks could be more flexible, but I don't particularly want to
try and change that as a part of this patch ;)

> http://codereview.chromium.org/6646009/diff/10006/git-cl-upload-hook
> File git-cl-upload-hook (right):
> 
> http://codereview.chromium.org/6646009/diff/10006/git-cl-upload-hook#newcode50
> git-cl-upload-hook:50: parser.add_option('--host-url', default=None,
> Actually, it is safe to get this value from git config since it's always the
> reference value.
> 

git_cl.py jumps through a lot of hoops to read the "correct" setting for this. I
didn't want to duplicate that logic on the other side of the process boundary
and risk getting it wrong.

> http://codereview.chromium.org/6646009/diff/10006/git_cl_hooks.py
> File git_cl_hooks.py (right):
> 
> http://codereview.chromium.org/6646009/diff/10006/git_cl_hooks.py#newcode35
> git_cl_hooks.py:35: class ChangeOptions:
> While at it make it a new style class:
> class ChangeOptions(object):
> 

Will do.

> http://codereview.chromium.org/6646009/diff/10006/presubmit_support.py
> File presubmit_support.py (right):
> 
>
http://codereview.chromium.org/6646009/diff/10006/presubmit_support.py#newcod...
> presubmit_support.py:246: self.host_url = host_url or
> 'http://codereview.chromium.org'
> Bad idea. You don't want reviews on the private instance to be silently
> redirected to the public one.

I'm not sure which things you're worried about. The reason this code is here is
to get over the hump of the calling client not being updated; otherwise I'd have
to modify presubmit_support, git_cl, gcl, and everything else all at once.

So, if this patch lands and no changes to gcl land, then gcl will default to
using codereview.chromium.org always, which I think is probably safe and right.

git-cl will always pass in the the instance it wants to use, so the only way it
could get out of sync would be if the user changed settings (which I can't
easily prevent, and probably isn't worth worrying about).

[Dirk Pranke, 2011-03-11 01:32:54]

On Thu, Mar 10, 2011 at 5:21 PM,  <dpranke@chromium.org> wrote:
> On 2011/03/11 00:48:41, Marc-Antoine Ruel wrote:
>>
>> On 2011/03/11 00:22:44, dpranke wrote:
>> > Running the git-cl PRESUBMIT hooks on this warns me to not forget to
>> > "fix
>> git-cl
>> > to download the newest version of git-cl-upload-hook". I'm a bit worried
>
> about
>>
>> > what this might entail, as I can imagine it but find no obvious code to
>> support
>> > it in the logs. Help?
>
>> Right now, once the hook has been copied into .git/hooks/, it's never
>> updated.
>> It means your change would not be propagated automatically to existing
>> checkouts. I don't have a great answer to this except fixing the code to
>
> update
>>
>> it automatically (?) I must say I'm not 100% happy with the way hooks are
>> set
>
> up
>>
>> since it's hard to pass data from and to the hook, like how you are
>> processing
>> the stdout for directives.
>
>
> Okay, per the other note I sent you and Evan, it looks to me like we can
> probably get rid of git-cl-upload-hook completely. Anyone object? I'll see
> if I
> can do that as a separate patch first.
>
> I agree that the hooks could be more flexible, but I don't particularly want
> to
> try and change that as a part of this patch ;)
>
>> http://codereview.chromium.org/6646009/diff/10006/git-cl-upload-hook
>> File git-cl-upload-hook (right):
>
>>
>>
http://codereview.chromium.org/6646009/diff/10006/git-cl-upload-hook#newcode50
>> git-cl-upload-hook:50: parser.add_option('--host-url', default=None,
>> Actually, it is safe to get this value from git config since it's always
>> the
>> reference value.
>
>
> git_cl.py jumps through a lot of hoops to read the "correct" setting for
> this. I
> didn't want to duplicate that logic on the other side of the process
> boundary
> and risk getting it wrong.
>
>> http://codereview.chromium.org/6646009/diff/10006/git_cl_hooks.py
>> File git_cl_hooks.py (right):
>
>>
>> http://codereview.chromium.org/6646009/diff/10006/git_cl_hooks.py#newcode35
>> git_cl_hooks.py:35: class ChangeOptions:
>> While at it make it a new style class:
>> class ChangeOptions(object):
>
>
> Will do.
>
>> http://codereview.chromium.org/6646009/diff/10006/presubmit_support.py
>> File presubmit_support.py (right):
>
>
>
http://codereview.chromium.org/6646009/diff/10006/presubmit_support.py#newcod...
>>
>> presubmit_support.py:246: self.host_url = host_url or
>> 'http://codereview.chromium.org'
>> Bad idea. You don't want reviews on the private instance to be silently
>> redirected to the public one.
>
> I'm not sure which things you're worried about. The reason this code is here
> is
> to get over the hump of the calling client not being updated; otherwise I'd
> have
> to modify presubmit_support, git_cl, gcl, and everything else all at once.
>
> So, if this patch lands and no changes to gcl land, then gcl will default to
> using codereview.chromium.org always, which I think is probably safe and
> right.
>
> git-cl will always pass in the the instance it wants to use, so the only way
> it
> could get out of sync would be if the user changed settings (which I can't
> easily prevent, and probably isn't worth worrying about).
>

I should have also mentioned that this setting won't do anything until the hooks
start looking at it. So, until CheckOwners is enabled, it's harmless. I hope to
remove the default value in a later patch that will happen before this.

This patch is already pretty big; I'd like to stop pulling stuff into
it, if that's okay.

I'm open to other suggestions for how to do this, though.

-- Dirk

[Dirk Pranke, 2011-03-11 04:33:46]

Okay, this set of changes is finally read to be landed, if we decide we want
things to work this way.

I will produce a different patch for this that has the git-cl-upload-hook logic
removed, so we can compare.

http://codereview.chromium.org/6646009/diff/8008/presubmit_support.py
File presubmit_support.py (left):

http://codereview.chromium.org/6646009/diff/8008/presubmit_support.py#oldcode159
presubmit_support.py:159: return True
I had to remove the IsMessage() check, because it turns out that I need the
"ADD: " lines to be echoed into the output, of course. DoPresubmitHooks() has no
way of knowing whether suppressing it is the right thing to do. We'll just have
to live with some noise in the output for now, but I think (not positive) that
this is okay.

http://codereview.chromium.org/6646009/diff/8008/presubmit_support.py#oldcode...
presubmit_support.py:1067: if not item.IsMessage():
See comment above.

[Dirk Pranke, 2011-03-11 21:09:48]

closing this issue for now ... I'll assume we'll go the other route and remove
the extra hooks step.