Please take another look
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh
File tools/merge-to-branch.sh (right):
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode2
tools/merge-to-branch.sh:2: # Copyright 2011 the V8 project authors. All
rights reserved.
On 2012/01/27 16:28:12, Jakob wrote:
nit: 2012
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode167
tools/merge-to-branch.sh:167: echo ">>> Step 3: Find the git revsions
associated with the patches."
On 2012/01/27 16:28:12, Jakob wrote:
nit: "revisions"
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode169
tools/merge-to-branch.sh:169: for REVISION in "$@"
On 2012/01/27 16:28:12, Jakob wrote:
nit: You can get closer to our C++ style by putting the "do" on the
same line,
separated by a semicolon:
for REVISION in "$@" ; do
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode171
tools/merge-to-branch.sh:171: NEXT_HASH=$(git svn find-rev "r$REVISION"
bleeding_edge)
On 2012/01/27 16:28:12, Jakob wrote:
"bleeding_edge" is one of your local git branches. Please use
"svn/bleeding_edge" instead.
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode175
tools/merge-to-branch.sh:175: if [ "$NEW_COMMIT_MSG" ] ; then
On 2012/01/27 16:28:12, Jakob wrote:
for consistency:
[[ -n "$NEW_COMMIT_MSG" ]] && NEW_COMMIT_MSG="$NEW_COMMIT_MSG,"
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode183
tools/merge-to-branch.sh:183: persist PATCH_COMMIT_HASHES
On 2012/01/27 16:28:12, Jakob wrote:
Have you tested whether persist() can handle arrays correctly?
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode192
tools/merge-to-branch.sh:192: echo "$NEW_COMMIT_MSG" > $COMMITMSG_FILE
On 2012/01/27 16:28:12, Jakob wrote:
If you move this to the end of step 3, you don't need the
persist()/restore_if_unset() cycle.
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode196
tools/merge-to-branch.sh:196: PATCH_MERGE_DESCRIPTION=$(git show --notes
$HASH \
On 2012/01/27 16:28:12, Jakob wrote:
I think what you mean here is just:
PATCH_MERGE_DESCRIPTION=$(git log -1 --format=%s $HASH)
no awk/sed necessary.
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode210
tools/merge-to-branch.sh:210: BUG=$(git show --notes $HASH | grep "BUG="
| awk -F '=' '{print $NF}')
On 2012/01/27 16:28:12, Jakob wrote:
I guess it doesn't really matter, but "git log -1" emits only the log
message
(and not the entire diff).
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode227
tools/merge-to-branch.sh:227: for HASH in ${PATCH_COMMIT_HASHES[@]}
On 2012/01/27 16:28:12, Jakob wrote:
missing restore_if_unset for PATCH_COMMIT_HASHES
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode230
tools/merge-to-branch.sh:230: || die "Cannot apply the patch for
r$MERGE_REVISION on $MERGE_TO_BRANCH"
On 2012/01/27 16:28:12, Jakob wrote:
Talking about a single MERGE_REVISION here is inconsistent with the
for-loops
elsewhere...
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode236
tools/merge-to-branch.sh:236: # These version numbers are used again
later for the trunk commit.
On 2012/01/27 16:28:12, Jakob wrote:
outdated comment?
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode237
tools/merge-to-branch.sh:237: MAJOR=$(grep "#define MAJOR_VERSION"
"$VERSION_FILE" | awk '{print $NF}')
Yes. For building the tag.
On 2012/01/27 16:28:12, Jakob wrote:
do we need MAJOR, MINOR and BUILD at all?
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode251
tools/merge-to-branch.sh:251: confirm "Automatically increment
BUILD_NUMBER? (Saying 'n' will fire up \
On 2012/01/27 16:28:12, Jakob wrote:
s/BUILD_NUMBER/PATCH_LEVEL/
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode282
tools/merge-to-branch.sh:282: echo -e "--------------------"
On 2012/01/27 16:28:12, Jakob wrote:
nit: "-e" is unnecessary (since you don't have any escaped characters
such as \n
in the echoed string)
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode296
tools/merge-to-branch.sh:296: echo "R=$REVIEWER" >> $COMMITMSG_FILE_COPY
On 2012/01/27 16:28:12, Jakob wrote:
You don't need this. "git cl upload --review" automatically inserts
the R= line.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode297
tools/merge-to-branch.sh:297: export EDITOR="cat $COMMITMSG_FILE_COPY >"
On 2012/01/27 16:28:12, Jakob wrote:
I'm not sure I like messing with my EDITOR variable... but it's up to
you.
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode305
tools/merge-to-branch.sh:305: compile, run tests. Do you want to commit
this new trunk revision to the \
On 2012/01/27 16:28:12, Jakob wrote:
nit: s/new trunk//
Also, we don't really need step 11 at all, since step 12 is asking for
confirmation yet again. I'd integrate the message that's printed here
into step
12's message ("Now is also the right time to run sanity checks in your
working
directory"), and get rid of step 11.
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode313
tools/merge-to-branch.sh:313: change. (If you need to iterate on the
patch, do so in another shell. Do not \
On 2012/01/27 16:28:12, Jakob wrote:
You can remove the "Do not modify..." part, as it doesn't apply here.
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode321
tools/merge-to-branch.sh:321: echo "git-cl dcommit"
On 2012/01/27 16:28:12, Jakob wrote:
Forgot to remove the "echo".
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode330
tools/merge-to-branch.sh:330: echo "svn copy -r ?
https://v8.googlecode.com/svn/branches/$MERGE_TO_BRANCH
https://v8.googlecode.com/svn/tags/$NEWMAJOR.$NEWMINOR.$NEWBUILD.$NEWPATCH
-m \"Tagging version $NEWMAJOR.$NEWMINOR.$NEWBUILD.$NEWPATCH\""
On 2012/01/27 16:28:12, Jakob wrote:
another "echo" that needs to go.
Done.
https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branch.sh#newcode337
tools/merge-to-branch.sh:337: [[ "$BRANCHNAME" != "$CURRENT_BRANCH" ]]
&& echo "git branch -D $BRANCHNAME"
On 2012/01/27 16:28:12, Jakob wrote:
another "echo" that needs to go.
Done.
https://chromiumcodereview.appspot.com/9298012/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
