Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1991563005:
Add "archive" command to git_cl.py. (Closed)
|
Created:
4 years, 7 months ago by Kevin M Modified:
4 years, 6 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org, tandrii+omg_git_cl_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionAdd "archive" command to git_cl.py.
This command archives branches whose Rietveldt status is closed by
creating new Git tags for each of the branches' heads, and then
deleting the branch. It automatically cleans up the clutter that
accumulates over time in a long-lived Git checkout.
For example, the branch "foo-bar" associated with the
closed issue 1568403002 will be archived to the tag
"git-cl-archived-1568403002-foo-bar".
BUG=616404
R=martiniss@chromium.org,tandrii@chromium.org
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/3bff56bb499c27610fa7f3d538996048e6116d73
Patch Set 1 #
Total comments: 10
Patch Set 2 : Inlined "changes" variable #Patch Set 3 : Made changes; added tests #Patch Set 4 : Rebase and modify unit tests accordingly. #
Total comments: 16
Patch Set 5 : tandrii comments #
Total comments: 8
Patch Set 6 : cl feedback #Patch Set 7 : reverse order of branch/issue ID #
Total comments: 28
Patch Set 8 : code review feedback #Patch Set 9 : "cleanup" => "archive" #Patch Set 10 : updated proposed tag name #
Total comments: 2
Patch Set 11 : use default sort() comparator #
Total comments: 6
Patch Set 12 : m-a ruel feedback #Patch Set 13 : rebase #Messages
Total messages: 49 (16 generated)
Description was changed from ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. This automatically cleans up the clutter that accumulates over time in a long-lived Git checkout For example, the closed branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "submitted-foo-bar-1568403002". R=martiniss@chromium.org ========== to ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. This automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the closed branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "submitted-foo-bar-1568403002". R=martiniss@chromium.org ==========
Description was changed from ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. This automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the closed branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "submitted-foo-bar-1568403002". R=martiniss@chromium.org ========== to ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. This automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "submitted-foo-bar-1568403002". R=martiniss@chromium.org ==========
Description was changed from ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. This automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "submitted-foo-bar-1568403002". R=martiniss@chromium.org ========== to ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "submitted-foo-bar-1568403002". R=martiniss@chromium.org ==========
martiniss@chromium.org changed reviewers: + tandrii@chromium.org
change looks fine, but I would like tandrii to look at it because he has a TODO in the code you copied, and because he's been making a fair amount of changes to git cl lately for gerrit work, and this might be impacted by that. If you could refactor the code path used by the two in a reasonable and not annoying/stretching it to fit both use case-y way, that'd be nice. https://codereview.chromium.org/1991563005/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode3123 git_cl.py:3123: branches = [c.GetBranch() for c in changes] Any reason to not combine this and the previous line? https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode3148 git_cl.py:3148: RunGit(['tag', tagname, branch]) What is the purpose of tagging the branch? Bookkeeping in case you look it up later? Personally I would just look at my closed CLs on the codereview site, but I can understand if you want this.
https://codereview.chromium.org/1991563005/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode3123 git_cl.py:3123: branches = [c.GetBranch() for c in changes] On 2016/05/20 03:11:55, martiniss wrote: > Any reason to not combine this and the previous line? No reason other than avoiding nested list comprehension. But if you're fine with it, I'm fine with it. Done. https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode3148 git_cl.py:3148: RunGit(['tag', tagname, branch]) On 2016/05/20 03:11:55, martiniss wrote: > What is the purpose of tagging the branch? Bookkeeping in case you look it up > later? > > Personally I would just look at my closed CLs on the codereview site, but I can > understand if you want this. The tags allow you to access your local Git commit history, which isn't propagated when you upload your patches or land your code.
(Stephen, thanks to referring this CL to me!) high level comments: 1. There have been a CL to refactor the git_cl_statuses() https://codereview.chromium.org/1893563002/ which I didn't have time to land yet (traveling). I really think it should first - i think it's ready. 2. We have a tool called "git rebase-update" (it has doc!) which removes branches that have been merged into their tracking branches. In some ways that's better than just checking for just closed, but setting tags could be quite handy at the cost of no GC. Overall, I think this tool deserves to live in depot_tools, and rebase-update should be made less surprising and focused on its one thing - rebasing. So, SGTM. 3. Add test in tests/git_cl_test.py. Also, please don't include Rietveld in description. All code here as I see works for both Gerrit & Rietveld. https://codereview.chromium.org/1991563005/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode2967 git_cl.py:2967: return are you sure this works? I thought return can't be inside a function that uses yield. You have to use raise StopIteration instead. https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode3105 git_cl.py:3105: '-y', action='store_true', dest='no_prompt', default=False, -f, --force to be inline with other cmds. https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode3116 git_cl.py:3116: print('No local branch found.') nit: why parenthesis here and not below?
https://codereview.chromium.org/1991563005/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode2967 git_cl.py:2967: return On 2016/05/20 20:56:19, tandrii(chromium)slowTillMay31 wrote: > are you sure this works? I thought return can't be inside a function that uses > yield. You have to use raise StopIteration instead. "return" from a generator raises a StopIteration. https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode3105 git_cl.py:3105: '-y', action='store_true', dest='no_prompt', default=False, On 2016/05/20 20:56:19, tandrii(chromium)slowTillMay31 wrote: > -f, --force to be inline with other cmds. Done. https://codereview.chromium.org/1991563005/diff/1/git_cl.py#newcode3116 git_cl.py:3116: print('No local branch found.') On 2016/05/20 20:56:19, tandrii(chromium)slowTillMay31 wrote: > nit: why parenthesis here and not below? C++ habits, I guess. Done.
I'm happy you're on board with the concept!
Kevin, can you rebase (non-trivial) on top of this https://codereview.chromium.org/1893563002/ (just landed)? I think your CL should get simpler as a result.
Rebase done!
thanks, it looks very good. A bunch of comments inline. https://codereview.chromium.org/1991563005/diff/60001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3093 git_cl.py:3093: parser.error('Unsupported args: %s' % args) parser.error('Unsupported args: %s' % ' '.join(args)) https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3109 git_cl.py:3109: 'submitted-%s-%s' % (cl.GetBranch(), cl.GetIssue())) WDYT about "git-cl-closed-%s-%s"? This way it's much less likely to conflict with tags from upstream, and also less confusing, as closed != submitted (or landed in Rietveld jargon). https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3114 git_cl.py:3114: print 'No closed branches found.' WDYT about 'No branches with closed codereview issues found'? https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3118 git_cl.py:3118: print "%*s | %s" % (alignment, "Branch name", "Archival tag name") nits: single quotes (s/"/') https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3124 git_cl.py:3124: prompt = raw_input('\nProceed with deletion (Y/N)? ') s/raw_input/ask_for_data and how about exiting immediately here if prompt didn't return "y": if not options.force: if ask_for_data(...).lower() != 'y': print 'Aborted.' return 1 for branch, tagname in proposal: ... https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3127 git_cl.py:3127: branch, tagname = next_item combine this and above: for branch, tagname in proposal: https://codereview.chromium.org/1991563005/diff/60001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1991563005/diff/60001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1465: ((['git', 'config', 'branch.foo.rietveldissue'],), '456'), can you change this call to return '' instead of 456, and then add ((['git', 'config', 'branch.foo.gerritissue'],), '456'), to simulate the Gerrit's case as well. https://codereview.chromium.org/1991563005/diff/60001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1485: git_cl.main(['cleanup', '-f']) assert this returns 0.
Also added logic to verify that the user is not currently on a closed branch, which would cause git branch deletion to fail, and a unit test for that check. https://codereview.chromium.org/1991563005/diff/60001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3093 git_cl.py:3093: parser.error('Unsupported args: %s' % args) On 2016/05/31 18:46:16, tandrii(chromium) wrote: > parser.error('Unsupported args: %s' % ' '.join(args)) Done. https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3109 git_cl.py:3109: 'submitted-%s-%s' % (cl.GetBranch(), cl.GetIssue())) On 2016/05/31 18:46:16, tandrii(chromium) wrote: > WDYT about "git-cl-closed-%s-%s"? > This way it's much less likely to conflict with tags from upstream, and also > less confusing, as closed != submitted (or landed in Rietveld jargon). Done. https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3114 git_cl.py:3114: print 'No closed branches found.' On 2016/05/31 18:46:16, tandrii(chromium) wrote: > WDYT about 'No branches with closed codereview issues found'? Done. https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3118 git_cl.py:3118: print "%*s | %s" % (alignment, "Branch name", "Archival tag name") On 2016/05/31 18:46:16, tandrii(chromium) wrote: > nits: single quotes (s/"/') Done. https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3124 git_cl.py:3124: prompt = raw_input('\nProceed with deletion (Y/N)? ') On 2016/05/31 18:46:16, tandrii(chromium) wrote: > s/raw_input/ask_for_data > > and how about exiting immediately here if prompt didn't return "y": > if not options.force: > if ask_for_data(...).lower() != 'y': > print 'Aborted.' > return 1 > > for branch, tagname in proposal: > ... That looks better, done. https://codereview.chromium.org/1991563005/diff/60001/git_cl.py#newcode3127 git_cl.py:3127: branch, tagname = next_item On 2016/05/31 18:46:16, tandrii(chromium) wrote: > combine this and above: > for branch, tagname in proposal: Done. https://codereview.chromium.org/1991563005/diff/60001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1991563005/diff/60001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1465: ((['git', 'config', 'branch.foo.rietveldissue'],), '456'), On 2016/05/31 18:46:16, tandrii(chromium) wrote: > can you change this call to return '' instead of 456, > and then add > > ((['git', 'config', 'branch.foo.gerritissue'],), '456'), > > to simulate the Gerrit's case as well. Done for a new branch "bar". https://codereview.chromium.org/1991563005/diff/60001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1485: git_cl.main(['cleanup', '-f']) On 2016/05/31 18:46:16, tandrii(chromium) wrote: > assert this returns 0. Done.
good idea for check of the current branch. https://codereview.chromium.org/1991563005/diff/80001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/80001/git_cl.py#newcode3117 git_cl.py:3117: current_branch = RunGit(['symbolic-ref', 'HEAD', '--short']).strip() use existing func: current_branch = GetCurrentBranch() https://codereview.chromium.org/1991563005/diff/80001/git_cl.py#newcode3120 git_cl.py:3120: if any(branch == current_branch for branch, _ in proposal): I'd print all the branches with closed issues first, and then do this check. WDYT? https://codereview.chromium.org/1991563005/diff/80001/git_cl.py#newcode3121 git_cl.py:3121: print 'You are currently using a branch associated with a closed' s/using a branch associated/on a branch %s associated and %s is current_branch. And I'd not print it above, as in normal case, it's not important to the command. https://codereview.chromium.org/1991563005/diff/80001/git_cl.py#newcode3122 git_cl.py:3122: print 'codereview issue, so cleanup cannot proceed. Please change to ' use 1 print statement (basically, call) and don't format this yourself: print ('You are currently using a branch associated with a closed ' 'codereview issue, so cleanup cannot proceed. .... '...')
https://codereview.chromium.org/1991563005/diff/80001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/80001/git_cl.py#newcode3117 git_cl.py:3117: current_branch = RunGit(['symbolic-ref', 'HEAD', '--short']).strip() On 2016/05/31 20:18:32, tandrii(chromium) wrote: > use existing func: > current_branch = GetCurrentBranch() Done. https://codereview.chromium.org/1991563005/diff/80001/git_cl.py#newcode3120 git_cl.py:3120: if any(branch == current_branch for branch, _ in proposal): On 2016/05/31 20:18:33, tandrii(chromium) wrote: > I'd print all the branches with closed issues first, and then do this check. > WDYT? Done. https://codereview.chromium.org/1991563005/diff/80001/git_cl.py#newcode3121 git_cl.py:3121: print 'You are currently using a branch associated with a closed' On 2016/05/31 20:18:33, tandrii(chromium) wrote: > s/using a branch associated/on a branch %s associated > > and %s is current_branch. And I'd not print it above, as in normal case, it's > not important to the command. Done. https://codereview.chromium.org/1991563005/diff/80001/git_cl.py#newcode3122 git_cl.py:3122: print 'codereview issue, so cleanup cannot proceed. Please change to ' On 2016/05/31 20:18:33, tandrii(chromium) wrote: > use 1 print statement (basically, call) and don't format this yourself: > > print ('You are currently using a branch associated with a closed ' > 'codereview issue, so cleanup cannot proceed. .... > '...') Done.
forgot to upload? :) and once you do, use --dry-run or hit CQ dry run manually here.
Sorry, thought the upload went through. :\ How's this?
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991563005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1991563005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "submitted-foo-bar-1568403002". R=martiniss@chromium.org ========== to ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-cleanup-foo-bar-1568403002". R=martiniss@chromium.org ==========
Description was changed from ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-cleanup-foo-bar-1568403002". R=martiniss@chromium.org ========== to ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-cleanup-1568403002-foo-bar". R=martiniss@chromium.org ==========
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991563005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1991563005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tandrii@chromium.org changed reviewers: + maruel@chromium.org
LGTM % very nitty nit And due to some coincidence I got to know that depot_tools has something hidden called "git-tree-prune" in git_utils folder. It's broken aom, but used to offer mostly overlapping functionality from git cl cleanup. My vote is to land git cl cleanup and get rid of git tree prune, as latter is untested and is clearly not widely used, as it's not in PATH of most devs. +maruel@ Any counter opinion? https://codereview.chromium.org/1991563005/diff/120001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3127 git_cl.py:3127: 'checkout another branch and run this command again.') % \ final nit: don't close ) here, then you don't need \ after %, and no need for parenthesis on around current_branch. 'checkout another branch and run this command again.' % current_branch)
Description was changed from ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-cleanup-1568403002-foo-bar". R=martiniss@chromium.org ========== to ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-cleanup-1568403002-foo-bar". BUG=616404 R=martiniss@chromium.org,tandrii@chromium.org ==========
fine with getting rid of git-tree-prune. I'm a bit confused about the purpose of this command vs git rebase-update, which accomplish more or less the same thing. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3085 git_cl.py:3085: remove empty line https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3087 git_cl.py:3087: '-f', action='store_true', dest='force', default=False, instead of dest='force', add '--force' remove default=False, default=None is good enough https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3098 git_cl.py:3098: print 'No local branch found.' Is that useful? Not sure it's worth printing anything. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3101 git_cl.py:3101: print 'Finding all branches associated with closed issues...' needed? https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3108 git_cl.py:3108: proposal = [(cl.GetBranch(), sort https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3113 git_cl.py:3113: if len(proposal) == 0: if not proposal: https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3114 git_cl.py:3114: print 'No branches with closed codereview issues found.' needed? https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3125 git_cl.py:3125: print ('You are currently on a branch \'%s\' which is associated with a ' no space between print and (), it's a function now. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3131 git_cl.py:3131: if not options.force: I don't see why this is useful. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3137 git_cl.py:3137: RunGit(['tag', tagname, branch]) Why create a tag? https://codereview.chromium.org/1991563005/diff/120001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1991563005/diff/120001/tests/git_cl_test.py#n... tests/git_cl_test.py:1399: #super tedious. oops
Thanks. > I'm a bit confused about the purpose of this command vs git rebase-update, which accomplish more or less the same thing. With Chromium, only the final landed state of the CL is retained in the git log. rebase-update, upon computing a null delta with a branch's upstream parent, will simply clobber that branch. All of the local commit history in that branch will be lost and garbage collected. This command preserves the local Git history for that branch by using tags, so nothing gets lost. Another possibility for the command name could be "archive", to emphasize the fact that this command retains commit history? https://codereview.chromium.org/1991563005/diff/120001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3085 git_cl.py:3085: On 2016/06/01 12:43:16, M-A Ruel wrote: > remove empty line Done. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3087 git_cl.py:3087: '-f', action='store_true', dest='force', default=False, On 2016/06/01 12:43:17, M-A Ruel wrote: > instead of dest='force', add '--force' > remove default=False, default=None is good enough Done. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3098 git_cl.py:3098: print 'No local branch found.' On 2016/06/01 12:43:16, M-A Ruel wrote: > Is that useful? Not sure it's worth printing anything. Done. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3101 git_cl.py:3101: print 'Finding all branches associated with closed issues...' On 2016/06/01 12:43:17, M-A Ruel wrote: > needed? I added this to alleviate itchy ctrl-C fingers. This command is run interactively, and it takes several seconds to get the statuses for all branches on a well-used Git checkout. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3108 git_cl.py:3108: proposal = [(cl.GetBranch(), On 2016/06/01 12:43:16, M-A Ruel wrote: > sort Done. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3113 git_cl.py:3113: if len(proposal) == 0: On 2016/06/01 12:43:16, M-A Ruel wrote: > if not proposal: Done. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3114 git_cl.py:3114: print 'No branches with closed codereview issues found.' On 2016/06/01 12:43:16, M-A Ruel wrote: > needed? I think so - the indirection from depending on CL status might make it unclear as to why running this command is a no-op for some users. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3125 git_cl.py:3125: print ('You are currently on a branch \'%s\' which is associated with a ' On 2016/06/01 12:43:17, M-A Ruel wrote: > no space between print and (), it's a function now. Done. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3127 git_cl.py:3127: 'checkout another branch and run this command again.') % \ On 2016/06/01 09:53:17, tandrii(chromium) wrote: > final nit: don't close ) here, then you don't need \ after %, > and no need for parenthesis on around current_branch. > > 'checkout another branch and run this command again.' % > current_branch) Done. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3131 git_cl.py:3131: if not options.force: On 2016/06/01 12:43:16, M-A Ruel wrote: > I don't see why this is useful. It makes this command suitable for running non-interactively e.g. as a command sequence in an alias, or as part of a bash script. Several other commands have a force option to skip the prompt, and for the reasons stated above, I think this switch is a painless way to enable other uses of this command. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3137 git_cl.py:3137: RunGit(['tag', tagname, branch]) On 2016/06/01 12:43:16, M-A Ruel wrote: > Why create a tag? It preserves the local commit Git history for the branch, which makes it easy to (e.g.) unearth useful commits that happened to be buried by subsequent commit churn. https://codereview.chromium.org/1991563005/diff/120001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1991563005/diff/120001/tests/git_cl_test.py#n... tests/git_cl_test.py:1399: #super tedious. On 2016/06/01 12:43:17, M-A Ruel wrote: > oops Done.
I think it'd prefer 'archive' to 'cleanup'. This commands doesn't cleanup, it creates tags, so archive is more appropriate.
Description was changed from ========== Add "cleanup" command to git_cl.py. Add "cleanup" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-cleanup-1568403002-foo-bar". BUG=616404 R=martiniss@chromium.org,tandrii@chromium.org ========== to ========== Add "archive" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-archived-1568403002-foo-bar". BUG=616404 R=martiniss@chromium.org,tandrii@chromium.org ==========
OK, it's "git cl archive" now. Please take a look.
https://codereview.chromium.org/1991563005/diff/120001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3125 git_cl.py:3125: print ('You are currently on a branch \'%s\' which is associated with a ' On 2016/06/01 12:43:17, M-A Ruel wrote: > no space between print and (), it's a function now. pedantic follow up: print is still a statement (easy proof: [print('')] gives syntax error. https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3137 git_cl.py:3137: RunGit(['tag', tagname, branch]) On 2016/06/01 17:49:04, Kevin M wrote: > On 2016/06/01 12:43:16, M-A Ruel wrote: > > Why create a tag? > > It preserves the local commit Git history for the branch, which makes it easy to > (e.g.) unearth useful commits that happened to be buried by subsequent commit > churn. Frankly, I don't think many devs would make use of these tags ever. However, consider the branch deleting behavior of "git rebase-update". It is badly surprising, even though it's actually quite useful and afaiu correct. Informally, I've heard of quite a few engineers not using it exactly because of this surprise. Heck, I was one of these engineers until I finally understood what it does. So, because of this, I think, creating a tag is that nice touch to keep our developers calm, albeit at the cost of no gc being possible. And that's why I ultimately support "git cl archive". https://codereview.chromium.org/1991563005/diff/180001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/180001/git_cl.py#newcode3110 git_cl.py:3110: proposal.sort(lambda x, y: x[0] > y[0]) no kwargs would be just fine for ASC order: proposal.sort()
https://codereview.chromium.org/1991563005/diff/180001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/180001/git_cl.py#newcode3110 git_cl.py:3110: proposal.sort(lambda x, y: x[0] > y[0]) On 2016/06/02 21:35:30, tandrii(chromium) wrote: > no kwargs would be just fine for ASC order: > proposal.sort() Uhh... how does Python know which tuple element I intended to use for comparison? O_o But it works, so OK.
On 2016/06/02 22:20:35, Kevin M wrote: > https://codereview.chromium.org/1991563005/diff/180001/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/1991563005/diff/180001/git_cl.py#newcode3110 > git_cl.py:3110: proposal.sort(lambda x, y: x[0] > y[0]) > On 2016/06/02 21:35:30, tandrii(chromium) wrote: > > no kwargs would be just fine for ASC order: > > proposal.sort() > > Uhh... how does Python know which tuple element I intended to use for > comparison? O_o > > But it works, so OK. assert (1,'whatever') < (2, False, "could be anything") and if you wanted to compare by other index first, then it's better to provide key func: sort(key=lambda t: t[1])
https://codereview.chromium.org/1991563005/diff/120001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3125 git_cl.py:3125: print ('You are currently on a branch \'%s\' which is associated with a ' On 2016/06/02 21:35:30, tandrii(chromium) wrote: > On 2016/06/01 12:43:17, M-A Ruel wrote: > > no space between print and (), it's a function now. > > pedantic follow up: print is still a statement (easy proof: [print('')] gives > syntax error. We'll eventually have to make this work in python 3, so I prefer to reduce the churn in the new code. https://codereview.chromium.org/1991563005/diff/200001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/200001/git_cl.py#newcode2950 git_cl.py:2950: if len(changes) == 0: if not changes: https://codereview.chromium.org/1991563005/diff/200001/git_cl.py#newcode3079 git_cl.py:3079: 2 empty lines between file level symbols https://codereview.chromium.org/1991563005/diff/200001/git_cl.py#newcode3141 git_cl.py:3141: 2 lines between file level symbols
https://codereview.chromium.org/1991563005/diff/120001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/120001/git_cl.py#newcode3125 git_cl.py:3125: print ('You are currently on a branch \'%s\' which is associated with a ' On 2016/06/03 13:06:39, M-A Ruel wrote: > On 2016/06/02 21:35:30, tandrii(chromium) wrote: > > On 2016/06/01 12:43:17, M-A Ruel wrote: > > > no space between print and (), it's a function now. > > > > pedantic follow up: print is still a statement (easy proof: [print('')] gives > > syntax error. > > We'll eventually have to make this work in python 3, so I prefer to reduce the > churn in the new code. Done. https://codereview.chromium.org/1991563005/diff/200001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1991563005/diff/200001/git_cl.py#newcode2950 git_cl.py:2950: if len(changes) == 0: On 2016/06/03 13:06:39, M-A Ruel wrote: > if not changes: Done. https://codereview.chromium.org/1991563005/diff/200001/git_cl.py#newcode3079 git_cl.py:3079: On 2016/06/03 13:06:39, M-A Ruel wrote: > 2 empty lines between file level symbols Done. https://codereview.chromium.org/1991563005/diff/200001/git_cl.py#newcode3141 git_cl.py:3141: On 2016/06/03 13:06:39, M-A Ruel wrote: > 2 lines between file level symbols Done.
lgtm
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/1991563005/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991563005/240001
Message was sent while issue was closed.
Description was changed from ========== Add "archive" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-archived-1568403002-foo-bar". BUG=616404 R=martiniss@chromium.org,tandrii@chromium.org ========== to ========== Add "archive" command to git_cl.py. This command archives branches whose Rietveldt status is closed by creating new Git tags for each of the branches' heads, and then deleting the branch. It automatically cleans up the clutter that accumulates over time in a long-lived Git checkout. For example, the branch "foo-bar" associated with the closed issue 1568403002 will be archived to the tag "git-cl-archived-1568403002-foo-bar". BUG=616404 R=martiniss@chromium.org,tandrii@chromium.org Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/3bff56bb499c27... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/3bff56bb499c27... |
