|
|
Created:
5 years, 3 months ago by Michael Hablich Modified:
5 years, 2 months ago Reviewers:
Michael Achenbach CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Release] Distinguish between merges and follow-up CLs
With this change mergeinfo.py gets more useful. There is
a distinction between merges and follow-up CLs/ports/reverts.
Also the information is show if the commit is already
rolled/an lkgr/deployed to Canary channel.
Additionally some formatting errors were resolved and
the already existing git wrapper used.
LOG=N
R=machenbach@chromium.org
NOTRY=true
Committed: https://crrev.com/8499fa2505f20622517f5be0f9e274a75fc00dcf
Cr-Commit-Position: refs/heads/master@{#31043}
Patch Set 1 #Patch Set 2 : Removed unused import #Patch Set 3 : Fixed search_related_commits.py and added initial test file #Patch Set 4 : First UnitTest #
Total comments: 18
Patch Set 5 : Proper check for merges, refactorings, unit tests #Patch Set 6 : Please review #
Total comments: 26
Patch Set 7 : Adressed comments #
Total comments: 1
Patch Set 8 : fixed nit #
Messages
Total messages: 13 (2 generated)
PTAL Ignore 'Depends on patchset'
https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo.py File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:12: def describe_commit(gitWorkingDir, hashToSearch, prettyPrint=True): nit: Still think our python style guide recommends _ names for variables not camel case... but if you insist, I don't care. https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:16: '--pretty=format:%H', Could you deduplicate this code? E.g. using something like pretty_flag = ' --pretty=format:%H' if prettyPrint else '' https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:26: raise Exception("NotImplemented") So: The non-pretty printed version wouldn't work yet? https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:28: '--grep='+hashToSearch, nit: indentation. https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:34: '--all', nit: indentation https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:68: parser = argparse.ArgumentParser('''Tool to check where a git commit was nit: I wouldn't use ''' as this will add the linebreak after 'was'. Rather two strings (if is doesn't fit), one in each line. E.g. parser = argparse.ArgumentParser( 'Tool to check where a git commit was ' 'merged and reverted.') https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... File tools/release/test_mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... tools/release/test_mergeinfo.py:7: from os import path nit: order https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... tools/release/test_mergeinfo.py:80: commits = self._execute_git( nit: remove local var? https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... tools/release/test_mergeinfo.py:120: False) Shoudn't this assert something?"
Thanks for the feedback. The idea evolved a 'little' bit so the uploaded version was not really meant to be released. Added tests etc. added to a lot more changes that need to be done. I will write into this CL when a new review pass is necessary. https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo.py File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:12: def describe_commit(gitWorkingDir, hashToSearch, prettyPrint=True): On 2015/09/17 14:19:00, Michael Achenbach wrote: > nit: Still think our python style guide recommends _ names for variables not > camel case... but if you insist, I don't care. I sticked to the style which was already present . Although I will rename them, no worries! https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:16: '--pretty=format:%H', On 2015/09/17 14:19:00, Michael Achenbach wrote: > Could you deduplicate this code? E.g. using something like > pretty_flag = ' --pretty=format:%H' if prettyPrint else '' Makes sense, will do. https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:26: raise Exception("NotImplemented") On 2015/09/17 14:19:00, Michael Achenbach wrote: > So: The non-pretty printed version wouldn't work yet? Well, it started out as an idea to simply add the new functionality. Than I wanted to add tests because there were none. In order to make the code better testable I noticed that testing 'prints' is not the right way ... essentially I am currently iterating to releasable version. That is also the reason why there are not enough tests yet etc. In the final version of this CL the NotImplemented will be implemented. https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:28: '--grep='+hashToSearch, On 2015/09/17 14:19:00, Michael Achenbach wrote: > nit: indentation. Acknowledged. https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:34: '--all', On 2015/09/17 14:19:00, Michael Achenbach wrote: > nit: indentation Acknowledged. https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... tools/release/mergeinfo.py:68: parser = argparse.ArgumentParser('''Tool to check where a git commit was On 2015/09/17 14:19:00, Michael Achenbach wrote: > nit: I wouldn't use ''' as this will add the linebreak after 'was'. Rather two > strings (if is doesn't fit), one in each line. E.g. > > parser = argparse.ArgumentParser( > 'Tool to check where a git commit was ' > 'merged and reverted.') Acknowledged. https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... File tools/release/test_mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... tools/release/test_mergeinfo.py:7: from os import path On 2015/09/17 14:19:00, Michael Achenbach wrote: > nit: order Acknowledged. https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... tools/release/test_mergeinfo.py:80: commits = self._execute_git( On 2015/09/17 14:19:00, Michael Achenbach wrote: > nit: remove local var? I think it is easier to debug that way. https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... tools/release/test_mergeinfo.py:120: False) On 2015/09/17 14:19:00, Michael Achenbach wrote: > Shoudn't this assert something?" Right. Currently I am in the process of writing the tests.
On 2015/09/18 07:38:47, Hablich wrote: > Thanks for the feedback. The idea evolved a 'little' bit so the uploaded version > was not really meant to be released. Added tests etc. added to a lot more > changes that need to be done. > > I will write into this CL when a new review pass is necessary. > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo.py > File tools/release/mergeinfo.py (right): > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > tools/release/mergeinfo.py:12: def describe_commit(gitWorkingDir, hashToSearch, > prettyPrint=True): > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > nit: Still think our python style guide recommends _ names for variables not > > camel case... but if you insist, I don't care. > > I sticked to the style which was already present . Although I will rename them, > no worries! > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > tools/release/mergeinfo.py:16: '--pretty=format:%H', > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > Could you deduplicate this code? E.g. using something like > > pretty_flag = ' --pretty=format:%H' if prettyPrint else '' > > Makes sense, will do. > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > tools/release/mergeinfo.py:26: raise Exception("NotImplemented") > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > So: The non-pretty printed version wouldn't work yet? > > Well, it started out as an idea to simply add the new functionality. Than I > wanted to add tests because there were none. In order to make the code better > testable I noticed that testing 'prints' is not the right way ... essentially I > am currently iterating to releasable version. That is also the reason why there > are not enough tests yet etc. > > In the final version of this CL the NotImplemented will be implemented. > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > tools/release/mergeinfo.py:28: '--grep='+hashToSearch, > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > nit: indentation. > > Acknowledged. > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > tools/release/mergeinfo.py:34: '--all', > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > nit: indentation > > Acknowledged. > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > tools/release/mergeinfo.py:68: parser = argparse.ArgumentParser('''Tool to check > where a git commit was > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > nit: I wouldn't use ''' as this will add the linebreak after 'was'. Rather two > > strings (if is doesn't fit), one in each line. E.g. > > > > parser = argparse.ArgumentParser( > > 'Tool to check where a git commit was ' > > 'merged and reverted.') > > Acknowledged. > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... > File tools/release/test_mergeinfo.py (right): > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... > tools/release/test_mergeinfo.py:7: from os import path > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > nit: order > > Acknowledged. > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... > tools/release/test_mergeinfo.py:80: commits = self._execute_git( > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > nit: remove local var? > > I think it is easier to debug that way. > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... > tools/release/test_mergeinfo.py:120: False) > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > Shoudn't this assert something?" > > Right. Currently I am in the process of writing the tests. PTAL at patchset 6.
On 2015/09/18 11:06:41, Hablich wrote: > On 2015/09/18 07:38:47, Hablich wrote: > > Thanks for the feedback. The idea evolved a 'little' bit so the uploaded > version > > was not really meant to be released. Added tests etc. added to a lot more > > changes that need to be done. > > > > I will write into this CL when a new review pass is necessary. > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo.py > > File tools/release/mergeinfo.py (right): > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > > tools/release/mergeinfo.py:12: def describe_commit(gitWorkingDir, > hashToSearch, > > prettyPrint=True): > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > nit: Still think our python style guide recommends _ names for variables not > > > camel case... but if you insist, I don't care. > > > > I sticked to the style which was already present . Although I will rename > them, > > no worries! > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > > tools/release/mergeinfo.py:16: '--pretty=format:%H', > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > Could you deduplicate this code? E.g. using something like > > > pretty_flag = ' --pretty=format:%H' if prettyPrint else '' > > > > Makes sense, will do. > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > > tools/release/mergeinfo.py:26: raise Exception("NotImplemented") > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > So: The non-pretty printed version wouldn't work yet? > > > > Well, it started out as an idea to simply add the new functionality. Than I > > wanted to add tests because there were none. In order to make the code better > > testable I noticed that testing 'prints' is not the right way ... essentially > I > > am currently iterating to releasable version. That is also the reason why > there > > are not enough tests yet etc. > > > > In the final version of this CL the NotImplemented will be implemented. > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > > tools/release/mergeinfo.py:28: '--grep='+hashToSearch, > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > nit: indentation. > > > > Acknowledged. > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > > tools/release/mergeinfo.py:34: '--all', > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > nit: indentation > > > > Acknowledged. > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/mergeinfo... > > tools/release/mergeinfo.py:68: parser = argparse.ArgumentParser('''Tool to > check > > where a git commit was > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > nit: I wouldn't use ''' as this will add the linebreak after 'was'. Rather > two > > > strings (if is doesn't fit), one in each line. E.g. > > > > > > parser = argparse.ArgumentParser( > > > 'Tool to check where a git commit was ' > > > 'merged and reverted.') > > > > Acknowledged. > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... > > File tools/release/test_mergeinfo.py (right): > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... > > tools/release/test_mergeinfo.py:7: from os import path > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > nit: order > > > > Acknowledged. > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... > > tools/release/test_mergeinfo.py:80: commits = self._execute_git( > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > nit: remove local var? > > > > I think it is easier to debug that way. > > > > > https://codereview.chromium.org/1341303002/diff/60001/tools/release/test_merg... > > tools/release/test_mergeinfo.py:120: False) > > On 2015/09/17 14:19:00, Michael Achenbach wrote: > > > Shoudn't this assert something?" > > > > Right. Currently I am in the process of writing the tests. > > PTAL at patchset 6. friendly ping for review
Optional: It might be more comprehensive if you once in your test setup method create one (slightly bigger) git test scenario that then is reused in all tests instead of creating different git state for each test. Then this scenario could be described once and all the commits could be named comprehensively. https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:29: '--grep='+hash_to_search, nit: space around + https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:29: '--grep='+hash_to_search, How often does it happen that developers use the full hash in a commit message to refer to another commit. And how often does it happen that they refer to another commit by other means, e.g. shortened hash, cr-commit-position, text of original cl, actual link of cl, etc? https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:37: return list(set(merges) - set(false_merges)) you might loose the original order while going through the sets https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:41: '--all', nit: indentation https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:56: branches = [ current.strip() for current in branches] 3 in 1: return map(str.strip, branches.splitlines()) https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:71: canaries.sort() or put sorted() around the call above... https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... File tools/release/test_mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:125: #This should be found nit: space after # also everywhere below https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:132: self._make_empty_commit(message) Suggestion: You could let _make_empty_commit return the hash of what just was created. In most of the cases you need an extra line for fetching it. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:142: self.assertEqual(len(followups), 0) I assume if you'd checked out a branch where a merge is on by any chance, you'd find the merge as a follow up. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:158: hash_of_first_commit = commits[0] The test (also for lkgr) would be more covering if roll (or lkgr) wouldn't point to the exact commit, but to some commit on top. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:159: self._execute_git(['branch', 'remotes/origin/roll']) Remember that the roll ref is just storing what should be rolled - not what is rolled. We don't have the latter (we only have the canary ref). https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:186: #import sys;sys.argv = ['', 'Test.testName'] nit: remove?
PTAL PS #7 https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:29: '--grep='+hash_to_search, On 2015/10/01 08:24:18, Michael Achenbach wrote: > nit: space around + Done. https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:29: '--grep='+hash_to_search, On 2015/10/01 08:24:18, Michael Achenbach wrote: > How often does it happen that developers use the full hash in a commit message > to refer to another commit. And how often does it happen that they refer to > another commit by other means, e.g. shortened hash, cr-commit-position, text of > original cl, actual link of cl, etc? As far as I have seen the most prominent form is referring to full commit hashes. The tools we are using for doing a revert and merge are also using the full commit hash. Our port suppliers do the same. It might make sense to invest in commit position and a reduced hash in future iterations. https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:37: return list(set(merges) - set(false_merges)) On 2015/10/01 08:24:18, Michael Achenbach wrote: > you might loose the original order while going through the sets Done. https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:41: '--all', On 2015/10/01 08:24:18, Michael Achenbach wrote: > nit: indentation Done. https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:56: branches = [ current.strip() for current in branches] On 2015/10/01 08:24:18, Michael Achenbach wrote: > 3 in 1: > return map(str.strip, branches.splitlines()) Did a mixture because I like it a little bit more verbose https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:71: canaries.sort() On 2015/10/01 08:24:18, Michael Achenbach wrote: > or put sorted() around the call above... I don't want to nest the list operation above even more. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... File tools/release/test_mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:125: #This should be found On 2015/10/01 08:24:18, Michael Achenbach wrote: > nit: space after # > also everywhere below Done. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:132: self._make_empty_commit(message) On 2015/10/01 08:24:18, Michael Achenbach wrote: > Suggestion: You could let _make_empty_commit return the hash of what just was > created. In most of the cases you need an extra line for fetching it. I like it. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:142: self.assertEqual(len(followups), 0) On 2015/10/01 08:24:18, Michael Achenbach wrote: > I assume if you'd checked out a branch where a merge is on by any chance, you'd > find the merge as a follow up Do you mean what will happen if you are applying this script not on master but on a release branch? It shouldn't matter because 'master' is hardwired. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:158: hash_of_first_commit = commits[0] On 2015/10/01 08:24:18, Michael Achenbach wrote: > The test (also for lkgr) would be more covering if roll (or lkgr) wouldn't point > to the exact commit, but to some commit on top. Ack. Also added another assert. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:159: self._execute_git(['branch', 'remotes/origin/roll']) On 2015/10/01 08:24:18, Michael Achenbach wrote: > Remember that the roll ref is just storing what should be rolled - not what is > rolled. We don't have the latter (we only have the canary ref). You are right. I changed the wording. https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:186: #import sys;sys.argv = ['', 'Test.testName'] On 2015/10/01 08:24:18, Michael Achenbach wrote: > nit: remove? Done.
lgtm - with one nit https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/100001/tools/release/mergeinf... tools/release/mergeinfo.py:56: branches = [ current.strip() for current in branches] On 2015/10/01 10:33:25, Hablich wrote: > On 2015/10/01 08:24:18, Michael Achenbach wrote: > > 3 in 1: > > return map(str.strip, branches.splitlines()) > > Did a mixture because I like it a little bit more verbose ok - 2 in 1 :) https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... File tools/release/test_mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/100001/tools/release/test_mer... tools/release/test_mergeinfo.py:142: self.assertEqual(len(followups), 0) On 2015/10/01 10:33:26, Hablich wrote: > On 2015/10/01 08:24:18, Michael Achenbach wrote: > > I assume if you'd checked out a branch where a merge is on by any chance, > you'd > > find the merge as a follow up > > Do you mean what will happen if you are applying this script not on master but > on a release branch? It shouldn't matter because 'master' is hardwired. > Right! https://codereview.chromium.org/1341303002/diff/120001/tools/release/mergeinf... File tools/release/mergeinfo.py (right): https://codereview.chromium.org/1341303002/diff/120001/tools/release/mergeinf... tools/release/mergeinfo.py:39: if merge_commit not in false_merges]) nit: indentation - either 4 or align with merge_commit
The CQ bit was checked by hablich@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/1341303002/#ps140001 (title: "fixed nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1341303002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1341303002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8499fa2505f20622517f5be0f9e274a75fc00dcf Cr-Commit-Position: refs/heads/master@{#31043} |