Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
938583002:
Make git-map-branches -vvv show CL status colors. (Closed)
|
Created:
5 years, 10 months ago by calamity Modified:
5 years, 9 months ago CC:
chromium-reviews, cmp-cc_chromium.org, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
tools Visibility:
Public. |
DescriptionMake git-map-branches -vvv show CL status colors.
This CL makes git-map-branches show CL status colors like git cl status
when -vvv is used. Statuses are fetched in parallel for speed.
BUG=379849
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=294414
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase, address comment, remove unused var #
Total comments: 8
Patch Set 3 : address_comments #
Total comments: 2
Patch Set 4 : address comments #Messages
Total messages: 29 (9 generated)
calamity@chromium.org changed reviewers: + iannucci@chromium.org
jsbell@chromium.org changed reviewers: + jsbell@chromium.org
drive-by lgtm Thanks for tackling this! https://codereview.chromium.org/938583002/diff/1/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/938583002/diff/1/git_map_branches.py#newcode132 git_map_branches.py:132: threads_left = len(self.__branches_info) Maybe `queue_length` or `branch_count` instead of `threads_left`? (To me at least, "xxxx_left" seems like a value that would shrink over time.) (Or maybe just remove the temp variable completely?)
https://codereview.chromium.org/938583002/diff/1/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/938583002/diff/1/git_map_branches.py#newcode132 git_map_branches.py:132: threads_left = len(self.__branches_info) On 2015/02/27 17:39:38, jsbell wrote: > Maybe `queue_length` or `branch_count` instead of `threads_left`? (To me at > least, "xxxx_left" seems like a value that would shrink over time.) > > (Or maybe just remove the temp variable completely?) Done.
mostly good, some bits. I see that you actually just copied the Queue junk from the other function, but it's pretty gross... if you want to take a stab at cleaning it, that would be cool. If not, then I understand as well :) https://codereview.chromium.org/938583002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/938583002/diff/20001/git_cl.py#newcode1344 git_cl.py:1344: If fast is false, this will spawn len(branches) number of threads and fetch I think this is a bit misleading (and 'fast' is a bad name for this). I would maybe make this option called 'fine_grained' and then: True: get fine-grained CL status from the server. False: simply indicate if there's a matching url for a given branch. Additionally, I would strongly consider using `multiprocessing.imap_unordered`, and then have an optionally adjustable number of workers. You can additionally use multiprocessing.pool.ThreadPool if you don't want to use actual processes for the parallel workers. https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... git_map_branches.py:119: self.__status_info = None why not default to `{}`? https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... git_map_branches.py:128: self.__status_info = {} then you can skip this https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... git_map_branches.py:255: if self.__status_info: and this would still work, but in general you wouldn't need the if statement at all
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/938583002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/938583002/diff/20001/git_cl.py#newcode1344 git_cl.py:1344: If fast is false, this will spawn len(branches) number of threads and fetch On 2015/03/03 01:00:14, iannucci wrote: > I think this is a bit misleading (and 'fast' is a bad name for this). I would > maybe make this option called 'fine_grained' and then: > True: get fine-grained CL status from the server. > False: simply indicate if there's a matching url for a given branch. > > Additionally, I would strongly consider using `multiprocessing.imap_unordered`, > and then have an optionally adjustable number of workers. > > You can additionally use multiprocessing.pool.ThreadPool if you don't want to > use actual processes for the parallel workers. Done. It spawns as many jobs as there are branches by default but that can be reduced via command line flag. ThreadPool is in python 3 I think. The python 2 backport isn't in third_party so... yeah. https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... git_map_branches.py:119: self.__status_info = None On 2015/03/03 01:00:14, iannucci wrote: > why not default to `{}`? Done. https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... git_map_branches.py:128: self.__status_info = {} On 2015/03/03 01:00:14, iannucci wrote: > then you can skip this Done. https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... git_map_branches.py:255: if self.__status_info: On 2015/03/03 01:00:14, iannucci wrote: > and this would still work, but in general you wouldn't need the if statement at > all Reverted to a verbosity check.
On 2015/03/03 04:58:12, calamity wrote: > https://codereview.chromium.org/938583002/diff/20001/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/938583002/diff/20001/git_cl.py#newcode1344 > git_cl.py:1344: If fast is false, this will spawn len(branches) number of > threads and fetch > On 2015/03/03 01:00:14, iannucci wrote: > > I think this is a bit misleading (and 'fast' is a bad name for this). I would > > maybe make this option called 'fine_grained' and then: > > True: get fine-grained CL status from the server. > > False: simply indicate if there's a matching url for a given branch. > > > > Additionally, I would strongly consider using > `multiprocessing.imap_unordered`, > > and then have an optionally adjustable number of workers. > > > > You can additionally use multiprocessing.pool.ThreadPool if you don't want to > > use actual processes for the parallel workers. > > Done. It spawns as many jobs as there are branches by default but that can be > reduced via command line flag. > > ThreadPool is in python 3 I think. The python 2 backport isn't in third_party > so... yeah. > There's actually a sort-of-undocumented version in py2: http://stackoverflow.com/questions/3033952/python-thread-pool-similar-to-the-... It actually works perfectly well too :) > https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py > File git_map_branches.py (right): > > https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... > git_map_branches.py:119: self.__status_info = None > On 2015/03/03 01:00:14, iannucci wrote: > > why not default to `{}`? > > Done. > > https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... > git_map_branches.py:128: self.__status_info = {} > On 2015/03/03 01:00:14, iannucci wrote: > > then you can skip this > > Done. > > https://codereview.chromium.org/938583002/diff/20001/git_map_branches.py#newc... > git_map_branches.py:255: if self.__status_info: > On 2015/03/03 01:00:14, iannucci wrote: > > and this would still work, but in general you wouldn't need the if statement > at > > all > > Reverted to a verbosity check.
lgtm https://codereview.chromium.org/938583002/diff/60001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/938583002/diff/60001/git_cl.py#newcode1442 git_cl.py:1442: tmp = {} can we rename this to e.g. branch_status or something?
Using ThreadPool. https://codereview.chromium.org/938583002/diff/60001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/938583002/diff/60001/git_cl.py#newcode1442 git_cl.py:1442: tmp = {} On 2015/03/11 20:18:46, iannucci wrote: > can we rename this to e.g. branch_status or something? Done.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/938583002/#ps80001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/938583002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 938583002-80001 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.00s) failed
Cloning into '/tmp/trialvJM0sy/__main__.GitCheckout.testAll/foo'...
done.
Switched to branch 'master'
Already on 'master'
.ECloning into '/tmp/trialvJM0sy/__main__.GitCheckout.testMove/foo'...
done.
.Cloning into '/tmp/trialvJM0sy/__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.717s
FAILED (errors=1)
Presubmit checks took 186.0s to calculate.
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/938583002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 938583002-80001 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/gclient_smoketest.py (60.48s) failed
Ffatal: reference is not a tree: fb11a694253b7cbb54f2862b53c89cff8bee2f76
EE..............................................
======================================================================
ERROR: testBlinkDEPSChangeUsingGit (__main__.BlinkDEPSTransitionSmokeTest)
Like testBlinkDEPSChangeUsingGclient, but move the main project using
----------------------------------------------------------------------
Traceback (most recent call last):
File "tests/gclient_smoketest.py", line 1630, in testBlinkDEPSChangeUsingGit
cwd=self.checkout_path)
File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
484, in check_call
check_call_out(args, **kwargs)
File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
478, in check_call_out
returncode, args, kwargs.get('cwd'), out[0], out[1])
CalledProcessError: Command git checkout -q
fb11a694253b7cbb54f2862b53c89cff8bee2f76 returned non-zero exit status 128 in
/tmp/trialYvZAMc/__main__.BlinkDEPSTransitionSmokeTest.testBlinkDEPSChangeUsingGit/src
======================================================================
ERROR: testBlinkLocalBranchesArePreserved
(__main__.BlinkDEPSTransitionSmokeTest)
Checks that the state of local git branches are effectively preserved
----------------------------------------------------------------------
Traceback (most recent call last):
File "tests/gclient_smoketest.py", line 1656, in
testBlinkLocalBranchesArePreserved
self.CheckStatusPreMergePoint()
File "tests/gclient_smoketest.py", line 1558, in CheckStatusPreMergePoint
self.blink), self.blink_git_url)
File "/b/infra_internal/commit_queue/workdir/tools/depot_tools/scm.py", line
121, in Capture
cwd=cwd, stderr=subprocess2.PIPE, env=env, **kwargs)
File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
515, in check_output
return check_call_out(args, stdout=PIPE, **kwargs)[0]
File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
475, in check_call_out
out, returncode = communicate(args, **kwargs)
File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
449, in communicate
proc = Popen(args, **kwargs)
File
"/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py", line
253, in __init__
% (str(e), kwargs.get('cwd'), args[0]))
OSError: Execution failed with error: [Errno 2] No such file or directory:
'/tmp/trialYvZAMc/__main__.BlinkDEPSTransitionSmokeTest.testBlinkLocalBranchesArePreserved/src/third_party/WebKit'.
Check that
/tmp/trialYvZAMc/__main__.BlinkDEPSTransitionSmokeTest.testBlinkLocalBranchesArePreserved/src/third_party/WebKit
or git exist and have execution permission.
======================================================================
FAIL: testBlinkDEPSChangeUsingGclient (__main__.BlinkDEPSTransitionSmokeTest)
Checks that {src,blink} repos are consistent when syncing going back and
----------------------------------------------------------------------
Traceback (most recent call last):
File "tests/gclient_smoketest.py", line 1601, in
testBlinkDEPSChangeUsingGclient
self.assertEqual(res[2], 0, 'DEPS change sync failed.')
AssertionError: DEPS change sync failed.
----------------------------------------------------------------------
Ran 49 tests in 60.286s
FAILED (failures=1, errors=2)
Presubmit checks took 189.4s to calculate.
On 2015/03/12 00:33:41, I haz the power (commit-bot) wrote:
> Presubmit check for 938583002-80001 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/gclient_smoketest.py (60.48s) failed
> Ffatal: reference is not a tree: fb11a694253b7cbb54f2862b53c89cff8bee2f76
> EE..............................................
> ======================================================================
> ERROR: testBlinkDEPSChangeUsingGit (__main__.BlinkDEPSTransitionSmokeTest)
> Like testBlinkDEPSChangeUsingGclient, but move the main project using
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "tests/gclient_smoketest.py", line 1630, in testBlinkDEPSChangeUsingGit
> cwd=self.checkout_path)
> File
> "/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py",
line
> 484, in check_call
> check_call_out(args, **kwargs)
> File
> "/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py",
line
> 478, in check_call_out
> returncode, args, kwargs.get('cwd'), out[0], out[1])
> CalledProcessError: Command git checkout -q
> fb11a694253b7cbb54f2862b53c89cff8bee2f76 returned non-zero exit status 128 in
>
/tmp/trialYvZAMc/__main__.BlinkDEPSTransitionSmokeTest.testBlinkDEPSChangeUsingGit/src
>
> ======================================================================
> ERROR: testBlinkLocalBranchesArePreserved
> (__main__.BlinkDEPSTransitionSmokeTest)
> Checks that the state of local git branches are effectively preserved
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "tests/gclient_smoketest.py", line 1656, in
> testBlinkLocalBranchesArePreserved
> self.CheckStatusPreMergePoint()
> File "tests/gclient_smoketest.py", line 1558, in CheckStatusPreMergePoint
> self.blink), self.blink_git_url)
> File "/b/infra_internal/commit_queue/workdir/tools/depot_tools/scm.py", line
> 121, in Capture
> cwd=cwd, stderr=subprocess2.PIPE, env=env, **kwargs)
> File
> "/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py",
line
> 515, in check_output
> return check_call_out(args, stdout=PIPE, **kwargs)[0]
> File
> "/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py",
line
> 475, in check_call_out
> out, returncode = communicate(args, **kwargs)
> File
> "/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py",
line
> 449, in communicate
> proc = Popen(args, **kwargs)
> File
> "/b/infra_internal/commit_queue/workdir/tools/depot_tools/subprocess2.py",
line
> 253, in __init__
> % (str(e), kwargs.get('cwd'), args[0]))
> OSError: Execution failed with error: [Errno 2] No such file or directory:
>
'/tmp/trialYvZAMc/__main__.BlinkDEPSTransitionSmokeTest.testBlinkLocalBranchesArePreserved/src/third_party/WebKit'.
> Check that
>
/tmp/trialYvZAMc/__main__.BlinkDEPSTransitionSmokeTest.testBlinkLocalBranchesArePreserved/src/third_party/WebKit
> or git exist and have execution permission.
>
> ======================================================================
> FAIL: testBlinkDEPSChangeUsingGclient (__main__.BlinkDEPSTransitionSmokeTest)
> Checks that {src,blink} repos are consistent when syncing going back and
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "tests/gclient_smoketest.py", line 1601, in
> testBlinkDEPSChangeUsingGclient
> self.assertEqual(res[2], 0, 'DEPS change sync failed.')
> AssertionError: DEPS change sync failed.
>
> ----------------------------------------------------------------------
> Ran 49 tests in 60.286s
>
> FAILED (failures=1, errors=2)
>
>
> Presubmit checks took 189.4s to calculate.
.... these tests are awful :(
3rd time's the charm?
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/938583002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=294414
Message was sent while issue was closed.
Wow, maybe you just have the magic touch.
Message was sent while issue was closed.
On 2015/03/12 00:50:55, calamity wrote: > Wow, maybe you just have the magic touch. fingers crossed the whole time. #truestory #realtalk
Message was sent while issue was closed.
On 2015/03/12 01:04:22, iannucci wrote: > On 2015/03/12 00:50:55, calamity wrote: > > Wow, maybe you just have the magic touch. > > fingers crossed the whole time. #truestory #realtalk Like I said I'm in this world to do something great so if don't mind let me
Message was sent while issue was closed.
On 2015/03/12 01:04:22, iannucci wrote: > On 2015/03/12 00:50:55, calamity wrote: > > Wow, maybe you just have the magic touch. > > fingers crossed the whole time. #truestory #realtalk Like I said I'm in this world to do something great so if don't mind let me
Message was sent while issue was closed.
On 2015/03/12 01:04:22, iannucci wrote: > On 2015/03/12 00:50:55, calamity wrote: > > Wow, maybe you just have the magic touch. > > fingers crossed the whole time. #truestory #realtalk Like I said I'm in this world to do something great so if don't mind let me |
