[depot_tools] New "git cl upload" flag to traverse dependent branches and re-upload them

[depot_tools] New "git cl upload" flag to traverse dependent branches and re-upload them.

Motivation:
The conversation in https://docs.google.com/document/d/1KZGFKZpOPvco81sYVRCzwlnjGctup71RAzY0MSb0ntc/edit?disco=AAAAAXU60E8

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

[rmistry, 2015-06-15 18:14:23]

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

[nodir, 2015-06-15 23:50:22]

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1524
git_cl.py:1524: Rietveld.
Rebasing would be very nice, for example if I want to test a change locally
against code in another CL. Do you plan to provide this functionality as an
option or a separate command?

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1529
git_cl.py:1529: parent_branch = ShortBranchName(branchref)
nit: call it root_branch?

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1543
git_cl.py:1543: tracked_to_dependents = {}
collections.defaultdict([])
lines 1546-1547 can be removed

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1545
git_cl.py:1545: branch_name, tracked = b.split()
this will crash on a non-tracked branch: b.split() will return a list of one
element

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1552
git_cl.py:1552: inorder_dependents = []
nit: "inorder" is somewhat confusing because it is about a different type of
traversal, see https://en.wikipedia.org/wiki/Tree_traversal 
I think what you meant is topological order
https://en.wikipedia.org/wiki/Topological_sorting

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1568
git_cl.py:1568: if inorder_dependents:
inverse the condition, return early and deindent the rest of code in this func

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1574
git_cl.py:1574: RunGit(['checkout', '-q', dependent_branch])
can CMDupload_branch_deps be called when the work tree is dirty?

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1576
git_cl.py:1576: if CMDupload(OptionParser(), args) != 0:
Will it ask for pathset title every time? 
I think this tool should give a standard title, like "Updated patchset
dependency 123/456"

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1577
git_cl.py:1577: print 'Upload failed for %s!' % dependent_branch
I think users would thank you if you check that the CL being updated already
depends on the latest patchset. For example, if upload of one of patchsets
failed, they wouldn't have to upload identical patchsets again

[iannucci, 2015-06-15 23:55:12]

iannucci@chromium.org changed reviewers:
+ iannucci@chromium.org

[iannucci, 2015-06-15 23:55:12]

added some more comments, nodir's comments are also good

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1524
git_cl.py:1524: Rietveld.
On 2015/06/15 23:50:22, nodir wrote:
> Rebasing would be very nice, for example if I want to test a change locally
> against code in another CL. Do you plan to provide this functionality as an
> option or a separate command?

git rebase-update can/does already do this.

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1548
git_cl.py:1548: tracked_to_dependents[tracked].append(branch_name)
It would be worth consolidating this branch hierarchy calculation with the same
calculation which is done in rebase-update. I can't remember if there's already
a function that does this in git_common, or if it's implemented in
git_rebase_update directly.

[nodir, 2015-06-15 23:58:43]

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1524
git_cl.py:1524: Rietveld.
On 2015/06/15 23:55:12, iannucci wrote:
> On 2015/06/15 23:50:22, nodir wrote:
> > Rebasing would be very nice, for example if I want to test a change locally
> > against code in another CL. Do you plan to provide this functionality as an
> > option or a separate command?
> 
> git rebase-update can/does already do this.

Acknowledged.

[rmistry, 2015-06-17 17:58:46]

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1529
git_cl.py:1529: parent_branch = ShortBranchName(branchref)
On 2015/06/15 23:50:21, nodir wrote:
> nit: call it root_branch?

Done.

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1543
git_cl.py:1543: tracked_to_dependents = {}
On 2015/06/15 23:50:21, nodir wrote:
> collections.defaultdict([])
> lines 1546-1547 can be removed

Cool. Done.

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1545
git_cl.py:1545: branch_name, tracked = b.split()
On 2015/06/15 23:50:22, nodir wrote:
> this will crash on a non-tracked branch: b.split() will return a list of one
> element

Done.

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1548
git_cl.py:1548: tracked_to_dependents[tracked].append(branch_name)
On 2015/06/15 23:55:12, iannucci wrote:
> It would be worth consolidating this branch hierarchy calculation with the
same
> calculation which is done in rebase-update. I can't remember if there's
already
> a function that does this in git_common, or if it's implemented in
> git_rebase_update directly.

I looked at git_rebase_update and tried using some of the git_common methods it
used like:
git_common.get_branch_tree and git_common.topo_iter(branch_tree)

I prefer the code here because it is specific and optimized for this particular
usecase.
get_branch_tree and topo_iter would work for this if we could specify a root
instead of it considering all roots, but I could not find a clean way to do this
in the existing implementation without rewrites (which I would like to avoid).

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1552
git_cl.py:1552: inorder_dependents = []
On 2015/06/15 23:50:22, nodir wrote:
> nit: "inorder" is somewhat confusing because it is about a different type of
> traversal, see https://en.wikipedia.org/wiki/Tree_traversal 
> I think what you meant is topological order
> https://en.wikipedia.org/wiki/Topological_sorting

Oops. Yea I meant preorder here.

                          test2
              test3               test3.1
          test4   test4.1
       test5


prints ['test2', 'test3', 'test4', 'test5', 'test4.1', 'test3.1']

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1568
git_cl.py:1568: if inorder_dependents:
On 2015/06/15 23:50:22, nodir wrote:
> inverse the condition, return early and deindent the rest of code in this func

Done.

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1574
git_cl.py:1574: RunGit(['checkout', '-q', dependent_branch])
On 2015/06/15 23:50:22, nodir wrote:
> can CMDupload_branch_deps be called when the work tree is dirty?

At this point you have checked out a branch so AFAIK the work tree there cannot
be dirty because you were out of that branch originally.

But the root branch could be dirty, I added a check for that.

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1576
git_cl.py:1576: if CMDupload(OptionParser(), args) != 0:
On 2015/06/15 23:50:22, nodir wrote:
> Will it ask for pathset title every time? 
> I think this tool should give a standard title, like "Updated patchset
> dependency 123/456"

I wanted to do this when I started this CL but forgot to.
Thanks for the reminder! Done.

https://codereview.chromium.org/1191473002/diff/100001/git_cl.py#newcode1577
git_cl.py:1577: print 'Upload failed for %s!' % dependent_branch
On 2015/06/15 23:50:21, nodir wrote:
> I think users would thank you if you check that the CL being updated already
> depends on the latest patchset. For example, if upload of one of patchsets
> failed, they wouldn't have to upload identical patchsets again

Yea I think that is an optimization that can be made in the future based on
feedback.
Here I prefer to upload patches instead of adding more checks and having to deal
with more edge cases (it does not hurt to upload many patches as discussed
before).

[agable, 2015-06-17 18:13:35]

Top level comment: why isn't this part of, say, "git cl upload --dependencies"
or something like that?

https://codereview.chromium.org/1191473002/diff/120001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1191473002/diff/120001/git_cl.py#newcode1550
git_cl.py:1550: # Create a dictionary of all local branches to the branches that
are dependent
Seems like you should be able to reuse code from git-map-branches for this? It
creates the whole dependency tree, you could just reuse that and then grab the
subtree you care about here.

https://codereview.chromium.org/1191473002/diff/120001/git_cl.py#newcode1576
git_cl.py:1576: if not dependents:
Do this before requiring user input above.

[rmistry, 2015-06-18 17:42:16]

Patchset #9 (id:160001) has been deleted

[rmistry, 2015-06-18 17:52:40]

https://codereview.chromium.org/1191473002/diff/120001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1191473002/diff/120001/git_cl.py#newcode1550
git_cl.py:1550: # Create a dictionary of all local branches to the branches that
are dependent
On 2015/06/17 18:13:34, agable wrote:
> Seems like you should be able to reuse code from git-map-branches for this? It
> creates the whole dependency tree, you could just reuse that and then grab the
> subtree you care about here.


My comment for this is similar to my earlier comment about get_branch_tree and
topo_iter:

I took a look at git_map_branches, I'm sure its possible to refactor it and
extract out, but it is not obvious and straightforward to me how to do this
cleanly without complicated changes (which I would like to avoid). I prefer the
code here because it is specific and optimized for this particular usecase.

https://codereview.chromium.org/1191473002/diff/120001/git_cl.py#newcode1576
git_cl.py:1576: if not dependents:
On 2015/06/17 18:13:34, agable wrote:
> Do this before requiring user input above.

Done.

[rmistry, 2015-06-18 17:52:54]

On 2015/06/17 18:13:35, agable wrote:
> Top level comment: why isn't this part of, say, "git cl upload --dependencies"
> or something like that?

This is a good idea. Done.

> 
> https://codereview.chromium.org/1191473002/diff/120001/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/1191473002/diff/120001/git_cl.py#newcode1550
> git_cl.py:1550: # Create a dictionary of all local branches to the branches
that
> are dependent
> Seems like you should be able to reuse code from git-map-branches for this? It
> creates the whole dependency tree, you could just reuse that and then grab the
> subtree you care about here.
> 
> https://codereview.chromium.org/1191473002/diff/120001/git_cl.py#newcode1576
> git_cl.py:1576: if not dependents:
> Do this before requiring user input above.

[agable, 2015-06-18 23:16:09]

This LGTM, but would be good for iannucci to do one final pass if he wants.

[rmistry, 2015-06-19 12:53:04]

On 2015/06/18 23:16:09, agable wrote:
> This LGTM, but would be good for iannucci to do one final pass if he wants.

I plan to submit this on Monday if there are no further comments.

[rmistry, 2015-06-19 17:16:17]

The CQ bit was checked by rmistry@google.com to run a CQ dry run

[rmistry, 2015-06-19 17:16:18]

The patchset sent to the CQ was uploaded after l-g-t-m from agable@chromium.org
Link to the patchset: https://codereview.chromium.org/1191473002/#ps220001
(title: "Fix patchset title")

[commit-bot: I haz the power, 2015-06-19 17:21:02]

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

[commit-bot: I haz the power, 2015-06-19 17:21:04]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[rmistry, 2015-06-19 17:23:32]

The CQ bit was checked by rmistry@google.com to run a CQ dry run

[rmistry, 2015-06-19 17:23:33]

The patchset sent to the CQ was uploaded after l-g-t-m from agable@chromium.org
Link to the patchset: https://codereview.chromium.org/1191473002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-19 17:23:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191473002/240001

[commit-bot: I haz the power, 2015-06-19 17:28:43]

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

[commit-bot: I haz the power, 2015-06-19 17:28:44]

Dry run: Try jobs failed on following builders:
  depot_tools_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/depot_tools_pre...)

[rmistry, 2015-06-19 17:42:19]

The CQ bit was checked by rmistry@google.com to run a CQ dry run

[rmistry, 2015-06-19 17:42:20]

The patchset sent to the CQ was uploaded after l-g-t-m from agable@chromium.org
Link to the patchset: https://codereview.chromium.org/1191473002/#ps260001
(title: "Fix lint")

[commit-bot: I haz the power, 2015-06-19 17:42:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191473002/260001

[commit-bot: I haz the power, 2015-06-19 17:44:21]

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

[commit-bot: I haz the power, 2015-06-19 17:44:23]

Dry run: This issue passed the CQ dry run.

[rmistry, 2015-06-22 12:20:16]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2015-06-22 12:20:21]

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

[commit-bot: I haz the power, 2015-06-22 12:22:20]

Committed patchset #13 (id:260001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295779