Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2261833003:
Don't run git add/rm after rebaseline commands for rebaseline-cl. (Closed)
DescriptionDon't run git add/rm after rebaseline commands for rebaseline-cl.
BUG=639410, 474273
Committed: https://crrev.com/2dc14ad8012cd602f7cee0ecbb0f9be05ed3d76e
Cr-Commit-Position: refs/heads/master@{#413354}
Patch Set 1 #
Total comments: 7
Messages
Total messages: 13 (4 generated)
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org, wkorman@chromium.org
https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right): https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:449: # or making it work and produce reasonable results for rebaseline-cl. Some thoughts: maybe if all file additions and deletions can be kept in a list for the whole operation, including all commands, and then scm.delete_list / scm.add_list called once at the end of _rebaseline, then that would be best? It seems like in general it may be unnecessary to do explicitly stage file additions/deletions, but I think that auto-rebaseline might rely on it, since it commits after calling _rebaseline().
Description was changed from ========== Don't run git add/rm after rebaseline commands for rebaseline-cl. BUG=639410 ========== to ========== Don't run git add/rm after rebaseline commands for rebaseline-cl. BUG=639410,474273 ==========
https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right): https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:449: # or making it work and produce reasonable results for rebaseline-cl. On 2016/08/19 at 22:45:46, qyearsley wrote: > Some thoughts: maybe if all file additions and deletions can be kept in a list for the whole operation, including all commands, and then scm.delete_list / scm.add_list called once at the end of _rebaseline, then that would be best? > > It seems like in general it may be unnecessary to do explicitly stage file additions/deletions, but I think that auto-rebaseline might rely on it, since it commits after calling _rebaseline(). Yeah, auto-rebaseline has to commit so that it can upload the CL, right? So we need to at least preserve this behavior until we turn that down. It does seem like if we're planning to add a file, we shouldn't bother deleting it. So I'd be supportive of that change unless we think of something it would break. https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:450: if update_scm: How much of a pain to add unit test to make sure this new param does what we intend? https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py (right): https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:69: self._rebaseline(options, test_prefix_list, update_scm=False) So in terms of addressing the bug, after this change, what is the behavior? Nothing will be auto-staged for the developer, but they'll see newly pulled/optimized baselines in their directory and can manually add them? Just want to make sure I am following re: this patch and the bug.
https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right): https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:450: if update_scm: On 2016/08/19 at 22:54:07, wkorman wrote: > How much of a pain to add unit test to make sure this new param does what we intend? Hmm, maybe not too much. Possible test methods could: - Run rebaseline-cl to rebaseline a test (with mock data all set up) and verify that tool.scm().added_paths is empty. - Run another rebaseline command and verify that tool.scm().added_paths has the right files. Probably will want to modify MockSCM so that deleted_paths can be verified too. Although, on the other hand, I did try this out once manually by running webkit-patch rebaseline-cl, so at least in that instance I verified that it appears to do what I want it to. Maybe not worth it to add tests now if (1) it can be tested manually for each new CL and (2) this behavior is likely to change soon. https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py (right): https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:69: self._rebaseline(options, test_prefix_list, update_scm=False) On 2016/08/19 at 22:54:07, wkorman wrote: > So in terms of addressing the bug, after this change, what is the behavior? Nothing will be auto-staged for the developer, but they'll see newly pulled/optimized baselines in their directory and can manually add them? Just want to make sure I am following re: this patch and the bug. Right, exactly. Changes will be made but not staged. So, if you run webkit-patch rebaseline-cl and it modifies an existing CL, you can still commit your changes with `git commit -a`, but if you're adding a new baseline, you must `git add` the new file before committing. This may be an extra step, but it's less confusing than the current behavior, where `git status` shows a bunch of files being both added and deleted.
I defer to wkorman@ here.
lgtm https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right): https://codereview.chromium.org/2261833003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:450: if update_scm: On 2016/08/19 at 23:32:27, qyearsley wrote: > On 2016/08/19 at 22:54:07, wkorman wrote: > > How much of a pain to add unit test to make sure this new param does what we intend? > > Hmm, maybe not too much. Possible test methods could: > - Run rebaseline-cl to rebaseline a test (with mock data all set up) and verify that tool.scm().added_paths is empty. > - Run another rebaseline command and verify that tool.scm().added_paths has the right files. Probably will want to modify MockSCM so that deleted_paths can be verified too. > > Although, on the other hand, I did try this out once manually by running webkit-patch rebaseline-cl, so at least in that instance I verified that it appears to do what I want it to. > > Maybe not worth it to add tests now if (1) it can be tested manually for each new CL and (2) this behavior is likely to change soon. Am ok with waiting on test as we hope to turn auto-rebaseline down soon.
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Don't run git add/rm after rebaseline commands for rebaseline-cl. BUG=639410,474273 ========== to ========== Don't run git add/rm after rebaseline commands for rebaseline-cl. BUG=639410,474273 Committed: https://crrev.com/2dc14ad8012cd602f7cee0ecbb0f9be05ed3d76e Cr-Commit-Position: refs/heads/master@{#413354} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2dc14ad8012cd602f7cee0ecbb0f9be05ed3d76e Cr-Commit-Position: refs/heads/master@{#413354} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
