LGTM with a bunch of comments.
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/auto_roll.py
File tools/push-to-trunk/auto_roll.py (right):
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/auto_roll.py#newcode108
tools/push-to-trunk/auto_roll.py:108: push_pattern = "^Version
[[:digit:]]*\.[[:digit:]]*\.[[:digit:]]* (based"
Same s/*/+/ comment here... this looks like a candidate for refactoring.
Let's add a helper to class Step, roughly:
def FindLastTrunkPush():
push_pattern = "^Version [[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+
(based"
args = "log -1 --format=%%H --grep=\"%s\" svn/trunk" % push_pattern
return self.Git(args).strip()
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/auto_roll.py#newcode113
tools/push-to-trunk/auto_roll.py:113: # TODO(machebach): This metric
counts all revisions. It could be
Who's machebach?
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/push_to_trunk.py
File tools/push-to-trunk/push_to_trunk.py (right):
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/push_to_trunk.py#newcode67
tools/push-to-trunk/push_to_trunk.py:67: options.b = None
I hope it's on your to-do list to give all these guys readable (long)
names :-)
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/push_to_trunk.py#newcode82
tools/push-to-trunk/push_to_trunk.py:82: self.b = getattr(options, 'b',
None)
Instead of using getattr(), the default value should be set in the
OptionParser. Again, something for the to-do list.
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/push_to_trunk.py#newcode108
tools/push-to-trunk/push_to_trunk.py:108: push_pattern = "^Version
[[:digit:]]*\.[[:digit:]]*\.[[:digit:]]* (based"
For increased correctness, s/*/+/
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/push_to_trunk.py#newcode116
tools/push-to-trunk/push_to_trunk.py:116: args = ("log -1 --format=%H
%s^ --grep=\"%s\""
s/%H/%%H/ like above? Why has this worked before? I guess it's never
been executed...
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/push_to_trunk.py#newcode133
tools/push-to-trunk/push_to_trunk.py:133: args = "svn find-rev r%d" %
int(last_push_be_svn)
"%d" % int(some_string) is the complicated way of saying "%s" %
some_string, eh?
https://codereview.chromium.org/169843002/diff/230001/tools/push-to-trunk/push_to_trunk.py#newcode615
tools/push-to-trunk/push_to_trunk.py:615: "bleeding edge revision that
was pushed to trunk."))
For clarification, I'd add something like "This is used for the
auto-generated ChangeLog entry" (since the patch itself will be created
between current trunk and current bleeding_edge, as opposed to old and
current bleeding_edge).
https://codereview.chromium.org/169843002/
--
--
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/groups/opt_out.
