Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127633003:
Implement git cl set-commit --dry-run for Rietveld. (Closed)
|
Created:
4 years, 5 months ago by tandrii(chromium) Modified:
4 years, 5 months ago CC:
chromium-reviews, tandrii+omg_git_cl_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:
depot_tools Visibility:
Public. |
DescriptionImplement git cl set-commit --dry-run for Rietveld.
BUG=622828
R=emso@chromium.org,machenbach@chromium.org
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/4b233bdb48571ab5b4f4567f3cee7a4dc28bb93c
Patch Set 1 #Patch Set 2 : really dry #
Total comments: 4
Patch Set 3 : review #
Total comments: 1
Patch Set 4 : be consistent #Messages
Total messages: 26 (12 generated)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement git cl set-commit --dry-run for Rietveld. BUG=622828 R=emsso@chromium.org,machenbach@chromium.org ========== to ========== Implement git cl set-commit --dry-run for Rietveld. BUG=622828 R=emso@chromium.org,machenbach@chromium.org ==========
tandrii@chromium.org changed reviewers: + emso@chromium.org - emsso@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
lgtm with suggestions https://codereview.chromium.org/2127633003/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2127633003/diff/20001/git_cl.py#newcode1743 git_cl.py:1743: """Patchset must match.""" fly-by nit: This is not a method comment, rather a line comment. Maybe add a useful method comment? https://codereview.chromium.org/2127633003/diff/20001/git_cl.py#newcode1794 git_cl.py:1794: if new_state == _CQState.COMMIT: The dry_run flag also changes the commit flag, but not vice-versa. Isn't that somewhat inconsistent? I assume there's logic on the other side that takes care of this. I'd find it more consistent if either dry-run only sets the dry-run flag, of if commit and none also unset the dry-run flag. Another suggestion: How about a tri-state? commit 0-1-2, where 2 is dry-run? That'd render the need for a flags method unnecessary.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
https://codereview.chromium.org/2127633003/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2127633003/diff/20001/git_cl.py#newcode1743 git_cl.py:1743: """Patchset must match.""" On 2016/07/06 07:47:59, Michael Achenbach wrote: > fly-by nit: This is not a method comment, rather a line comment. Maybe add a > useful method comment? Done. https://codereview.chromium.org/2127633003/diff/20001/git_cl.py#newcode1794 git_cl.py:1794: if new_state == _CQState.COMMIT: On 2016/07/06 07:47:58, Michael Achenbach wrote: > The dry_run flag also changes the commit flag, but not vice-versa. Isn't that > somewhat inconsistent? I assume there's logic on the other side that takes care > of this. I'd find it more consistent if either dry-run only sets the dry-run > flag, of if commit and none also unset the dry-run flag. > > Another suggestion: How about a tri-state? commit 0-1-2, where 2 is dry-run? > That'd render the need for a flags method unnecessary. well, this is rietveld API, and I'm not going to touch it. In Gerrit, I actually use 0-1-2 semantic.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2127633003/#ps60001 (title: "be consistent")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2127633003/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2127633003/diff/40001/git_cl.py#newcode1800 git_cl.py:1800: self.SetFlag('commit', '0') indeed, let's make this erase both flags.
Message was sent while issue was closed.
Description was changed from ========== Implement git cl set-commit --dry-run for Rietveld. BUG=622828 R=emso@chromium.org,machenbach@chromium.org ========== to ========== Implement git cl set-commit --dry-run for Rietveld. BUG=622828 R=emso@chromium.org,machenbach@chromium.org Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/4b233bdb48571a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/4b233bdb48571a... |
