Add -B/--bucket flag to git-cl try

Add -B/--bucket flag to git-cl try

BUG=

Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/6c0efe6050d1579198c5cc2c7b9d8ff077dbe46f

[borenet, 2016-10-14 16:42:32]

borenet@google.com changed reviewers:
+ borenet@google.com, nodir@chromium.org, tandrii@chromium.org

[nodir, 2016-10-14 18:41:18]

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py#newcode4803
git_cl.py:4803: for bot in old_style:
let's not support old style stuff in new code paths

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py#newcode4850
git_cl.py:4850: bucket = ""
use ''

[borenet, 2016-10-14 18:46:42]

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py#newcode4803
git_cl.py:4803: for bot in old_style:
On 2016/10/14 18:41:18, nodir wrote:
> let's not support old style stuff in new code paths

Unfortunately the old code path goes through here as well, so I don't think I
can remove it without changing behavior.

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py#newcode4850
git_cl.py:4850: bucket = ""
On 2016/10/14 18:41:17, nodir wrote:
> use ''

Done.

[nodir, 2016-10-14 18:54:16]

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py#newcode4803
git_cl.py:4803: for bot in old_style:
On 2016/10/14 18:46:42, borenet wrote:
> On 2016/10/14 18:41:18, nodir wrote:
> > let's not support old style stuff in new code paths
> 
> Unfortunately the old code path goes through here as well, so I don't think I
> can remove it without changing behavior.

I think it is better copy-paste code that is needed for the new code path and
leave old code untouched. This way we will be able to delete old code later
without touching new code paths.

It is about 4 lines of copying and less branching

[tandrii(chromium), 2016-10-14 19:34:09]

(fast but incomplete review; will do better one next week)

https://codereview.chromium.org/2419113002/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/40001/git_cl.py#newcode382
git_cl.py:382: 'master': master,
how about not doing prefixing masters inside this method, but instead specify
already correct bucket names as input to this function. Then, this function
would merely check that bucket starts with 'master.' prefix and if so set this
property, otherwise no master property would get set.
Of course, complexity then will have to leave in CMDTry itself. However, I agree
with nodir: allow -B or -m flags, but not both, and copy-paste paths. WDYT?

[borenet, 2016-10-17 14:25:00]

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/20001/git_cl.py#newcode4803
git_cl.py:4803: for bot in old_style:
On 2016/10/14 18:54:16, nodir wrote:
> On 2016/10/14 18:46:42, borenet wrote:
> > On 2016/10/14 18:41:18, nodir wrote:
> > > let's not support old style stuff in new code paths
> > 
> > Unfortunately the old code path goes through here as well, so I don't think
I
> > can remove it without changing behavior.
> 
> I think it is better copy-paste code that is needed for the new code path and
> leave old code untouched. This way we will be able to delete old code later
> without touching new code paths.
> 
> It is about 4 lines of copying and less branching

Done. It seems that when using "-b <bot>" we get the old_style (non-tuple) path.
 Is that correct?

https://codereview.chromium.org/2419113002/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/40001/git_cl.py#newcode382
git_cl.py:382: 'master': master,
On 2016/10/14 19:34:08, tandrii(chromium) wrote:
> how about not doing prefixing masters inside this method, but instead specify
> already correct bucket names as input to this function. Then, this function
> would merely check that bucket starts with 'master.' prefix and if so set this
> property, otherwise no master property would get set.
> Of course, complexity then will have to leave in CMDTry itself. However, I
agree
> with nodir: allow -B or -m flags, but not both, and copy-paste paths. WDYT?

That's actually one of the changes that I made here: the "buckets" argument
consists of already-correct bucket names, and I added the _unprefix_master
function so that the "master" property didn't change.  Changed to only supply
the "master" property when bucket.startswith("master.").

[nodir, 2016-10-17 17:56:02]

lgtm % (comment, tandrii's review)

https://codereview.chromium.org/2419113002/diff/60001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/60001/git_cl.py#newcode4861
git_cl.py:4861: old_style = filter(lambda x: isinstance(x, basestring),
options.bot)
I think this is noop since options.bot cannot be tuples. The original code
checks this because of line L4821 which is not executed in this code path. 

then I think it would be simpler to do
buckets = {options.bucket: {b: [] for b in opions.bot}}

[borenet, 2016-10-17 17:59:45]

https://codereview.chromium.org/2419113002/diff/60001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/60001/git_cl.py#newcode4861
git_cl.py:4861: old_style = filter(lambda x: isinstance(x, basestring),
options.bot)
On 2016/10/17 17:56:02, nodir wrote:
> I think this is noop since options.bot cannot be tuples. The original code
> checks this because of line L4821 which is not executed in this code path. 
> 
> then I think it would be simpler to do
> buckets = {options.bucket: {b: [] for b in opions.bot}}

D'oh. Yeah, totally not needed when we're not mixing old/new formats.

[tandrii(chromium), 2016-10-18 21:45:38]

LGTM

Thanks a lot for making this.

https://codereview.chromium.org/2419113002/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/40001/git_cl.py#newcode382
git_cl.py:382: 'master': master,
On 2016/10/17 14:25:00, borenet wrote:
> On 2016/10/14 19:34:08, tandrii(chromium) wrote:
> > how about not doing prefixing masters inside this method, but instead
specify
> > already correct bucket names as input to this function. Then, this function
> > would merely check that bucket starts with 'master.' prefix and if so set
this
> > property, otherwise no master property would get set.
> > Of course, complexity then will have to leave in CMDTry itself. However, I
> agree
> > with nodir: allow -B or -m flags, but not both, and copy-paste paths. WDYT?
> 
> That's actually one of the changes that I made here: the "buckets" argument
> consists of already-correct bucket names, and I added the _unprefix_master
> function so that the "master" property didn't change.  Changed to only supply
> the "master" property when bucket.startswith("master.").

Acknowledged.

https://codereview.chromium.org/2419113002/diff/80001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/80001/git_cl.py#newcode406
git_cl.py:406: parameters['properties']['master'] = master
nit: weird indentation.
+1 for this conditional though! We need the same in CQ.

https://codereview.chromium.org/2419113002/diff/80001/git_cl.py#newcode4860
git_cl.py:4860: buckets = {options.bucket: {b: [] for b in options.bot}}
great, i wish the else below can be shortened to just CQ dry run, but that will
come later.

[borenet, 2016-10-19 15:10:16]

https://codereview.chromium.org/2419113002/diff/80001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2419113002/diff/80001/git_cl.py#newcode406
git_cl.py:406: parameters['properties']['master'] = master
On 2016/10/18 21:45:38, tandrii(chromium) wrote:
> nit: weird indentation.
> +1 for this conditional though! We need the same in CQ.

Fixed. Go has me instinctively hitting tab all the time.

[borenet, 2016-10-19 15:10:28]

The CQ bit was checked by borenet@google.com

[borenet, 2016-10-19 15:10:28]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org,
tandrii@chromium.org
Link to the patchset: https://codereview.chromium.org/2419113002/#ps100001
(title: "Fix indent")

[commit-bot: I haz the power, 2016-10-19 15:10:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 15:13:32]

Description was changed from

==========
Add -B/--bucket flag to git-cl try

BUG=
==========

to

==========
Add -B/--bucket flag to git-cl try

BUG=

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/6c0efe6050d157...
==========

[commit-bot: I haz the power, 2016-10-19 15:13:33]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/6c0efe6050d157...