https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_related_commits.py
File tools/release/search_related_commits.py (right):
https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_related_commits.py#newcode21
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_related_commits.py#newcode25
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_related_commits.py#newcode84
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_related_commits.py#newcode191
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_related_commits.py#newcode197
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_related_commits.py#newcode201
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_related_commits.py#newcode210
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_search_related_commits.py
File tools/release/test_search_related_commits.py (right):
https://codereview.chromium.org/1098123002/diff/140001/tools/release/test_search_related_commits.py#newcode47
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_search_related_commits.py#newcode68
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_search_related_commits.py#newcode86
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_search_related_commits.py#newcode100
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_search_related_commits.py#newcode121
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.
https://codereview.chromium.org/1098123002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
