Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1033043002:
Script to determine in what branch a commit was merged into (Closed)
|
Created:
5 years, 9 months ago by Michael Hablich Modified:
5 years, 8 months ago Reviewers:
Michael Achenbach CC:
v8-dev, tandrii(chromium) Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Descriptionusage: Tool to check where a git commit was merged and reverted.
[-h] [-g GIT_DIR] hash
positional arguments:
hash Hash of the commit to be searched.
optional arguments:
-h, --help show this help message and exit
-g GIT_DIR, --git-dir GIT_DIR
The path to your git working directory.
BUG=
Committed: https://crrev.com/ef7e6fb1654399c137c583b86170645ae29aa6f1
Cr-Commit-Position: refs/heads/master@{#27563}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Made some changes proposed by machenbach #
Total comments: 2
Patch Set 3 : Added the requested space #Messages
Total messages: 19 (3 generated)
hablich@chromium.org changed reviewers: + machenbach@chromium.org, tandrii@chromium.org
https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:2: # Copyright 2013 the V8 project authors. All rights reserved. Please use short license header, see newer files, e.g. create_release.py with 2015. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:35: def printAnalysis(gitWorkingDir, hashToSearch): nit: as it is a new file rather use pythonish method names, e.g.: print_analysis and git_execute https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:37: gitExecute(gitWorkingDir, ['status']) I'd make the api of this tool more concise. Just search for the cherry-pick commits and nothing else - e.g. no show and no status, etc. Then it's easier to use the tool from other tools and consume the output. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:39: gitExecute(gitWorkingDir, ['pull']) I wouldn't let the script attempt to pull as pull merges to the current branch. Rather use fetch. Or even better, assume the checkout is in an up-to-date state. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:45: gitExecute(gitWorkingDir, ["log",'--all', '--grep='+hashToSearch]) Add --format="%H" so that only the hashes are returned. Grepping for the hash will lead to some false positives e.g. commits that revert the original commit and mention it in the commit message. Rather search for the exact expression "Merged deadbeef" log searches only on the current HEAD IIRC. I think you need something like rev-list - not sure right now. rev-list only returns hashes which you could then use in another command. Or use log on all the heads and branch-heads. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:54: parser.add_argument("-g", "--git-dir", required=True, This should be optional and just work for the current git checkout in which the script lives. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:57: parser.add_argument("--hash", help="Hash of the commit to be searched.", required=True) I'd make this a parameterless argument so that the tool can be called: mergeinfo.py deadbeef
Made some changes proposed by machenbach
Please provide arguments to the comments that weren't resolved (e.g. the ambiguity of grepping for a hash). I can also further explain my point-of-views if some are unclear. https://codereview.chromium.org/1033043002/diff/20001/tools/release/mergeinfo.py File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1033043002/diff/20001/tools/release/mergeinfo... tools/release/mergeinfo.py:32: parser.add_argument('hash',nargs=1, help="Hash of the commit to be searched.") nit: space after comma
https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:45: gitExecute(gitWorkingDir, ["log",'--all', '--grep='+hashToSearch]) Ignore my comment about log seaching in HEADS. You added --all which does the trick...
Here is an example about ambiguous output: git log --all --grep=f86aadd1d45c756467dff8e08a055b462d7a060b
On 2015/03/27 11:03:06, Michael Achenbach (travelling) wrote: > Here is an example about ambiguous output: > git log --all --grep=f86aadd1d45c756467dff8e08a055b462d7a060b I have commented everyone of your comments. Hmm looks fine for me. Finds the commits related to the provided hash. IMO reverts and relands are also very important. Without them you don't know if the content of the CL is IN or OUT. Additionally I am ok with one more commit found than one to few.
https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:2: # Copyright 2013 the V8 project authors. All rights reserved. On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > Please use short license header, see newer files, e.g. create_release.py with > 2015. Done. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:35: def printAnalysis(gitWorkingDir, hashToSearch): On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > nit: as it is a new file rather use pythonish method names, e.g.: > print_analysis and git_execute Acknowledged. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:37: gitExecute(gitWorkingDir, ['status']) On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > I'd make the api of this tool more concise. Just search for the cherry-pick > commits and nothing else - e.g. no show and no status, etc. Then it's easier to > use the tool from other tools and consume the output. Not meant for consumption by other tools. For now it should show you every information it can find on a hash. And reverts are also very interesting. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:39: gitExecute(gitWorkingDir, ['pull']) On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > I wouldn't let the script attempt to pull as pull merges to the current branch. > Rather use fetch. Or even better, assume the checkout is in an up-to-date state. Acknowledged. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:45: gitExecute(gitWorkingDir, ["log",'--all', '--grep='+hashToSearch]) On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > Add --format="%H" so that only the hashes are returned. > > Grepping for the hash will lead to some false positives e.g. commits that revert > the original commit and mention it in the commit message. Rather search for the > exact expression "Merged deadbeef" > > log searches only on the current HEAD IIRC. I think you need something like > rev-list - not sure right now. rev-list only returns hashes which you could then > use in another command. Or use log on all the heads and branch-heads. It should be human readable. I like it more verbose. The parameter --all searches also the refs. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:54: parser.add_argument("-g", "--git-dir", required=True, On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > This should be optional and just work for the current git checkout in which the > script lives. Acknowledged. https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... tools/release/mergeinfo.py:57: parser.add_argument("--hash", help="Hash of the commit to be searched.", required=True) On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > I'd make this a parameterless argument so that the tool can be called: > mergeinfo.py deadbeef Acknowledged. https://codereview.chromium.org/1033043002/diff/20001/tools/release/mergeinfo.py File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1033043002/diff/20001/tools/release/mergeinfo... tools/release/mergeinfo.py:32: parser.add_argument('hash',nargs=1, help="Hash of the commit to be searched.") On 2015/03/27 10:55:01, Michael Achenbach (travelling) wrote: > nit: space after comma Done.
On 2015/03/27 11:36:40, Hablich wrote: > https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py > File tools/release/mergeinfo.py (right): > > https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... > tools/release/mergeinfo.py:2: # Copyright 2013 the V8 project authors. All > rights reserved. > On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > > Please use short license header, see newer files, e.g. create_release.py with > > 2015. > > Done. > > https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... > tools/release/mergeinfo.py:35: def printAnalysis(gitWorkingDir, hashToSearch): > On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > > nit: as it is a new file rather use pythonish method names, e.g.: > > print_analysis and git_execute > > Acknowledged. > > https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... > tools/release/mergeinfo.py:37: gitExecute(gitWorkingDir, ['status']) > On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > > I'd make the api of this tool more concise. Just search for the cherry-pick > > commits and nothing else - e.g. no show and no status, etc. Then it's easier > to > > use the tool from other tools and consume the output. > > Not meant for consumption by other tools. For now it should show you every > information it can find on a hash. And reverts are also very interesting. > > https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... > tools/release/mergeinfo.py:39: gitExecute(gitWorkingDir, ['pull']) > On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > > I wouldn't let the script attempt to pull as pull merges to the current > branch. > > Rather use fetch. Or even better, assume the checkout is in an up-to-date > state. > > Acknowledged. > > https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... > tools/release/mergeinfo.py:45: gitExecute(gitWorkingDir, ["log",'--all', > '--grep='+hashToSearch]) > On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > > Add --format="%H" so that only the hashes are returned. > > > > Grepping for the hash will lead to some false positives e.g. commits that > revert > > the original commit and mention it in the commit message. Rather search for > the > > exact expression "Merged deadbeef" > > > > log searches only on the current HEAD IIRC. I think you need something like > > rev-list - not sure right now. rev-list only returns hashes which you could > then > > use in another command. Or use log on all the heads and branch-heads. > > It should be human readable. I like it more verbose. The parameter --all > searches also the refs. > > https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... > tools/release/mergeinfo.py:54: parser.add_argument("-g", "--git-dir", > required=True, > On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > > This should be optional and just work for the current git checkout in which > the > > script lives. > > Acknowledged. > > https://codereview.chromium.org/1033043002/diff/1/tools/release/mergeinfo.py#... > tools/release/mergeinfo.py:57: parser.add_argument("--hash", help="Hash of the > commit to be searched.", required=True) > On 2015/03/26 19:31:00, Michael Achenbach (travelling) wrote: > > I'd make this a parameterless argument so that the tool can be called: > > mergeinfo.py deadbeef > > Acknowledged. > > https://codereview.chromium.org/1033043002/diff/20001/tools/release/mergeinfo.py > File tools/release/mergeinfo.py (right): > > https://codereview.chromium.org/1033043002/diff/20001/tools/release/mergeinfo... > tools/release/mergeinfo.py:32: parser.add_argument('hash',nargs=1, help="Hash of > the commit to be searched.") > On 2015/03/27 10:55:01, Michael Achenbach (travelling) wrote: > > nit: space after comma > > Done. My fault. Forgot to publish the comments.
OK - the idea about the log lg. I'm not totally happy about the 'status' and 'show': 1) Any user could just type these commands manually easily, but has no choice to not use them when they run as default in this tool. 2) Both commands sometimes provide huge output (while log is rather constant). Status produces, e.g. lots of output when there are lots of untracked files, show also shows the patch which is sometimes huge. Why are these two commands useful when one wants to know where a patch got merged?
On 2015/03/30 19:13:56, Michael Achenbach (travelling) wrote: > OK - the idea about the log lg. I'm not totally happy about the 'status' and > 'show': > 1) Any user could just type these commands manually easily, but has no choice to > not use them when they run as default in this tool. > 2) Both commands sometimes provide huge output (while log is rather constant). > Status produces, e.g. lots of output when there are lots of untracked files, > show also shows the patch which is sometimes huge. > > Why are these two commands useful when one wants to know where a patch got > merged? The idea was that you can double check that you selected the correct hash. Although I get the point regarding the DIFFs. They might be too much.
LGTM - up to you to evaluate the usefulness of status and show - if it turns out not to be useful, you can change it in a follow up.
Ahh - please delete the whitespace in the CL description.
hablich@chromium.org changed reviewers: - tandrii@chromium.org
The CQ bit was checked by hablich@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1033043002/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/ef7e6fb1654399c137c583b86170645ae29aa6f1 Cr-Commit-Position: refs/heads/master@{#27563} |
