I have a bunch of comments.
Once the way this script works has stabilized, I think it would be a good
idea
to factor out the code it shares with push-to-trunk.sh into a shared "header
file", but that doesn't have to happen in this CL.
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh
File tools/merge-to-branch.sh (right):
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode2
tools/merge-to-branch.sh:2: # Copyright 2011 the V8 project authors. All
rights reserved.
nit: 2012
http://codereview.chromium.org/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."
nit: "revisions"
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode169
tools/merge-to-branch.sh:169: for REVISION in "$@"
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
http://codereview.chromium.org/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)
"bleeding_edge" is one of your local git branches. Please use
"svn/bleeding_edge" instead.
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode175
tools/merge-to-branch.sh:175: if [ "$NEW_COMMIT_MSG" ] ; then
for consistency:
[[ -n "$NEW_COMMIT_MSG" ]] && NEW_COMMIT_MSG="$NEW_COMMIT_MSG,"
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode183
tools/merge-to-branch.sh:183: persist PATCH_COMMIT_HASHES
Have you tested whether persist() can handle arrays correctly?
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode192
tools/merge-to-branch.sh:192: echo "$NEW_COMMIT_MSG" > $COMMITMSG_FILE
If you move this to the end of step 3, you don't need the
persist()/restore_if_unset() cycle.
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode196
tools/merge-to-branch.sh:196: PATCH_MERGE_DESCRIPTION=$(git show --notes
$HASH \
I think what you mean here is just:
PATCH_MERGE_DESCRIPTION=$(git log -1 --format=%s $HASH)
no awk/sed necessary.
http://codereview.chromium.org/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}')
I guess it doesn't really matter, but "git log -1" emits only the log
message (and not the entire diff).
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode227
tools/merge-to-branch.sh:227: for HASH in ${PATCH_COMMIT_HASHES[@]}
missing restore_if_unset for PATCH_COMMIT_HASHES
http://codereview.chromium.org/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"
Talking about a single MERGE_REVISION here is inconsistent with the
for-loops elsewhere...
http://codereview.chromium.org/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.
outdated comment?
http://codereview.chromium.org/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}')
do we need MAJOR, MINOR and BUILD at all?
http://codereview.chromium.org/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 \
s/BUILD_NUMBER/PATCH_LEVEL/
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode282
tools/merge-to-branch.sh:282: echo -e "--------------------"
nit: "-e" is unnecessary (since you don't have any escaped characters
such as \n in the echoed string)
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode296
tools/merge-to-branch.sh:296: echo "R=$REVIEWER" >> $COMMITMSG_FILE_COPY
You don't need this. "git cl upload --review" automatically inserts the
R= line.
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode297
tools/merge-to-branch.sh:297: export EDITOR="cat $COMMITMSG_FILE_COPY >"
I'm not sure I like messing with my EDITOR variable... but it's up to
you.
http://codereview.chromium.org/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 \
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.
http://codereview.chromium.org/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 \
You can remove the "Do not modify..." part, as it doesn't apply here.
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode321
tools/merge-to-branch.sh:321: echo "git-cl dcommit"
Forgot to remove the "echo".
http://codereview.chromium.org/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\""
another "echo" that needs to go.
http://codereview.chromium.org/9298012/diff/2001/tools/merge-to-branch.sh#newcode337
tools/merge-to-branch.sh:337: [[ "$BRANCHNAME" != "$CURRENT_BRANCH" ]]
&& echo "git branch -D $BRANCHNAME"
another "echo" that needs to go.
http://codereview.chromium.org/9298012/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
