Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9298012:
Add patch-merging script (Closed)
|
Created:
8 years, 11 months ago by danno Modified:
8 years, 10 months ago Reviewers:
Jakob Kummerow CC:
v8-dev Visibility:
Public. |
DescriptionAdd a script to automatically merge patches to a branch
Committed: https://code.google.com/p/v8/source/detail?r=10561
Patch Set 1 #Patch Set 2 : Tweaks #
Total comments: 44
Patch Set 3 : address feedback #Patch Set 4 : tweaks #Patch Set 5 : correct help syntax #Patch Set 6 : Review feedback #Patch Set 7 : fix bugs #
Total comments: 10
Messages
Total messages: 5 (0 generated)
First draft, lots of loose edges, but feedback welcome.
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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... 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#new... tools/merge-to-branch.sh:337: [[ "$BRANCHNAME" != "$CURRENT_BRANCH" ]] && echo "git branch -D $BRANCHNAME" another "echo" that needs to go.
Please take another look https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branc... File tools/merge-to-branch.sh (right): https://chromiumcodereview.appspot.com/9298012/diff/2001/tools/merge-to-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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-branc... 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.
LGTM with nits. http://codereview.chromium.org/9298012/diff/7002/tools/merge-to-branch.sh File tools/merge-to-branch.sh (right): http://codereview.chromium.org/9298012/diff/7002/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:206: NEW_COMMIT_MSG="Merged$NEW_COMMIT_MSG into \ nit: fits on one line http://codereview.chromium.org/9298012/diff/7002/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:211: for HASH in ${PATCH_COMMIT_HASHES[@]} ; do nit: double space before "do" http://codereview.chromium.org/9298012/diff/7002/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:228: echo "BUG=none" >> $COMMITMSG_FILE I'd just remove this. "BUG=none" adds no useful information. http://codereview.chromium.org/9298012/diff/7002/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:230: echo -n "TEST=none" >> $COMMITMSG_FILE Same here. http://codereview.chromium.org/9298012/diff/7002/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:251: MAJOR=$(grep "#define MAJOR_VERSION" "$VERSION_FILE" | awk '{print $NF}') AFAICS the tag is created using NEWMAJOR, NEWMINOR, NEWBUILD, which are created and persisted in the next step. So you really don't need these.
Feedback addressed, landing https://chromiumcodereview.appspot.com/9298012/diff/7002/tools/merge-to-branc... File tools/merge-to-branch.sh (right): https://chromiumcodereview.appspot.com/9298012/diff/7002/tools/merge-to-branc... tools/merge-to-branch.sh:206: NEW_COMMIT_MSG="Merged$NEW_COMMIT_MSG into \ On 2012/01/31 11:11:10, Jakob wrote: > nit: fits on one line Done. https://chromiumcodereview.appspot.com/9298012/diff/7002/tools/merge-to-branc... tools/merge-to-branch.sh:211: for HASH in ${PATCH_COMMIT_HASHES[@]} ; do On 2012/01/31 11:11:10, Jakob wrote: > nit: double space before "do" Done. https://chromiumcodereview.appspot.com/9298012/diff/7002/tools/merge-to-branc... tools/merge-to-branch.sh:228: echo "BUG=none" >> $COMMITMSG_FILE On 2012/01/31 11:11:10, Jakob wrote: > I'd just remove this. "BUG=none" adds no useful information. Done. https://chromiumcodereview.appspot.com/9298012/diff/7002/tools/merge-to-branc... tools/merge-to-branch.sh:230: echo -n "TEST=none" >> $COMMITMSG_FILE On 2012/01/31 11:11:10, Jakob wrote: > Same here. Done. https://chromiumcodereview.appspot.com/9298012/diff/7002/tools/merge-to-branc... tools/merge-to-branch.sh:251: MAJOR=$(grep "#define MAJOR_VERSION" "$VERSION_FILE" | awk '{print $NF}') On 2012/01/31 11:11:10, Jakob wrote: > AFAICS the tag is created using NEWMAJOR, NEWMINOR, NEWBUILD, which are created > and persisted in the next step. So you really don't need these. Done. |
