Add minimal Gerrit support to 'git cl config' and 'git cl upload'

Add minimal Gerrit support to 'git cl config' and 'git cl upload'

This is the bare beginnings of Gerrit support for (non-repo) depot_tools,
based on Roland's work.
Differnt from http://codereview.chromium.org/8826015/
 it read codereview.settings and if it has GERRTI_HOST and GERRIT_PORT,
 then "git cl config" configured it for gerrit.
   installs hooks/commit-msg
   git config gerrit.host $GERRIT_HOST
   git config gerrit.port $GERRIT_PORT

 if it has gerrit.host config, "git cl upload" will upload a change
 to gerrit as
   "git push --receive-pack=... origin master"
 it scans description and extract reviewers from R= line.

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

[ukai, 2012-01-31 10:46:43]

As evan's comment on 8826015, I add gerrit support in git-cl in this CL.  Or
should I create another command?
I'm new to gerrit, so I'm not sure it would cover basic gerrit workflow.  Please
tell me better way if any.

If it looks ok, I'll add some tests for gerrit case.

[Evan Martin, 2012-01-31 19:07:56]

http://codereview.chromium.org/9264065/diff/1/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode150
git_cl.py:150: self.is_gerrit = False
Set this to None here...

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode271
git_cl.py:271: self.is_gerrit = self._GetConfig('gerrit.host', error_ok=True)
...so that here we can change this test to
  if self.is_gerrit is None:
so that this test doesn't alwasy run when self.is_gerrit is False.

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode907
git_cl.py:907: print reviewers
Probably don't mean to print here.

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode915
git_cl.py:915: git_command.append('--receive-pack=git receive-pack ' +
This has a space in the argument. I don't think it will work as-is (?)

[Roland McGrath, 2012-01-31 19:24:20]

http://codereview.chromium.org/9264065/diff/1/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode915
git_cl.py:915: git_command.append('--receive-pack=git receive-pack ' +
On 2012/01/31 19:07:56, Evan Martin wrote:
> This has a space in the argument. I don't think it will work as-is (?)

No, it has to be this way.

[ukai, 2012-02-01 05:57:14]

http://codereview.chromium.org/9264065/diff/1/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode150
git_cl.py:150: self.is_gerrit = False
On 2012/01/31 19:07:56, Evan Martin wrote:
> Set this to None here...

Done.

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode271
git_cl.py:271: self.is_gerrit = self._GetConfig('gerrit.host', error_ok=True)
On 2012/01/31 19:07:56, Evan Martin wrote:
> ...so that here we can change this test to
>   if self.is_gerrit is None:
> so that this test doesn't alwasy run when self.is_gerrit is False.

Done.

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode907
git_cl.py:907: print reviewers
On 2012/01/31 19:07:56, Evan Martin wrote:
> Probably don't mean to print here.

Done.

http://codereview.chromium.org/9264065/diff/1/git_cl.py#newcode915
git_cl.py:915: git_command.append('--receive-pack=git receive-pack ' +
On 2012/01/31 19:07:56, Evan Martin wrote:
> This has a space in the argument. I don't think it will work as-is (?)
we pass "git receive-pack --reviewer=.." as value for --receive-pack option.
we want to run
 git push '--receive-pack=git receive-pack --reviewer=foo@a.com' origin
HEAD:refs/for/branch

[M-A Ruel, 2012-02-01 15:50:35]

personal nit: I prefer to keep the internal logic symmetric between both
workflow as much as possible so the code doesn't become a control flow mess

http://codereview.chromium.org/9264065/diff/6002/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/9264065/diff/6002/git_cl.py#newcode886
git_cl.py:886: # It is probably not worthwhile to support different workflows.
What about CL pipelining? Why not just get the branch ancestor?

http://codereview.chromium.org/9264065/diff/6002/git_cl.py#newcode903
git_cl.py:903: reviewer_regexp = re.compile(r'^\s*(TBR|R)=(.+)$', re.MULTILINE)
Why can't you reuse the parsing code? It'd be nice to have a proper Changelist
object instance.

http://codereview.chromium.org/9264065/diff/6002/git_cl.py#newcode960
git_cl.py:960: args = ['master']
Why?

http://codereview.chromium.org/9264065/diff/6002/git_cl.py#newcode988
git_cl.py:988: upload_args = ['--assume_yes']  # Don't ask about untracked
files.
I'd rather move the rest into RietveldUpload to make the difference unambiguous.

[ukai, 2012-02-02 04:21:59]

http://codereview.chromium.org/9264065/diff/6002/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/9264065/diff/6002/git_cl.py#newcode886
git_cl.py:886: # It is probably not worthwhile to support different workflows.
On 2012/02/01 15:50:35, Marc-Antoine Ruel wrote:
> What about CL pipelining? Why not just get the branch ancestor?

I'm new to gerrit and don't know how to do it yet...

http://codereview.chromium.org/9264065/diff/6002/git_cl.py#newcode903
git_cl.py:903: reviewer_regexp = re.compile(r'^\s*(TBR|R)=(.+)$', re.MULTILINE)
On 2012/02/01 15:50:35, Marc-Antoine Ruel wrote:
> Why can't you reuse the parsing code? It'd be nice to have a proper Changelist
> object instance.

Done.

http://codereview.chromium.org/9264065/diff/6002/git_cl.py#newcode960
git_cl.py:960: args = ['master']
On 2012/02/01 15:50:35, Marc-Antoine Ruel wrote:
> Why?

I initially thought args[0] to be used for target branch in gerrit.
I changed it to use --target_branch option.

http://codereview.chromium.org/9264065/diff/6002/git_cl.py#newcode988
git_cl.py:988: upload_args = ['--assume_yes']  # Don't ask about untracked
files.
On 2012/02/01 15:50:35, Marc-Antoine Ruel wrote:
> I'd rather move the rest into RietveldUpload to make the difference
unambiguous.

Done.

[M-A Ruel, 2012-02-02 14:16:23]

lgtm with nits, no need to wait for me again, use the CQ.

http://codereview.chromium.org/9264065/diff/3005/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/9264065/diff/3005/git_cl.py#newcode906
git_cl.py:906: cc = filter(None, (cl.GetCCList(), cc))
Note that cl.GetCCList() returns a string and cc is a list. So you a
tuple(string, list)

http://codereview.chromium.org/9264065/diff/3005/git_cl.py#newcode908
git_cl.py:908: receive_options += ['--cc=' + email for email in cc]
Do you think it does what you want it to do?

I would probably shorten lines 903-908 to:
cc = ','.join(filter(None, (options.cc, cc.GetCCList()))
if cc:
  receive_options.extend(['--cc', cc])

http://codereview.chromium.org/9264065/diff/3005/git_cl.py#newcode910
git_cl.py:910: reviewers = filter(None, change_desc.reviewers.split(','))
I see that you want to correctly parse "foo,,bar" but I don't think it's
valuable.

http://codereview.chromium.org/9264065/diff/3005/git_cl.py#newcode916
git_cl.py:916: git_command.append('--receive-pack=git receive-pack ' +
You don't technically need to use a quote but I'd prefer you to, especially if
this is ever to be run on windows. So something like:
git_command.append('--receive-pack')
git_command.append('"git receive-pack %s"' % ' '.join(receive_options))

note the extra quotes.

[ukai, 2012-02-03 03:34:27]

http://codereview.chromium.org/9264065/diff/3005/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/9264065/diff/3005/git_cl.py#newcode906
git_cl.py:906: cc = filter(None, (cl.GetCCList(), cc))
On 2012/02/02 14:16:23, Marc-Antoine Ruel wrote:
> Note that cl.GetCCList() returns a string and cc is a list. So you a
> tuple(string, list)

Oh, fixed.

http://codereview.chromium.org/9264065/diff/3005/git_cl.py#newcode908
git_cl.py:908: receive_options += ['--cc=' + email for email in cc]
On 2012/02/02 14:16:23, Marc-Antoine Ruel wrote:
> Do you think it does what you want it to do?
> 
> I would probably shorten lines 903-908 to:
> cc = ','.join(filter(None, (options.cc, cc.GetCCList()))
> if cc:
>   receive_options.extend(['--cc', cc])

I'm not sure gerrit's receive-pack accepts --cc foo,bar style.
From
http://gerrit-documentation.googlecode.com/svn/Documentation/2.1.7/cmd-receiv...,
I think we need to pass as --cc foo --cc bar

http://codereview.chromium.org/9264065/diff/3005/git_cl.py#newcode916
git_cl.py:916: git_command.append('--receive-pack=git receive-pack ' +
On 2012/02/02 14:16:23, Marc-Antoine Ruel wrote:
> You don't technically need to use a quote but I'd prefer you to, especially if
> this is ever to be run on windows. So something like:
> git_command.append('--receive-pack')
> git_command.append('"git receive-pack %s"' % ' '.join(receive_options))
> 
> note the extra quotes.

added quotes.

[commit-bot: I haz the power, 2012-02-03 03:35:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ukai@chromium.org/9264065/10001

[commit-bot: I haz the power, 2012-02-03 03:41:48]

Change committed as 120276

[ukai, 2012-02-03 04:36:25]

I found a bug with this CL.
Please take a look at http://codereview.chromium.org/9325018/