Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1098123002:
[release-tools] Tool to find related commits (Closed)
|
Created:
5 years, 8 months ago by Michael Hablich Modified:
5 years, 7 months 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. |
Description[release-tools] Tool to find related commits
usage: This tool analyzes the commit range between <of> and <until>. It finds commits which belong together e.g. Implement/Revert pairs and Implement/Port/Revert triples. All supplied hashes need to be from the same branch e.g. master.
Example for M42: ./search_related_commits.py --prettyprint --separator e0110920d6f98f0ba2ac0d680f635ae3f094a04e b856e8785933a2a9cd884ab8966fee0e7098927e b1c2a3495624a9776c7df865d972886f2d078c10
BUG=
NOTRY=true
Committed: https://crrev.com/276a846f69dcae94d601732cf06dc003ff6071e3
Cr-Commit-Position: refs/heads/master@{#28197}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Not for review: Just an update so it is saved #Patch Set 3 : Have a look, tests will follow #Patch Set 4 : --grep from git now fed properly by python #Patch Set 5 : Prettyprint, sorting by date #
Total comments: 27
Patch Set 6 : Did reformatting+refactoring #
Total comments: 9
Patch Set 7 : Made changes, made code testable, added 4 tests #
Total comments: 15
Patch Set 8 : Changes according to review #
Total comments: 24
Patch Set 9 : More changes according to review + more tests #
Messages
Total messages: 18 (3 generated)
hablich@chromium.org changed reviewers: + machenbach@chromium.org
detectReverts.py -> detect_reverts.py Please be more pedantic about style as long as there is no automation for it, e.g. spaces, indentation and var names. I commented on a few. https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts.py File tools/release/detectReverts.py (right): https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... tools/release/detectReverts.py:12: def print_analysis(gitWorkingDir, hashToSearch, upperBound): nit: python var names https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... tools/release/detectReverts.py:14: raw_output = git_execute(gitWorkingDir, ["rev-list", hashToSearch + ".." + upperBound]) Suggestiong: you could use log on origin/master with --grep="[Rr]evert XXX" and --format=%H XXX stands for the way numbers or hashes would be regexped in grep - don't recall that right now. Then you have everything in one git call. https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... tools/release/detectReverts.py:18: candidate_hashes = raw_output.split("\n") splitlines() https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... tools/release/detectReverts.py:28: nit: two empty lines between things on toplevel https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... tools/release/detectReverts.py:29: def git_execute(workingDir, commands): working_dir https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... tools/release/detectReverts.py:39: parser = argparse.ArgumentParser('Tool to check where a git commit was merged and reverted.') Is a tool for one commit useful? Why not a whole range of commits? E.g. B is branch point. Tell me if there's anything interesting about commits in A..B within the range B..C (where C might be HEAD) and where A might be the a commit a week ago. https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... tools/release/detectReverts.py:41: help="The path to your git working directory.") nit: indentation
On 2015/04/21 14:14:24, Michael Achenbach wrote: > detectReverts.py -> detect_reverts.py > > Please be more pedantic about style as long as there is no automation for it, > e.g. spaces, indentation and var names. I commented on a few. > > https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts.py > File tools/release/detectReverts.py (right): > > https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... > tools/release/detectReverts.py:12: def print_analysis(gitWorkingDir, > hashToSearch, upperBound): > nit: python var names > > https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... > tools/release/detectReverts.py:14: raw_output = git_execute(gitWorkingDir, > ["rev-list", hashToSearch + ".." + upperBound]) > Suggestiong: you could use log on origin/master with --grep="[Rr]evert XXX" and > --format=%H > > XXX stands for the way numbers or hashes would be regexped in grep - don't > recall that right now. > > Then you have everything in one git call. > > https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... > tools/release/detectReverts.py:18: candidate_hashes = raw_output.split("\n") > splitlines() > > https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... > tools/release/detectReverts.py:28: > nit: two empty lines between things on toplevel > > https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... > tools/release/detectReverts.py:29: def git_execute(workingDir, commands): > working_dir > > https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... > tools/release/detectReverts.py:39: parser = argparse.ArgumentParser('Tool to > check where a git commit was merged and reverted.') > Is a tool for one commit useful? Why not a whole range of commits? E.g. B is > branch point. Tell me if there's anything interesting about commits in A..B > within the range B..C (where C might be HEAD) and where A might be the a commit > a week ago. > > https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... > tools/release/detectReverts.py:41: help="The path to your git working > directory.") > nit: indentation This was not meant for review yet as it is not finished. As told via chat this is intended to share the idea with you :-).
Some comments. Will make another review round after you looked through those. Many of the formatting comments relate to the whole file (I didn't comment on all instances). https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:14: git_working_dir, of, until, deadline, verbose=False): nit: 4 space indentation for params. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:14: git_working_dir, of, until, deadline, verbose=False): Suggestion: Better name for "of"? It know it sounds fluent as search_all_related_commits "of", but the single variable "of" in the code flow is confusing. I'd like something like "revision" better. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:30: if len(related_commits) > 0: nit: if related_commits: https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:32: already_treated_commits = already_treated_commits + related_commits already_treated_commits.extend(related_commits) https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:40: if deadline != "": Better no default for deadline (i.e. default is None) and then: if deadline: https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:41: afterDeadline = False unused variable? https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:96: hits = (_convert_to_array(found_by_hash) nit: Indentation: Either use 4 space indentation or same level, operators on last line ending: hits = ( _convert_to_array(found_by_hash) + _convert_to_array(found_by_commit_pos) + _convert_to_array(found_by_title) ) or hits = (_convert_to_array(found_by_hash) + _convert_to_array(found_by_commit_pos) + _convert_to_array(found_by_title)) https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:155: all_related_commits = (search_all_related_commits(options.git_dir, nit: Doesn't need the outer parenthesis + params all on one level: all_related_commits = search_all_related_commits( options.git_dir, ... Or indent like this: all_related_commits = ( search_all_related_commits( options.git_dir, ... https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:161: high_level_commits = sorted(all_related_commits.keys(),key = lambda x:( nit: ,space :space https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:162: (_git_execute(options.git_dir, nit: too many parenthesis https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:168: print "+" + (_git_execute( Difficult to read the way the parenthesis are set and the way it is indented. Maybe extract method? https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:183: print "| " + (_git_execute( Same here.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:14: git_working_dir, of, until, deadline, verbose=False): On 2015/04/24 12:45:12, Michael Achenbach wrote: > nit: 4 space indentation for params. Done. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:14: git_working_dir, of, until, deadline, verbose=False): On 2015/04/24 12:45:12, Michael Achenbach wrote: > Suggestion: Better name for "of"? It know it sounds fluent as > search_all_related_commits "of", but the single variable "of" in the code flow > is confusing. I'd like something like "revision" better. Renamed it inside the function. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:30: if len(related_commits) > 0: On 2015/04/24 12:45:12, Michael Achenbach wrote: > nit: > if related_commits: I like it that way as it is more explicit and better readable. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:32: already_treated_commits = already_treated_commits + related_commits On 2015/04/24 12:45:12, Michael Achenbach wrote: > already_treated_commits.extend(related_commits) Done. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:40: if deadline != "": On 2015/04/24 12:45:12, Michael Achenbach wrote: > Better no default for deadline (i.e. default is None) and then: > if deadline: Done. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:41: afterDeadline = False On 2015/04/24 12:45:12, Michael Achenbach wrote: > unused variable? Done. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:96: hits = (_convert_to_array(found_by_hash) On 2015/04/24 12:45:12, Michael Achenbach wrote: > nit: Indentation: Either use 4 space indentation or same level, operators on > last line ending: > hits = ( > _convert_to_array(found_by_hash) + > _convert_to_array(found_by_commit_pos) + > _convert_to_array(found_by_title) > ) > or > hits = (_convert_to_array(found_by_hash) + > _convert_to_array(found_by_commit_pos) + > _convert_to_array(found_by_title)) Done. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:155: all_related_commits = (search_all_related_commits(options.git_dir, On 2015/04/24 12:45:12, Michael Achenbach wrote: > nit: Doesn't need the outer parenthesis + params all on one level: > all_related_commits = search_all_related_commits( > options.git_dir, > ... > > Or indent like this: > all_related_commits = ( > search_all_related_commits( > options.git_dir, > ... Done. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:168: print "+" + (_git_execute( On 2015/04/24 12:45:12, Michael Achenbach wrote: > Difficult to read the way the parenthesis are set and the way it is indented. > Maybe extract method? Done. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:183: print "| " + (_git_execute( On 2015/04/24 12:45:12, Michael Achenbach wrote: > Same here. Done.
Some more readability comments and replies, before diving into the functionality a bit deeper. Could you add an example call of the script in the CL description? We sometimes add this: TEST=tools/release/search_related_commits.py ... https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:30: if len(related_commits) > 0: On 2015/04/27 11:06:08, Hablich wrote: > On 2015/04/24 12:45:12, Michael Achenbach wrote: > > nit: > > if related_commits: > > I like it that way as it is more explicit and better readable. Hmm, but not pythonish. Most places in the infrastructure code, we use the shorter version as bool(related_commits) implies len(related_commits) > 0 https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:60: found_by_hash = (_git_execute( I don't know for what the outer parentheses are needed? Also in the remainder of this file... https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:162: (_git_execute(options.git_dir, On 2015/04/24 12:45:12, Michael Achenbach wrote: > nit: too many parenthesis What about this? Maybe also move the lambda into a local variable for better readability. https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:168: print "+" + (_git_execute( On 2015/04/27 11:06:07, Hablich wrote: > On 2015/04/24 12:45:12, Michael Achenbach wrote: > > Difficult to read the way the parenthesis are set and the way it is indented. > > Maybe extract method? > > Done. Much more readable :) https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:16: hash = of not liked https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:43: hash = of not liked https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:67: git_working_dir, ( You don't need additional parentheses in python as long as you are in the scope of one opening parenthesis. https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:69: search_range, nit: indentation, align list items with content in [ https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:143: def _pretty_print_entry(hash, pre_text, verbose): Format and readability - how about: output = _git_execute( options.git_dir, ["show", "--quiet", "--date=iso", "--format=%ad # %H # %s", hash], verbose, ) print pre_text + output.strip()
PTAL https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:16: hash = of On 2015/04/27 13:09:43, Michael Achenbach wrote: > not liked Done. https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:43: hash = of On 2015/04/27 13:09:43, Michael Achenbach wrote: > not liked Done. https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:67: git_working_dir, ( On 2015/04/27 13:09:43, Michael Achenbach wrote: > You don't need additional parentheses in python as long as you are in the scope > of one opening parenthesis. Done. https://codereview.chromium.org/1098123002/diff/100001/tools/release/search_r... tools/release/search_related_commits.py:143: def _pretty_print_entry(hash, pre_text, verbose): On 2015/04/27 13:09:43, Michael Achenbach wrote: > Format and readability - how about: > > output = _git_execute( > options.git_dir, > ["show", > "--quiet", > "--date=iso", > "--format=%ad # %H # %s", > hash], > verbose, > ) > print pre_text + output.strip() Done.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_re... tools/release/search_related_commits.py:162: (_git_execute(options.git_dir, On 2015/04/27 13:09:43, Michael Achenbach wrote: > On 2015/04/24 12:45:12, Michael Achenbach wrote: > > nit: too many parenthesis > > What about this? Maybe also move the lambda into a local variable for better > readability. I still have the feeling this comment was overlooked. https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_r... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_r... tools/release/search_related_commits.py:22: #Adding start hash too nit: Space after # - end comments with periods. https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_r... tools/release/search_related_commits.py:78: git_working_dir,( nit: space after comma https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... File tools/release/test_search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:6: import unittest nit sort by module name https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:22: def _execute_git(self, commands): Maybe use *args for commands (e.g. *commands), then you don't need lists below? https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:35: f = open(self.base_dir + "/" + file_name, 'w') Use "with" statement https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:39: self._execute_git(["add", self.base_dir + "/" + file_name]) Do you need files? There is a flag that allows empty commits (--allow-empty?). Do files give any benefit for these tests? https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:46: check_call(["git", "init", self.base_dir]) I like this testing approach! Will look into the details tomorrow. https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:94: f = open(self.base_dir + "/first", 'w') use "with" statement and/or extract some helper methods for similar stuff. This seems recurring.
https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_r... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_r... tools/release/search_related_commits.py:22: #Adding start hash too On 2015/04/28 21:23:51, Michael Achenbach wrote: > nit: Space after # - end comments with periods. Done. https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_r... tools/release/search_related_commits.py:78: git_working_dir,( On 2015/04/28 21:23:51, Michael Achenbach wrote: > nit: space after comma Done. https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... File tools/release/test_search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:6: import unittest On 2015/04/28 21:23:51, Michael Achenbach wrote: > nit sort by module name Done. https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:22: def _execute_git(self, commands): On 2015/04/28 21:23:51, Michael Achenbach wrote: > Maybe use *args for commands (e.g. *commands), then you don't need lists below? Acknowledged. https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:35: f = open(self.base_dir + "/" + file_name, 'w') On 2015/04/28 21:23:51, Michael Achenbach wrote: > Use "with" statement Acknowledged. https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:39: self._execute_git(["add", self.base_dir + "/" + file_name]) On 2015/04/28 21:23:51, Michael Achenbach wrote: > Do you need files? There is a flag that allows empty commits (--allow-empty?). > Do files give any benefit for these tests? Acknowledged. https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_sea... tools/release/test_search_related_commits.py:94: f = open(self.base_dir + "/first", 'w') On 2015/04/28 21:23:51, Michael Achenbach wrote: > use "with" statement and/or extract some helper methods for similar stuff. This > seems recurring. Acknowledged.
https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts.py File tools/release/detectReverts.py (right): https://codereview.chromium.org/1098123002/diff/1/tools/release/detectReverts... tools/release/detectReverts.py:39: parser = argparse.ArgumentParser('Tool to check where a git commit was merged and reverted.') On 2015/04/21 14:14:24, Michael Achenbach wrote: > Is a tool for one commit useful? Why not a whole range of commits? E.g. B is > branch point. Tell me if there's anything interesting about commits in A..B > within the range B..C (where C might be HEAD) and where A might be the a commit > a week ago. Could you answer to this? Has that been done? The main description of the tool is maybe a bit misleading. https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:21: #Adding start hash too nit: space after # - also below https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:25: already_treated_commits = [] Make already_treated_commits a set as it has lots of inclusion checks and order doesn't matter? https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:84: # Replace brackets or else they are wrongly interpreted by --grep Are there no other things that need to be escaped? Is there a general regexp escape method in python that could be applied? https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:191: output.append("\n".join(hits)) As you _use_ main() like a generator, how about making it a generator. E.g. instead of output.append("\n".join(hits)) write: yield "\n".join(hits) and remove "output = []" and "return output" https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:197: "This tool searches the git repository for " Is this description accurate? Isn't the tool searching through all commits between of and until? https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:201: parser.add_argument("--verbose", action="store_true", This is quite chatty. How about calling this "--debug"? I don't think it is useful for the end user, i.e. in the sense of "give me some more details". https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:210: parser.add_argument("--deadline", required=False, "deadline" and "until" are very similar in their word meaning. Maybe name "deadline" "separator" or "branch-point" or something? https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... File tools/release/test_search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:47: #self._add_commit_new_file("first", message, "Content") nit: remove? https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:68: result.keys()[0], Maybe better assertTrue(result.get(hash_of_first_commit)). Not sure if keys() are deterministic. https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:86: def testSearchByCommitPosition(self): How about one negative example where nothing is found and nothing matches? https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:100: result = search_related_commits.search_all_related_commits( Looks like the deadline=something case is not covered, but isn't that the most interesting one? https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:121: self.base_dir, hash_of_first_commit, "HEAD",None) nit: space after , - also rest of this file
LGTM (after fixing the last comments series). Retried the example, was a bit blind on my first try...
https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... File tools/release/search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:21: #Adding start hash too On 2015/05/04 08:57:53, Michael Achenbach wrote: > nit: space after # - also below Done. https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:25: already_treated_commits = [] On 2015/05/04 08:57:53, Michael Achenbach wrote: > Make already_treated_commits a set as it has lots of inclusion checks and order > doesn't matter? Acknowledged. https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:84: # Replace brackets or else they are wrongly interpreted by --grep On 2015/05/04 08:57:53, Michael Achenbach wrote: > Are there no other things that need to be escaped? Is there a general regexp > escape method in python that could be applied? The String is already escaped when it is given to Popen automatically. So re.escape is not producing the right results as in that case the curly braces are wrong. When the I have only found that the square brackets need some special handling. So this seems to be ok. https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:191: output.append("\n".join(hits)) On 2015/05/04 08:57:53, Michael Achenbach wrote: > As you _use_ main() like a generator, how about making it a generator. E.g. > instead of > output.append("\n".join(hits)) > write: > yield "\n".join(hits) > > and remove "output = []" and "return output" sgtm https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:197: "This tool searches the git repository for " On 2015/05/04 08:57:53, Michael Achenbach wrote: > Is this description accurate? Isn't the tool searching through all commits > between of and until? Updated description. https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:201: parser.add_argument("--verbose", action="store_true", On 2015/05/04 08:57:53, Michael Achenbach wrote: > This is quite chatty. How about calling this "--debug"? I don't think it is > useful for the end user, i.e. in the sense of "give me some more details". The intention is to keep in line with the rest of the tools which only have --verbose https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_r... tools/release/search_related_commits.py:210: parser.add_argument("--deadline", required=False, On 2015/05/04 08:57:53, Michael Achenbach wrote: > "deadline" and "until" are very similar in their word meaning. Maybe name > "deadline" "separator" or "branch-point" or something? separator sounds good. https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... File tools/release/test_search_related_commits.py (right): https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:47: #self._add_commit_new_file("first", message, "Content") On 2015/05/04 08:57:53, Michael Achenbach wrote: > nit: remove? Acknowledged. https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:68: result.keys()[0], On 2015/05/04 08:57:53, Michael Achenbach wrote: > Maybe better assertTrue(result.get(hash_of_first_commit)). Not sure if keys() > are deterministic. Good point. The dict is (and should be) order-agnostic. https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:86: def testSearchByCommitPosition(self): On 2015/05/04 08:57:53, Michael Achenbach wrote: > How about one negative example where nothing is found and nothing matches? Done. https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:100: result = search_related_commits.search_all_related_commits( On 2015/05/04 08:57:53, Michael Achenbach wrote: > Looks like the deadline=something case is not covered, but isn't that the most > interesting one? Done. https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_sea... tools/release/test_search_related_commits.py:121: self.base_dir, hash_of_first_commit, "HEAD",None) On 2015/05/04 08:57:53, Michael Achenbach wrote: > nit: space after , - also rest of this file Done.
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/1098123002/#ps160001 (title: "More changes according to review + more tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098123002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/276a846f69dcae94d601732cf06dc003ff6071e3 Cr-Commit-Position: refs/heads/master@{#28197}
Message was sent while issue was closed.
RSLGTM! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
