Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
799273003:
Add tools/find-commit-for-patch.py (Closed)
|
Created:
6 years ago by Jakob Kummerow Modified:
6 years ago Reviewers:
Michael Achenbach CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAdd tools/find-commit-for-patch.py
It searches for a commit that a given patch can cleanly be applied to.
Committed: https://crrev.com/01a0931728c08e0bcf410e4a66b1b19e08d39862
Cr-Commit-Position: refs/heads/master@{#25845}
Patch Set 1 #Patch Set 2 : fix 'new file' corner case #
Total comments: 15
Patch Set 3 : addressed comments #Messages
Total messages: 9 (2 generated)
jkummerow@chromium.org changed reviewers: + machenbach@chromium.org
PTAL. Might come in handy to have this (I've needed it recently).
lgtm with comments: The comments are just improvement suggestions and not required for correctness. You can ignore them if you like. Generally a script like that should rather live in depot_tools than in v8. Maybe we can keep that in mind. Possibly integrated into git_cl.py to work as a parameter to "git cl patch" or as a separate command like "git cl patch-base <issue number>". https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... File tools/find-commit-for-patch.py (right): https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:25: "--verbose", "-v", default=False, action="store_true", nit: Could use python's logging module. Set it to log level info if verbose is specified and use logging.info instead of print everywhere without the trailing "if verbose:". https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:35: if line.startswith("diff --git "): Remark (no action required): This scans all lines - not only headers. It wouldn't work if the text "diff --git" is included in the patched file's text that surrounds a diff location. E.g. a file with test data for this script :) https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:36: # diff --git a/src/objects.cc b/src/objects.cc The patch could have been made with the option --src-prefix or --dst-prefix (e.g. to be patched into chromium/src/v8 or something). This script wouldn't accept this. Maybe add a text to the assertion which explains the restriction. https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:58: output = subprocess.check_output(["git", "ls-tree", "-r", commit]) Possible improvement: This command accepts a list of paths. You could pass all the paths in "files". Then all output must correspond to a file in files. https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:61: words = line.split() nit: You can assign more explicitely: _, _, actual_hash, filename = line.split() https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:63: if filename in files: nit: possible optimization: expected_hash = files.get(filename) if expected_hash: ... https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:74: while limit > 0: nit: Maybe: for _ in range(limit):
Thanks for the review. I've addressed the small comments; I think for tools like this one bigger changes should be done lazily as needed. > Generally a script like that should rather live in depot_tools than in v8. Maybe > we can keep that in mind. Possibly integrated into git_cl.py to work as a > parameter to "git cl patch" or as a separate command like "git cl patch-base > <issue number>". Sure, this script is not V8 specific, but v8/tools/ is where I put the tools I use for working on V8. Feel free to move it to depot_tools and integrate it more deeply with git-cl -- I fully agree that this would be nice to have, but I'm not willing to spend time on it right now. (The ideal solution, IMHO, would be "git cl patch <issue-nr> --full-auto", in addition to the "patch-base" lookup tool you describe.) https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... File tools/find-commit-for-patch.py (right): https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:25: "--verbose", "-v", default=False, action="store_true", On 2014/12/16 10:29:54, Michael Achenbach wrote: > nit: Could use python's logging module. Set it to log level info if verbose is > specified and use logging.info instead of print everywhere without the trailing > "if verbose:". Acknowledged. IIUC this wouldn't provide observable improvement, though, so I'm too lazy to do the change. https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:35: if line.startswith("diff --git "): On 2014/12/16 10:29:54, Michael Achenbach wrote: > Remark (no action required): This scans all lines - not only headers. It > wouldn't work if the text "diff --git" is included in the patched file's text > that surrounds a diff location. E.g. a file with test data for this script :) Actually, I'm pretty sure it does work on all valid patch files, because the check is for "startswith", and modified and context lines would start with "+", "-", or " ". https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:36: # diff --git a/src/objects.cc b/src/objects.cc On 2014/12/16 10:29:54, Michael Achenbach wrote: > The patch could have been made with the option --src-prefix or --dst-prefix > (e.g. to be patched into chromium/src/v8 or something). This script wouldn't > accept this. Maybe add a text to the assertion which explains the restriction. I'd just say the first time someone encounters this limitation, they are welcome to fix it. The script works for patches generated by "git cl upload" as well as "git diff" (without special arguments). https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:58: output = subprocess.check_output(["git", "ls-tree", "-r", commit]) On 2014/12/16 10:29:54, Michael Achenbach wrote: > Possible improvement: This command accepts a list of paths. You could pass all > the paths in "files". Then all output must correspond to a file in files. Done -- good idea! https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:61: words = line.split() On 2014/12/16 10:29:54, Michael Achenbach wrote: > nit: You can assign more explicitely: > _, _, actual_hash, filename = line.split() Done. https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:63: if filename in files: On 2014/12/16 10:29:54, Michael Achenbach wrote: > nit: possible optimization: > expected_hash = files.get(filename) > if expected_hash: > ... Acknowledged, but after your comment above, we don't need any check here any more (filename is guaranteed to be in files). https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:74: while limit > 0: On 2014/12/16 10:29:54, Michael Achenbach wrote: > nit: Maybe: for _ in range(limit): Done.
lgtm https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... File tools/find-commit-for-patch.py (right): https://codereview.chromium.org/799273003/diff/20001/tools/find-commit-for-pa... tools/find-commit-for-patch.py:35: if line.startswith("diff --git "): On 2014/12/16 13:51:31, Jakob wrote: > On 2014/12/16 10:29:54, Michael Achenbach wrote: > > Remark (no action required): This scans all lines - not only headers. It > > wouldn't work if the text "diff --git" is included in the patched file's text > > that surrounds a diff location. E.g. a file with test data for this script :) > > Actually, I'm pretty sure it does work on all valid patch files, because the > check is for "startswith", and modified and context lines would start with "+", > "-", or " ". Ah - I didn't see the space in the beginning of the context lines... then it's alright.
The CQ bit was checked by jkummerow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/799273003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/01a0931728c08e0bcf410e4a66b1b19e08d39862 Cr-Commit-Position: refs/heads/master@{#25845} |
