git_cl_hooks: get cl fields vi git_cl accessors instead of using backquote

git_cl_hooks: get cl fields vi git_cl accessors instead of using backquote

This is part 2 of a 2-part change which fixes a bug where git-cl
hangs if ~/.codereview_upload_cookies does not exist. It is
actually waiting on user input for email/password (but you don't see the
prompt because it's happening in a subprocess).

BUG=none
TEST=Verified that the hang is fixed.

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

[Evan Martin, 2010-10-04 23:09:27]

Why does this change fix the bug?

[Evan Martin, 2010-10-04 23:19:39]

This looks good to me, but let's see what M-A thinks

http://codereview.chromium.org/3622002/diff/1/3
File git_cl_repo (right):

http://codereview.chromium.org/3622002/diff/1/3#newcode1
git_cl_repo:1: git-cl-repo
I don't think you meant to include this -- it's normally a subdirectory, right?

[Mandeep Singh Baines, 2010-10-04 23:24:30]

On Mon, Oct 4, 2010 at 4:09 PM,  <evan@chromium.org> wrote:
> Why does this change fix the bug?
>

GetEmail is no longer being called from a subprocess. Previously, the hook
code would try to run git-cl in subprocess and you'd get hung because
of the email prompt which you never saw because its going into the same
pipe as the output you're trying to capture. Besides, you didn't have access
to stdin anyway.

Now you're calling the accessor directly from the pre-submit which is exec'ed.

> http://codereview.chromium.org/3622002/show
>

[Mandeep Singh Baines, 2010-10-04 23:50:42]

On Mon, Oct 4, 2010 at 4:19 PM,  <evan@chromium.org> wrote:
> This looks good to me, but let's see what M-A thinks
>
>
> http://codereview.chromium.org/3622002/diff/1/3
> File git_cl_repo (right):
>
> http://codereview.chromium.org/3622002/diff/1/3#newcode1
> git_cl_repo:1: git-cl-repo
> I don't think you meant to include this -- it's normally a subdirectory,
> right?
>

In order to import it, I needed to convert the dashes to under-scores.
I couldn't
figure out any other way to import it. I tried putting it in quotes but python
didn't like that either:

from "git-cl-repo" import git_cl

So I created a symlink to workaround. I could avoid the symlink and just
call the directory git_cl_repo but I didn't like that because the tool is
called git-cl and I was worried about potentially breaking anything that
assumed the dir was there but its not a strong dislike.

> http://codereview.chromium.org/3622002/show
>

[Evan Martin, 2010-10-04 23:53:01]

On 2010/10/04 23:50:42, Mandeep Singh Baines wrote:
> So I created a symlink to workaround.

Aw, bummer.  Those don't work on Windows, and we have git users there.

> I could avoid the symlink and just
> call the directory git_cl_repo but I didn't like that because the tool is
> called git-cl and I was worried about potentially breaking anything that
> assumed the dir was there but its not a strong dislike.

Yeah, this might be a better idea.  M-A would know what to do.

[Mandeep Singh Baines, 2010-10-04 23:55:04]

On Mon, Oct 4, 2010 at 4:53 PM,  <evan@chromium.org> wrote:
> On 2010/10/04 23:50:42, Mandeep Singh Baines wrote:
>>
>> So I created a symlink to workaround.
>
> Aw, bummer.  Those don't work on Windows, and we have git users there.
>

D'oh. Forgot windows.

>> I could avoid the symlink and just
>> call the directory git_cl_repo but I didn't like that because the tool is
>> called git-cl and I was worried about potentially breaking anything that
>> assumed the dir was there but its not a strong dislike.
>
> Yeah, this might be a better idea.  M-A would know what to do.
>

Yeah, I'll talk to M-a tomorrow.

> http://codereview.chromium.org/3622002/show
>

[M-A Ruel, 2010-10-04 23:59:30]

What about changing depot_tools/git-cl first to git clone into git_cl_repo first
instead of git-cl-repo and then commit this patch? I'd prefer that to a symlink.
The only side-effect is not a big deal; an old unused directory in
depot_tools/git-cl-repo.

The change itself lgtm.

[Evan Stade, 2010-10-07 22:02:56]

this is causing git cl presubmit to crash. Reverting it fixes the problem. Trace
follows:

$ git cl dcommit
Running presubmit hooks...
Traceback (most recent call last):
  File "/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit", line 52, in
<module>
    exec git_cl_hooks.RunHooks(hook_name=program_name,
upstream_branch=sys.argv[1])
  File "/usr/local/google/chrome/depot_tools/git_cl_hooks.py", line 88, in
RunHooks
    options.may_prompt):
  File "/usr/local/google/chrome/depot_tools/presubmit_support.py", line 967, in
DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/usr/local/google/chrome/depot_tools/presubmit_support.py", line 898, in
ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 117, in CheckChangeOnCommit
  File "/usr/local/google/chrome/depot_tools/presubmit_canned_checks.py", line
455, in CheckRietveldTryJobExecution
    host_url, input_api.change.issue, input_api.change.patchset)
TypeError: %d format: a number is required, not str
Sending crash report ...
  exception: %d format: a number is required, not str
  args: ['/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit',
'refs/remotes/origin/trunk']
  stack:   File "/usr/local/google/chrome/src/.git/hooks/pr
  host: [...]
  user: estade
  cwd: /usr/local/google/chrome/src
A stack trace has been sent to the maintainers.
Command "/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit
refs/remotes/origin/trunk" failed.

[Mandeep Singh Baines, 2010-10-07 23:03:39]

D'oh. Fix coming up.

On Thu, Oct 7, 2010 at 3:02 PM,  <estade@chromium.org> wrote:
> this is causing git cl presubmit to crash. Reverting it fixes the problem.
> Trace
> follows:
>
> $ git cl dcommit
> Running presubmit hooks...
> Traceback (most recent call last):
>  File "/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit", line 52, in
> <module>
>    exec git_cl_hooks.RunHooks(hook_name=program_name,
> upstream_branch=sys.argv[1])
>  File "/usr/local/google/chrome/depot_tools/git_cl_hooks.py", line 88, in
> RunHooks
>    options.may_prompt):
>  File "/usr/local/google/chrome/depot_tools/presubmit_support.py", line 967,
> in
> DoPresubmitChecks
>    results += executer.ExecPresubmitScript(presubmit_script, filename)
>  File "/usr/local/google/chrome/depot_tools/presubmit_support.py", line 898,
> in
> ExecPresubmitScript
>    result = eval(function_name + '(*__args)', context)
>  File "<string>", line 1, in <module>
>  File "<string>", line 117, in CheckChangeOnCommit
>  File "/usr/local/google/chrome/depot_tools/presubmit_canned_checks.py",
> line
> 455, in CheckRietveldTryJobExecution
>    host_url, input_api.change.issue, input_api.change.patchset)
> TypeError: %d format: a number is required, not str
> Sending crash report ...
>  exception: %d format: a number is required, not str
>  args: ['/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit',
> 'refs/remotes/origin/trunk']
>  stack:   File "/usr/local/google/chrome/src/.git/hooks/pr
>  host: [...]
>  user: estade
>  cwd: /usr/local/google/chrome/src
> A stack trace has been sent to the maintainers.
> Command "/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit
> refs/remotes/origin/trunk" failed.
>
>
> http://codereview.chromium.org/3622002/show
>

[willchan no longer on Chromium, 2010-10-07 23:34:19]

It's been broken for at least an hour.  msb, are you reverting?  Note that
people are confused why git cl dcommit is broken and are asking on irc why
this is happening.  The appropriate response for breaking a core utility
like this should be an immediate revert to make sure it works for people
again.

On Thu, Oct 7, 2010 at 4:03 PM, Mandeep Singh Baines <msb@chromium.org>wrote:

> D'oh. Fix coming up.
>
> On Thu, Oct 7, 2010 at 3:02 PM,  <estade@chromium.org> wrote:
> > this is causing git cl presubmit to crash. Reverting it fixes the
> problem.
> > Trace
> > follows:
> >
> > $ git cl dcommit
> > Running presubmit hooks...
> > Traceback (most recent call last):
> >  File "/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit", line 52,
> in
> > <module>
> >    exec git_cl_hooks.RunHooks(hook_name=program_name,
> > upstream_branch=sys.argv[1])
> >  File "/usr/local/google/chrome/depot_tools/git_cl_hooks.py", line 88, in
> > RunHooks
> >    options.may_prompt):
> >  File "/usr/local/google/chrome/depot_tools/presubmit_support.py", line
> 967,
> > in
> > DoPresubmitChecks
> >    results += executer.ExecPresubmitScript(presubmit_script, filename)
> >  File "/usr/local/google/chrome/depot_tools/presubmit_support.py", line
> 898,
> > in
> > ExecPresubmitScript
> >    result = eval(function_name + '(*__args)', context)
> >  File "<string>", line 1, in <module>
> >  File "<string>", line 117, in CheckChangeOnCommit
> >  File "/usr/local/google/chrome/depot_tools/presubmit_canned_checks.py",
> > line
> > 455, in CheckRietveldTryJobExecution
> >    host_url, input_api.change.issue, input_api.change.patchset)
> > TypeError: %d format: a number is required, not str
> > Sending crash report ...
> >  exception: %d format: a number is required, not str
> >  args: ['/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit',
> > 'refs/remotes/origin/trunk']
> >  stack:   File "/usr/local/google/chrome/src/.git/hooks/pr
> >  host: [...]
> >  user: estade
> >  cwd: /usr/local/google/chrome/src
> > A stack trace has been sent to the maintainers.
> > Command "/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit
> > refs/remotes/origin/trunk" failed.
> >
> >
> > http://codereview.chromium.org/3622002/show
> >
>

[Mandeep Singh Baines, 2010-10-07 23:45:24]

Reverted.

On Thu, Oct 7, 2010 at 4:34 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> It's been broken for at least an hour.  msb, are you reverting?  Note that
> people are confused why git cl dcommit is broken and are asking on irc why
> this is happening.  The appropriate response for breaking a core utility
> like this should be an immediate revert to make sure it works for people
> again.
>
> On Thu, Oct 7, 2010 at 4:03 PM, Mandeep Singh Baines <msb@chromium.org>
> wrote:
>>
>> D'oh. Fix coming up.
>>
>> On Thu, Oct 7, 2010 at 3:02 PM,  <estade@chromium.org> wrote:
>> > this is causing git cl presubmit to crash. Reverting it fixes the
>> > problem.
>> > Trace
>> > follows:
>> >
>> > $ git cl dcommit
>> > Running presubmit hooks...
>> > Traceback (most recent call last):
>> >  File "/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit", line 52,
>> > in
>> > <module>
>> >    exec git_cl_hooks.RunHooks(hook_name=program_name,
>> > upstream_branch=sys.argv[1])
>> >  File "/usr/local/google/chrome/depot_tools/git_cl_hooks.py", line 88,
>> > in
>> > RunHooks
>> >    options.may_prompt):
>> >  File "/usr/local/google/chrome/depot_tools/presubmit_support.py", line
>> > 967,
>> > in
>> > DoPresubmitChecks
>> >    results += executer.ExecPresubmitScript(presubmit_script, filename)
>> >  File "/usr/local/google/chrome/depot_tools/presubmit_support.py", line
>> > 898,
>> > in
>> > ExecPresubmitScript
>> >    result = eval(function_name + '(*__args)', context)
>> >  File "<string>", line 1, in <module>
>> >  File "<string>", line 117, in CheckChangeOnCommit
>> >  File "/usr/local/google/chrome/depot_tools/presubmit_canned_checks.py",
>> > line
>> > 455, in CheckRietveldTryJobExecution
>> >    host_url, input_api.change.issue, input_api.change.patchset)
>> > TypeError: %d format: a number is required, not str
>> > Sending crash report ...
>> >  exception: %d format: a number is required, not str
>> >  args: ['/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit',
>> > 'refs/remotes/origin/trunk']
>> >  stack:   File "/usr/local/google/chrome/src/.git/hooks/pr
>> >  host: [...]
>> >  user: estade
>> >  cwd: /usr/local/google/chrome/src
>> > A stack trace has been sent to the maintainers.
>> > Command "/usr/local/google/chrome/src/.git/hooks/pre-cl-dcommit
>> > refs/remotes/origin/trunk" failed.
>> >
>> >
>> > http://codereview.chromium.org/3622002/show
>> >
>
>

[stuartmorgan, 2010-10-07 23:49:55]

On 2010/10/07 23:45:24, Mandeep Singh Baines wrote:
> Reverted.

Argh, we were about to land http://codereview.chromium.org/3541019 to fix this,
as discussed in IRC.

[Evan Stade, 2010-10-07 23:52:11]

thank you for reverting

-- Evan Stade



On Thu, Oct 7, 2010 at 4:49 PM,  <stuartmorgan@chromium.org> wrote:
> On 2010/10/07 23:45:24, Mandeep Singh Baines wrote:
>>
>> Reverted.
>
> Argh, we were about to land http://codereview.chromium.org/3541019 to fix
> this,
> as discussed in IRC.
>
> http://codereview.chromium.org/3622002/show
>

[Mandeep Singh Baines, 2010-10-07 23:57:03]

On Thu, Oct 7, 2010 at 4:49 PM,  <stuartmorgan@chromium.org> wrote:
> On 2010/10/07 23:45:24, Mandeep Singh Baines wrote:
>>
>> Reverted.
>
> Argh, we were about to land http://codereview.chromium.org/3541019 to fix
> this,
> as discussed in IRC.
>

Oops. Didn't know about that change. Wasn't on the CC.

I'm on IRC but on the chromium-os-dev channel.

> http://codereview.chromium.org/3622002/show
>

LGTM on the change though.

You could cherry-pick my old change and then cherry-pick your change on top.

[Evan Stade, 2010-10-08 00:02:42]

On Thu, Oct 7, 2010 at 4:56 PM, Mandeep Singh Baines <msb@chromium.org> wrote:
> On Thu, Oct 7, 2010 at 4:49 PM,  <stuartmorgan@chromium.org> wrote:
>> On 2010/10/07 23:45:24, Mandeep Singh Baines wrote:
>>>
>>> Reverted.
>>
>> Argh, we were about to land http://codereview.chromium.org/3541019 to fix
>> this,
>> as discussed in IRC.
>>
>
> Oops. Didn't know about that change. Wasn't on the CC.
>
> I'm on IRC but on the chromium-os-dev channel.

you should probably always sit in #chromium, but if you are committing
to the chrome tree, you should definitely be in #chromium

>
>> http://codereview.chromium.org/3622002/show
>>
>
> LGTM on the change though.
>
> You could cherry-pick my old change and then cherry-pick your change on top.
>

[Mandeep Singh Baines, 2010-10-08 00:04:47]

On Thu, Oct 7, 2010 at 5:02 PM, Evan Stade <estade@chromium.org> wrote:
> On Thu, Oct 7, 2010 at 4:56 PM, Mandeep Singh Baines <msb@chromium.org> wrote:
>> On Thu, Oct 7, 2010 at 4:49 PM,  <stuartmorgan@chromium.org> wrote:
>>> On 2010/10/07 23:45:24, Mandeep Singh Baines wrote:
>>>>
>>>> Reverted.
>>>
>>> Argh, we were about to land http://codereview.chromium.org/3541019 to fix
>>> this,
>>> as discussed in IRC.
>>>
>>
>> Oops. Didn't know about that change. Wasn't on the CC.
>>
>> I'm on IRC but on the chromium-os-dev channel.
>
> you should probably always sit in #chromium, but if you are committing
> to the chrome tree, you should definitely be in #chromium
>

Good point. I do make git-cl and depot_tools changes every once in a while.
I'll add chromium to my list of channels.

>>
>>> http://codereview.chromium.org/3622002/show
>>>
>>
>> LGTM on the change though.
>>
>> You could cherry-pick my old change and then cherry-pick your change on top.
>>
>