Rietveld: Display the remote tracked ref in both old and new UIs

Rietveld: Display the remote tracked ref in both old and new UIs.

The revert patchset button also preserves the remote tracking ref information in the new inverted CL.

This change goes hand in hand with the corresponding depot_tools change here:
https://codereview.chromium.org/781523002/
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
NOTRY=true

Committed: https://chromium.googlesource.com/infra/infra/+/abe93bea09182b5309d1c17f7b83e4899db05dff

[rmistry, 2014-12-04 12:56:13]

rmistry@google.com changed reviewers:
+ agable@chromium.org, jrobbins@chromium.org, sergeyberezin@chromium.org,
sheyang@chromium.org

[rmistry, 2014-12-04 12:56:13]

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

[rmistry, 2014-12-04 12:56:14]

CQ folks, please review the overall look/design based on conversations in
https://chromereviews.googleplex.com/115567013/.

Jason, please review the code.

[Jason Robbins -- corp, 2014-12-05 23:41:14]

jrobbins@google.com changed reviewers:
+ jrobbins@google.com

[Jason Robbins -- corp, 2014-12-05 23:41:15]

lgtm

[agable, 2014-12-06 00:48:34]

agable@chromium.org changed reviewers:
- jrobbins@google.com

[agable, 2014-12-06 00:48:34]

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

[agable, 2014-12-06 00:48:35]

I like all of it except for the name :) I have some issues with the values set
in the sample CLs, but I'll bring those issues up in the depot_tools CL that
actually populates the value. The UI code here looks good as far as I'm
concerned, but will leave the final approval to jrobbins@.

https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
File appengine/chromium_rietveld/codereview/models.py (right):

https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
appengine/chromium_rietveld/codereview/models.py:80: remote_branch =
ndb.StringProperty()
Calling it "remote_branch" is a little misleading. It is representing
information as it was on the developer's machine. What we really want is "what
ref in the golden-source-of-truth repo will this land on?". To that effect, I
think just "ref" or "target_ref" would be better.

[agable, 2014-12-06 00:49:20]

Oh good, looks like he already approved it. No idea why the new UI decided to
generate "changed reviewer" message for me though; I didn't touch the reviewers
field. Bug rep;ort!

[rmistry, 2014-12-08 14:05:30]

Patchset #4 (id:60001) has been deleted

[rmistry, 2014-12-08 14:43:20]

I also updated the depot_tools CL https://codereview.chromium.org/781523002/ to
use target_ref.

https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
File appengine/chromium_rietveld/codereview/models.py (right):

https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
appengine/chromium_rietveld/codereview/models.py:80: remote_branch =
ndb.StringProperty()
On 2014/12/06 00:48:35, agable wrote:
> Calling it "remote_branch" is a little misleading. It is representing
> information as it was on the developer's machine. What we really want is "what
> ref in the golden-source-of-truth repo will this land on?". To that effect, I
> think just "ref" or "target_ref" would be better.

Agreed. Changed to target_ref.

[rmistry, 2014-12-08 14:48:39]

On 2014/12/08 14:43:20, rmistry wrote:
> I also updated the depot_tools CL https://codereview.chromium.org/781523002/
to
> use target_ref.
> 
>
https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
> File appengine/chromium_rietveld/codereview/models.py (right):
> 
>
https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
> appengine/chromium_rietveld/codereview/models.py:80: remote_branch =
> ndb.StringProperty()
> On 2014/12/06 00:48:35, agable wrote:
> > Calling it "remote_branch" is a little misleading. It is representing
> > information as it was on the developer's machine. What we really want is
"what
> > ref in the golden-source-of-truth repo will this land on?". To that effect,
I
> > think just "ref" or "target_ref" would be better.
> 
> Agreed. Changed to target_ref.

Also noticed that the revert patchset button was not sending the target_ref to
the inverted CL. Fixed this.

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

On 2014/12/08 14:48:39, rmistry wrote:
> On 2014/12/08 14:43:20, rmistry wrote:
> > I also updated the depot_tools CL https://codereview.chromium.org/781523002/
> to
> > use target_ref.
> > 
> >
>
https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
> > File appengine/chromium_rietveld/codereview/models.py (right):
> > 
> >
>
https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
> > appengine/chromium_rietveld/codereview/models.py:80: remote_branch =
> > ndb.StringProperty()
> > On 2014/12/06 00:48:35, agable wrote:
> > > Calling it "remote_branch" is a little misleading. It is representing
> > > information as it was on the developer's machine. What we really want is
> "what
> > > ref in the golden-source-of-truth repo will this land on?". To that
effect,
> I
> > > think just "ref" or "target_ref" would be better.
> > 
> > Agreed. Changed to target_ref.
> 
> Also noticed that the revert patchset button was not sending the target_ref to
> the inverted CL. Fixed this.

Updated the description with new test CLs.

[rmistry, 2014-12-08 18:09:08]

On 2014/12/08 15:10:26, rmistry wrote:
> On 2014/12/08 14:48:39, rmistry wrote:
> > On 2014/12/08 14:43:20, rmistry wrote:
> > > I also updated the depot_tools CL
https://codereview.chromium.org/781523002/
> > to
> > > use target_ref.
> > > 
> > >
> >
>
https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
> > > File appengine/chromium_rietveld/codereview/models.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
> > > appengine/chromium_rietveld/codereview/models.py:80: remote_branch =
> > > ndb.StringProperty()
> > > On 2014/12/06 00:48:35, agable wrote:
> > > > Calling it "remote_branch" is a little misleading. It is representing
> > > > information as it was on the developer's machine. What we really want is
> > "what
> > > > ref in the golden-source-of-truth repo will this land on?". To that
> effect,
> > I
> > > > think just "ref" or "target_ref" would be better.
> > > 
> > > Agreed. Changed to target_ref.
> > 
> > Also noticed that the revert patchset button was not sending the target_ref
to
> > the inverted CL. Fixed this.
> 
> Updated the description with new test CLs.

If this CL still looks good then I would like to land it and get it in today's
push. That way at least 1 of the 3 required CLs will be in.

[rmistry, 2014-12-09 12:25:09]

On 2014/12/08 18:09:08, rmistry wrote:
> On 2014/12/08 15:10:26, rmistry wrote:
> > On 2014/12/08 14:48:39, rmistry wrote:
> > > On 2014/12/08 14:43:20, rmistry wrote:
> > > > I also updated the depot_tools CL
> https://codereview.chromium.org/781523002/
> > > to
> > > > use target_ref.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
> > > > File appengine/chromium_rietveld/codereview/models.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/773083004/diff/40001/appengine/chromium_rietv...
> > > > appengine/chromium_rietveld/codereview/models.py:80: remote_branch =
> > > > ndb.StringProperty()
> > > > On 2014/12/06 00:48:35, agable wrote:
> > > > > Calling it "remote_branch" is a little misleading. It is representing
> > > > > information as it was on the developer's machine. What we really want
is
> > > "what
> > > > > ref in the golden-source-of-truth repo will this land on?". To that
> > effect,
> > > I
> > > > > think just "ref" or "target_ref" would be better.
> > > > 
> > > > Agreed. Changed to target_ref.
> > > 
> > > Also noticed that the revert patchset button was not sending the
target_ref
> to
> > > the inverted CL. Fixed this.
> > 
> > Updated the description with new test CLs.
> 
> If this CL still looks good then I would like to land it and get it in today's
> push. That way at least 1 of the 3 required CLs will be in.

Jason said offline that the change looks good to him. Aaron?

[agable, 2014-12-09 21:20:36]

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

[agable, 2014-12-09 21:20:37]

LGTM! I'm so happy this is actually happening.

[rmistry, 2014-12-09 21:22:47]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2014-12-09 21:23:45]

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

[commit-bot: I haz the power, 2014-12-09 21:29:56]

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

[commit-bot: I haz the power, 2014-12-09 21:29:57]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[rmistry, 2014-12-09 21:31:16]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2014-12-09 21:31:54]

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

[commit-bot: I haz the power, 2014-12-09 21:32:13]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/abe93bea09182b5309d1c17f7b83e...