Support --target-branch option to git-cl upload for Rietveld

Support --target-branch option to git-cl upload for Rietveld

This is similar to the Gerrit behavior in that we default to master,
unless the remote upstream is a branch head.

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

[Mike Wittman, 2015-01-07 21:19:08]

wittman@chromium.org changed reviewers:
+ agable@chromium.org, rmistry@chromium.org

[Mike Wittman, 2015-01-07 21:37:27]

PTAL. I think this will do the most sensible thing, at least for Chrome
development.

[agable, 2015-01-07 21:45:09]

https://codereview.chromium.org/822503005/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/822503005/diff/1/git_cl.py#newcode1752
git_cl.py:1752: if options.target_branch:
And what if the user passes --target_branch=refs/heads/master?

https://codereview.chromium.org/822503005/diff/1/git_cl.py#newcode1856
git_cl.py:1856: help='Remote branch to use for CL.  ' +
As noted above, this text is not explicit enough to know what kind of format to
use. origin/master? master? refs/remotes/origin/master? refs/heads/master? And
of those could be construed as a valid of way of specifying a "remote branch",
but the code above only handles one of them (master).

[Mike Wittman, 2015-01-07 23:53:39]

Patchset #2 (id:20001) has been deleted

[Mike Wittman, 2015-01-07 23:55:34]

https://codereview.chromium.org/822503005/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/822503005/diff/1/git_cl.py#newcode1752
git_cl.py:1752: if options.target_branch:
On 2015/01/07 21:45:09, agable wrote:
> And what if the user passes --target_branch=refs/heads/master?

Good point. I've updated the branch recognition to (I believe) recognize all
valid local and remote branch ref paths.

https://codereview.chromium.org/822503005/diff/1/git_cl.py#newcode1856
git_cl.py:1856: help='Remote branch to use for CL.  ' +
On 2015/01/07 21:45:09, agable wrote:
> As noted above, this text is not explicit enough to know what kind of format
to
> use. origin/master? master? refs/remotes/origin/master? refs/heads/master? And
> of those could be construed as a valid of way of specifying a "remote branch",
> but the code above only handles one of them (master).

We should now handle all of these cases.

[Mike Wittman, 2015-01-09 20:14:34]

ping

[rmistry, 2015-01-12 13:54:04]

rmistry@google.com changed reviewers:
+ rmistry@google.com
- rmistry@chromium.org

[rmistry, 2015-01-12 13:54:05]

Moving me from Reviewers to CC list. I believe Aaron is the right person to
review this.

[scheib, 2015-01-16 23:28:50]

On 2015/01/12 13:54:05, rmistry wrote:
> Moving me from Reviewers to CC list. I believe Aaron is the right person to
> review this.

Thanks for the patch Mike.

Aaron, PTAL? 435702 is so frustrating.

[Mike Wittman, 2015-01-22 19:24:50]

wittman@chromium.org changed reviewers:
+ cmp@chromium.org
- agable@chromium.org

[Mike Wittman, 2015-01-22 19:33:22]

On 2015/01/16 23:28:50, scheib wrote:
> On 2015/01/12 13:54:05, rmistry wrote:
> > Moving me from Reviewers to CC list. I believe Aaron is the right person to
> > review this.
> 
> Thanks for the patch Mike.
> 
> Aaron, PTAL? 435702 is so frustrating.

It's been two weeks since the last action on this bug so changing reviewers.

Chase, please take a look.

[cmp, 2015-01-22 20:53:15]

On 2015/01/22 19:33:22, Mike Wittman wrote:
> It's been two weeks since the last action on this bug so changing reviewers.
> 
> Chase, please take a look.

Aaron was out teaching a course for the past couple of days.  He confirmed he
would look at this CL today.

[cmp, 2015-01-22 20:53:57]

cmp@chromium.org changed reviewers:
+ agable@chromium.org
- cmp@chromium.org

[cmp, 2015-01-22 20:54:17]

Moved agable back to reviewer list.

[agable, 2015-01-22 22:59:11]

Mostly LGTM. The one real thing I'd like is some unit tests. The test format for
this is insane, I know, but tests/git_cl_test.py does exist, and it can make
sure that the commands output by this under various circumstances all make
sense.

https://codereview.chromium.org/822503005/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/822503005/diff/40001/git_cl.py#newcode1759
git_cl.py:1759: prefix_replacements = (('^((refs/)?remotes/)?branch-heads/',
nit: would prefer these be formatted as

prefix_replacements = (
    ('before',       'after'),
    ('other before', 'other after'),
)

I think it's easier to see cause and effect, and it looks like it'll fit.

https://codereview.chromium.org/822503005/diff/40001/git_cl.py#newcode1873
git_cl.py:1873: help='Remote branch to use for CL.  ' +
"to use for" is awkward and not super specific. Not sure what a better phrasing
would be though.

https://codereview.chromium.org/822503005/diff/40001/git_cl.py#newcode1874
git_cl.py:1874: 'Default: remote branch head, or master')
nit: indentation

[Mike Wittman, 2015-01-23 23:54:47]

PTAL.

On 2015/01/22 22:59:11, agable wrote:
> Mostly LGTM. The one real thing I'd like is some unit tests. The test format
for
> this is insane, I know, but tests/git_cl_test.py does exist, and it can make
> sure that the commands output by this under various circumstances all make
> sense.

Ugh, that approach to testing high-level functionality against issued git
commands needs to be nuked from orbit.

I tried to adapt _run_reviewer_test, but managing all the possible ways to name
refs in git commands across all the tests was prohibitively complicated. It was
massively simpler and easier to split the functionality into its own function
which can be tested independently.

https://codereview.chromium.org/822503005/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/822503005/diff/40001/git_cl.py#newcode1759
git_cl.py:1759: prefix_replacements = (('^((refs/)?remotes/)?branch-heads/',
On 2015/01/22 22:59:11, agable wrote:
> nit: would prefer these be formatted as
> 
> prefix_replacements = (
>     ('before',       'after'),
>     ('other before', 'other after'),
> )
> 
> I think it's easier to see cause and effect, and it looks like it'll fit.

Done.

https://codereview.chromium.org/822503005/diff/40001/git_cl.py#newcode1873
git_cl.py:1873: help='Remote branch to use for CL.  ' +
On 2015/01/22 22:59:11, agable wrote:
> "to use for" is awkward and not super specific. Not sure what a better
phrasing
> would be though.

Cleaned this up by referring to a metavar.

https://codereview.chromium.org/822503005/diff/40001/git_cl.py#newcode1874
git_cl.py:1874: 'Default: remote branch head, or master')
On 2015/01/22 22:59:11, agable wrote:
> nit: indentation

Done.

[agable, 2015-01-26 17:37:04]

Yuuuuup. It's a completely horrid and broken testing system. Thanks for writing
your own considerably more sane test :) This whole CL LGTM now % the one nit.

Thanks again!

https://codereview.chromium.org/822503005/diff/80001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/822503005/diff/80001/git_cl.py#newcode1683
git_cl.py:1683: """Computes the remote branch given the remote and remote branch
state from
nit: Docstring style is 

"""Single line description.

More multi-line description
if necessary.

Args:
  argument (type): description
  otherargument (type): other description
"""

[Mike Wittman, 2015-01-26 19:31:08]

https://codereview.chromium.org/822503005/diff/80001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/822503005/diff/80001/git_cl.py#newcode1683
git_cl.py:1683: """Computes the remote branch given the remote and remote branch
state from
On 2015/01/26 17:37:03, agable wrote:
> nit: Docstring style is 
> 
> """Single line description.
> 
> More multi-line description
> if necessary.
> 
> Args:
>   argument (type): description
>   otherargument (type): other description
> """

Done.

[Mike Wittman, 2015-01-26 19:31:12]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2015-01-26 19:31:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/822503005/100001

[commit-bot: I haz the power, 2015-01-26 19:34:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-26 19:34:26]

Presubmit check for 822503005-100001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
tests/checkout_test.py (38.08s) failed
Cloning into '/tmp/trialNeMJqj/__main__.GitCheckout.testAll/foo'...
done.
Switched to branch 'master'
Already on 'master'
.ECloning into '/tmp/trialNeMJqj/__main__.GitCheckout.testMove/foo'...
done.
.Cloning into '/tmp/trialNeMJqj/__main__.GitCheckout.testProcess/foo'...
done.
....................
======================================================================
ERROR: testException (__main__.GitCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/checkout_test.py", line 347, in setUp
    self.enabled = self.FAKE_REPOS.set_up_git()
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 359, in set_up_git
    self.set_up()
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 211, in set_up
    self.cleanup_dirt()
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 229, in cleanup_dirt
    if not self.tear_down_git():
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 276, in tear_down_git
    wait_for_port_to_free(self.host, self.git_port)
  File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/fake_repos.py",
line 161, in wait_for_port_to_free
    assert False, '%d is still bound' % port
AssertionError: 20000 is still bound

----------------------------------------------------------------------
Ran 23 tests in 37.808s

FAILED (errors=1)


Presubmit checks took 170.3s to calculate.

[Mike Wittman, 2015-01-26 20:12:09]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2015-01-26 20:12:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/822503005/100001

[commit-bot: I haz the power, 2015-01-26 20:15:52]

Committed patchset #5 (id:100001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=293807