Major overhaul. - Each step can now be selected as entry point (-s flag) - Addressed all of your concerns - Fixed a bunch of other issues that I found by testing
I've performed today's push-to-trunk using this script, so it has been tested in
the real world by now. Keep 'em comments coming :-) http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh File push-to-trunk.sh (right): http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode1 push-to-trunk.sh:1: #! /bin/bash On 2011/09/06 16:08:52, tfarina wrote:
no spaces between ! and / ?
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode30 push-to-trunk.sh:30: TRUNKBRANCH=trunk-push On 2011/09/06 09:28:55, danno wrote:
For safety sake, shouldn't you delete the trunk-push branch every
time, too,
just like the prepare-push branch.
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode48 push-to-trunk.sh:48: if [ -n "$(git branch | grep $BRANCHNAME)" ] ; then On 2011/09/05 21:02:15, Mikhail Naganov (Chromium) wrote:
Perhaps, you need to check that this isn't the current branch,
otherwise
deletion will fail.
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode59 push-to-trunk.sh:59: git checkout -b $BRANCHNAME svn/bleeding_edge On 2011/09/06 09:28:55, danno wrote:
You probably want to check that there are no uncommitted changes on
the current
branch before switching to another branch.
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode111 push-to-trunk.sh:111: $EDITOR src/version.cc On 2011/09/06 09:28:55, danno wrote:
How about suggesting the next logical version number based on an
uptick of the
build number and only requiring the editing step if the user doesn't
accept the
default?
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode118 push-to-trunk.sh:118: Now working on version $NEWMAJOR.$NEWMINOR.$NEWBUILD." On 2011/09/06 09:28:55, danno wrote:
Error handling in case something goes wrong?
Done. I can't imagine what could go wrong here, though. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode123 push-to-trunk.sh:123: git cl upload -r $REVIEWER --send-mail On 2011/09/06 09:28:55, danno wrote:
You should probably allow the user to specific --prepare-push and
--do-push to
do everything up to this point and after this point in two separate
steps. Done. Each step can be selected as entry point now. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode128 push-to-trunk.sh:128: read ANSWER On 2011/09/06 09:28:55, danno wrote:
If seems a bit fragile, a stray return proceeds without a LGTM. It
would be neat
if you polled the generated CL every few seconds here and only
proceeded if it
contained the LGTM.
Also seems really dangerous to do this without error checking the
result of git
cl upload.
Done: Error checking added; prevention of stray <Return> by requiring the user to type "LGTM<Return>". Any more sophisticated solution is postponed to possible future updates to this script. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode133 push-to-trunk.sh:133: On 2011/09/06 09:28:55, danno wrote:
I don't think this does what you want. What it somebody else committed
something
to bleeding_edge in the meantime, too? Then that becomes part of the
trunk push
patch, which is wrong.
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode159 push-to-trunk.sh:159: On 2011/09/06 09:28:55, danno wrote:
Might be a good idea to show the resulting commit message and ask for verification of the contents.
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode161 push-to-trunk.sh:161: git checkout -b $TRUNKBRANCH svn/trunk On 2011/09/05 21:02:15, Mikhail Naganov (Chromium) wrote:
Will fail if $TRUNKBRANCH already exists for some reason. If you don't
care to
overwrite an existing branch, do "git branch -f $TRUNKBRANCH
svn/trunk" Done: $TRUNKBRANCH now gets deleted at the beginning of the script. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode164 push-to-trunk.sh:164: patch -p1 < "$PATCHFILE" On 2011/09/06 09:28:55, danno wrote:
Should probably check for errors applying the patch and abort, just in
case. Done. However, due to the way the patch file is generated, I don't think it can ever fail. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode177 push-to-trunk.sh:177: git commit -F "$COMMITMSG" On 2011/09/06 09:28:55, danno wrote:
Error handling?
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode188 push-to-trunk.sh:188: git svn dcommit On 2011/09/06 09:28:55, danno wrote:
Error handling, you definitely don't want to make a tag without
knowing the
commit worked.
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode194 push-to-trunk.sh:194: git checkout -f master On 2011/09/06 09:28:55, danno wrote:
Not everybody has a master (like me). You probably should remember the
current
branch from which you started the script and change back to that, it's
more user
friendly.
Done. http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode201 push-to-trunk.sh:201: echo "Please don't forget to update the v8rel spreadsheet, and \ On 2011/09/06 09:28:55, danno wrote:
For extra points and the respect of the entire team, automatically
update v8rel
using http://code.google.com/apis/spreadsheets/ :-)
Advanced feature -> post-V1 ;-) http://codereview.chromium.org/7835035/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
