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.py#newcode39
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_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
nit: space after # - also below
https://codereview.chromium.org/1098123002/diff/140001/tools/release/search_related_commits.py#newcode25
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_related_commits.py#newcode84
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_related_commits.py#newcode191
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_related_commits.py#newcode197
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_related_commits.py#newcode201
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_related_commits.py#newcode210
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_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")
nit: remove?
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],
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_search_related_commits.py#newcode86
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_search_related_commits.py#newcode100
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_search_related_commits.py#newcode121
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
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.
