Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1893563002:
Use CLs more consistently instead of branch names (Closed)
|
Created:
4 years, 8 months ago by Clemens Hammacher 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. |
DescriptionUse CLs more consistently instead of branch names
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300132
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300673
Patch Set 1 #
Total comments: 7
Patch Set 2 : address tandrii's comments #
Total comments: 4
Patch Set 3 : remove unneeded auth_config #Patch Set 4 : fix git map-branches -vv #
Total comments: 5
Patch Set 5 : update again #Patch Set 6 : fix #
Total comments: 2
Patch Set 7 : last changes #Messages
Total messages: 48 (19 generated)
The CQ bit was checked by clemensh@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/1893563002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1893563002/1
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.
clemensh@chromium.org changed reviewers: + tandrii@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/1893563002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1893563002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks for awesome CL! https://codereview.chromium.org/1893563002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1893563002/diff/1/git_cl.py#newcode2897 git_cl.py:2897: def fetch_cl_status(cl, auth_config=None): auth_config isn't needed any more. https://codereview.chromium.org/1893563002/diff/1/git_cl.py#newcode2904 git_cl.py:2904: url += ' (broken)' i think with your awesome refactoring, we can go further and remove this function. cl.GetIssueUrl() doesn't block, and can be computed on the fly in status_str (line 3117). The logic for " (broken)" can be moved there too. then the only remaining bit here is just calling cl.GetStatus(), which can be used directly in line 2926. https://codereview.chromium.org/1893563002/diff/1/git_cl.py#newcode2926 git_cl.py:2926: fetch = lambda cl: fetch_cl_status(cl, auth_config=auth_config) so based on above comment, i propose to change this to: fetch = lambda cl: (cl, cl.GetStatus()) https://codereview.chromium.org/1893563002/diff/1/git_cl.py#newcode2939 git_cl.py:2939: cl = Changelist(branchref=c.GetBranch(), auth_config=auth_config) I think you can just use one from changes list, and if so, nit: s/c/cl https://codereview.chromium.org/1893563002/diff/1/git_cl.py#newcode2941 git_cl.py:2941: yield (c, url, 'waiting' if url else 'error') and based on refactoring suggestion for line 2926, this can become just: yield (cl, 'waiting' if cl.GetIssueURL() else 'error') https://codereview.chromium.org/1893563002/diff/1/git_cl.py#newcode3105 git_cl.py:3105: for cl in sorted(changes, key = lambda c : c.GetBranch()): nits: no space before ":" and no space around "=" for key-value arguments. => for cl in sorted(changes, key=lambda c: c.GetBranch()):
https://codereview.chromium.org/1893563002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1893563002/diff/1/git_cl.py#newcode2904 git_cl.py:2904: url += ' (broken)' On 2016/04/15 at 15:38:35, tandrii(chromium) wrote: > i think with your awesome refactoring, we can go further and remove this function. > > cl.GetIssueUrl() doesn't block, and can be computed on the fly in status_str (line 3117). The logic for " (broken)" can be moved there too. > > then the only remaining bit here is just calling cl.GetStatus(), which can be used directly in line 2926. Removing this function would conflict with this other CL: https://codereview.chromium.org/1895463002 I will address the other comments though.
Description was changed from ========== Use CLs more consistently instead of branch names BUG= ========== to ========== Use CLs more consistently instead of branch names ==========
The CQ bit was checked by clemensh@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/1893563002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1893563002/20001
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.
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/1893563002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1893563002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
LGTM % remove unused auth_config Thanks! https://codereview.chromium.org/1893563002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1893563002/diff/20001/git_cl.py#newcode2901 git_cl.py:2901: changes, fine_grained, max_processes=None, auth_config=None): remove unused auth_config kwarg. https://codereview.chromium.org/1893563002/diff/20001/git_cl.py#newcode3091 git_cl.py:3091: auth_config=auth_config) (cont) and this kwarg is not needed.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/1893563002/#ps40001 (title: "remove unneeded auth_config")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1893563002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1893563002/40001
https://codereview.chromium.org/1893563002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1893563002/diff/20001/git_cl.py#newcode2901 git_cl.py:2901: changes, fine_grained, max_processes=None, auth_config=None): On 2016/04/24 at 18:33:12, tandrii(chromium) wrote: > remove unused auth_config kwarg. done. https://codereview.chromium.org/1893563002/diff/20001/git_cl.py#newcode3091 git_cl.py:3091: auth_config=auth_config) On 2016/04/24 at 18:33:12, tandrii(chromium) wrote: > (cont) and this kwarg is not needed. done.
Message was sent while issue was closed.
Description was changed from ========== Use CLs more consistently instead of branch names ========== to ========== Use CLs more consistently instead of branch names Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300132 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300132
Message was sent while issue was closed.
According to git bisect, this broke: git map-branches -vv AttributeError: 'str' object has no attribute 'GetIssueURL'
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1913923002/ by tandrii@chromium.org. The reason for reverting is: speculative revert..
The new patch also fixes git map-branches -vv. PTAL.
The CQ bit was checked by clemensh@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/1893563002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1893563002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1893563002/diff/60001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1893563002/diff/60001/git_cl.py#newcode2164 git_cl.py:2164: data = self._GetChangeDetail(['CURRENT_REVISION']) argh, rebase artifact. https://codereview.chromium.org/1893563002/diff/60001/git_cl.py#newcode2904 git_cl.py:2904: """Returns a blocking iterable of (cl, status) for given branches. s/for given branches/for given changelists https://codereview.chromium.org/1893563002/diff/60001/git_cl.py#newcode2910 git_cl.py:2910: to spawn to fetch CL status from the server. Otherwise 1 process per branch is s/branch/Changelist https://codereview.chromium.org/1893563002/diff/60001/git_cl.py#newcode2917 git_cl.py:2917: if type(c) is str else c for c in changes] Let's keep get_cl_statuses() API clear: changes is list is of changelists, not branches. And let BranchMapper comply with it by creating the list. Which means same as your PS#3 + 2 line change in map branches. https://codereview.chromium.org/1893563002/diff/60001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/1893563002/diff/60001/git_map_branches.py#new... git_map_branches.py:141: self.__status_info[cl.GetBranchRef()] = (cl.GetIssueURL(), It used to be called 'branch' not branchref. Are you sure it was semantically branchref?
LGTM % (nit + suggestion) and Thanks! Clemens, please send a PTAL or otherwise ping reviewer (me) once you upload next revision that is ready for review. https://codereview.chromium.org/1893563002/diff/100001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1893563002/diff/100001/git_cl.py#newcode2922 git_cl.py:2922: def get_cl_statuses( nit: merge two lines. https://codereview.chromium.org/1893563002/diff/100001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/1893563002/diff/100001/git_map_branches.py#ne... git_map_branches.py:139: for _ in xrange(len(self.__branches_info)): for _ in self.__branches_info: would work just fine. I think even simpler one below works: for cl, status in status_info: ....
friendly ping :)
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/1893563002/#ps120001 (title: "last changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1893563002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1893563002/120001
Message was sent while issue was closed.
Description was changed from ========== Use CLs more consistently instead of branch names Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300132 ========== to ========== Use CLs more consistently instead of branch names Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300132 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300673 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300673
Message was sent while issue was closed.
sky@chromium.org changed reviewers: + sky@chromium.org
Message was sent while issue was closed.
I suspect this broke git-tree-prune
Message was sent while issue was closed.
On 2016/05/31 21:45:54, sky wrote: > I suspect this broke git-tree-prune What is git-tree-prune, and how does it break?
Message was sent while issue was closed.
On 2016/06/01 09:29:56, Clemens Hammacher wrote: > On 2016/05/31 21:45:54, sky wrote: > > I suspect this broke git-tree-prune > > What is git-tree-prune, and how does it break? So, i filed a bug for this: http://crbug.com/616404 |
