depot_tools: Send the remote tracked ref to Rietveld via upload.py

depot_tools: Send the remote tracked ref to Rietveld via upload.py.

If your local branch is tracking another local branch then the correct transitively tracked remote ref will still be correctly uploaded to Rietveld.

This change goes hand in hand with the corresponding Rietveld change here:
https://codereview.chromium.org/773083004/
The motivation for both CLs came from the discussion in the internal CL: https://chromereviews.googleplex.com/115567013/

AFAIK either change can be submitted first without breaking anything in the other framework.

Observe the "Tracked Ref" field in the below CLs-

Tracking a remote ref:
* https://skia-codereview-staging.appspot.com/8861001 (Tracking skiabot-test's refs/heads/master)
* https://skia-codereview-staging.appspot.com/851002 (Tracking skiabot-test's refs/diff/test1)
* https://skia-codereview-staging.appspot.com/2891001 (Tracking Chromium's refs/heads/master)
* https://skia-codereview-staging.appspot.com/1931003 (Tracking Chromium's refs/branch-heads/1916)

Tracking a local branch which in turn tracks a remote ref:
* https://skia-codereview-staging.appspot.com/3891002 (Transitively tracking skiabot-test's refs/heads/master)
* https://skia-codereview-staging.appspot.com/4921001 (Transitively tracking Chromium's refs/branch-heads/1916)

CL when no target_ref is specified in depot_tools/third_party/upload.py:
* https://skia-codereview-staging.appspot.com/3871003 (CL with missing target_ref should default to /refs/heads/master)


Try the above links with and without the 'Deprecated UI' checked in https://skia-codereview-staging.appspot.com/settings


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

[rmistry, 2014-12-04 12:54:24]

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

[rmistry, 2014-12-04 12:54:25]

rmistry@google.com changed required reviewers:
+ agable@chromium.org

[rmistry, 2014-12-04 13:54:27]

On 2014/12/04 12:54:25, rmistry wrote:

I uploaded a chromium CL using branch-heads:
https://skia-codereview-staging.appspot.com/8851001

The remote branch shows up as "refs/remotes/branch-heads/1916"
I am assuming that this instead needs to be "refs/branch-heads/1916"
perhaps the fix is just to undo the 

    elif branch.startswith('refs/branch-heads/'):
        self._remote = (remote, branch.replace('refs/', 'refs/remotes/'))

substitution before I upload to Rietveld ?

[rmistry, 2014-12-04 14:07:36]

On 2014/12/04 13:54:27, rmistry wrote:
> On 2014/12/04 12:54:25, rmistry wrote:
> 
> I uploaded a chromium CL using branch-heads:
> https://skia-codereview-staging.appspot.com/8851001
> 
> The remote branch shows up as "refs/remotes/branch-heads/1916"
> I am assuming that this instead needs to be "refs/branch-heads/1916"
> perhaps the fix is just to undo the 
> 
>     elif branch.startswith('refs/branch-heads/'):
>         self._remote = (remote, branch.replace('refs/', 'refs/remotes/'))
> 
> substitution before I upload to Rietveld ?

Removed "remotes" @Patchset5.
The chromium CL now looks like this:
https://skia-codereview-staging.appspot.com/3871004

[rmistry, 2014-12-08 14:45:53]

Renamed remote_branch to target_ref in PatchSet8.

I also updated the code in git_cl.py to remove some of the manipulations done to
the target_ref, namely:
* refs/remotes/origin/refs/diff/test -> refs/diff/test
* refs/remotes/origin/master -> refs/heads/master
* refs/remotes/branch-heads/test -> refs/branch-heads/test

[rmistry, 2014-12-08 15:10:21]

On 2014/12/08 14:45:53, rmistry wrote:
> Renamed remote_branch to target_ref in PatchSet8.
> 
> I also updated the code in git_cl.py to remove some of the manipulations done
to
> the target_ref, namely:
> * refs/remotes/origin/refs/diff/test -> refs/diff/test
> * refs/remotes/origin/master -> refs/heads/master
> * refs/remotes/branch-heads/test -> refs/branch-heads/test

Updated the description with new test CLs.

[agable, 2014-12-09 20:42:55]

agable@chromium.org changed required reviewers:
- agable@chromium.org

[agable, 2014-12-09 20:42:56]

LGTM!

https://codereview.chromium.org/781523002/diff/140001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/781523002/diff/140001/git_cl.py#newcode1764
git_cl.py:1764: upload_args.extend(['--target_ref', remote_branch])
So what happens if either remote or remote_branch comes back None/False? Under
what circumstances does that happen? How can we prevent it?

https://codereview.chromium.org/781523002/diff/140001/third_party/upload.py
File third_party/upload.py (right):

https://codereview.chromium.org/781523002/diff/140001/third_party/upload.py#n...
third_party/upload.py:653: group.add_option("--target_ref", action="store",
dest="target_ref",
Make sure that this change gets landed in Rietveld's upload.py (in infra.git)
first, and then gets downstreamed to here.

[rmistry, 2014-12-09 20:54:43]

https://codereview.chromium.org/781523002/diff/140001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/781523002/diff/140001/git_cl.py#newcode1764
git_cl.py:1764: upload_args.extend(['--target_ref', remote_branch])
On 2014/12/09 20:42:56, agable wrote:
> So what happens if either remote or remote_branch comes back None/False? Under
> what circumstances does that happen? How can we prevent it?

Those are good questions and I am not sure. Looking at GetRemoteBranch it does
not appear that either should come back as None but it is probably good to be
defensive here.

https://codereview.chromium.org/781523002/diff/140001/third_party/upload.py
File third_party/upload.py (right):

https://codereview.chromium.org/781523002/diff/140001/third_party/upload.py#n...
third_party/upload.py:653: group.add_option("--target_ref", action="store",
dest="target_ref",
On 2014/12/09 20:42:56, agable wrote:
> Make sure that this change gets landed in Rietveld's upload.py (in infra.git)
> first, and then gets downstreamed to here.

I am glad you brought this up.
Could you explain how this works? is this a copy of
https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rie...
or is it brought here by some DEPS (which I could not find).

Do I need to make a change to
https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rie...
and to depot_tools/third_party/upload.py ?

[agable, 2014-12-09 23:36:33]

https://codereview.chromium.org/781523002/diff/140001/third_party/upload.py
File third_party/upload.py (right):

https://codereview.chromium.org/781523002/diff/140001/third_party/upload.py#n...
third_party/upload.py:653: group.add_option("--target_ref", action="store",
dest="target_ref",
On 2014/12/09 20:54:43, rmistry wrote:
> On 2014/12/09 20:42:56, agable wrote:
> > Make sure that this change gets landed in Rietveld's upload.py (in
infra.git)
> > first, and then gets downstreamed to here.
> 
> I am glad you brought this up.
> Could you explain how this works? is this a copy of
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rie...
> or is it brought here by some DEPS (which I could not find).
> 
> Do I need to make a change to
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rie...
> and to depot_tools/third_party/upload.py ?

It is a copy of the one in chromium_rietveld. The procedure is
1) Figure out the change you want to land in depot_tools
2) Land it in infra.git
3) Land it in depot_tools

It used to be more important, when Rietveld lived in its own repo and we didn't
100% control it. We had to land changes there first and then downstream them to
us. Now, I think the order matters less. But we do have to make sure the same
changes land in both.

[rmistry, 2014-12-10 12:36:20]

On 2014/12/09 23:36:33, agable wrote:
> https://codereview.chromium.org/781523002/diff/140001/third_party/upload.py
> File third_party/upload.py (right):
> 
>
https://codereview.chromium.org/781523002/diff/140001/third_party/upload.py#n...
> third_party/upload.py:653: group.add_option("--target_ref", action="store",
> dest="target_ref",
> On 2014/12/09 20:54:43, rmistry wrote:
> > On 2014/12/09 20:42:56, agable wrote:
> > > Make sure that this change gets landed in Rietveld's upload.py (in
> infra.git)
> > > first, and then gets downstreamed to here.
> > 
> > I am glad you brought this up.
> > Could you explain how this works? is this a copy of
> >
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rie...
> > or is it brought here by some DEPS (which I could not find).
> > 
> > Do I need to make a change to
> >
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rie...
> > and to depot_tools/third_party/upload.py ?
> 
> It is a copy of the one in chromium_rietveld. The procedure is
> 1) Figure out the change you want to land in depot_tools
> 2) Land it in infra.git
> 3) Land it in depot_tools
> 
> It used to be more important, when Rietveld lived in its own repo and we
didn't
> 100% control it. We had to land changes there first and then downstream them
to
> us. Now, I think the order matters less. But we do have to make sure the same
> changes land in both.

Thank you for the explanation.
https://codereview.chromium.org/746343004/ has landed in infra.git.
I will land this depot_tools CL later today.

[agable, 2014-12-10 19:14:04]

SGTM

[rmistry, 2014-12-10 20:55:51]

On 2014/12/10 19:14:04, agable wrote:
> SGTM

Now that the Rietveld change in https://codereview.chromium.org/773083004/ is
live I am going to submit this one.

[rmistry, 2014-12-10 20:56:00]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2014-12-10 20:56:23]

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

[commit-bot: I haz the power, 2014-12-10 20:58:25]

Committed patchset #8 (id:140001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=293334