Introduce push-to-trunk.sh (for git users)

Introduce push-to-trunk.sh (for git users)

Committed: http://code.google.com/p/v8/source/detail?r=9186

[Jakob Kummerow, 2011-09-05 15:35:15]

PTAL.

This is not tested with a real push to trunk yet, I have only performed a
dry-run (with all commands modifying server state being commented out).

[mnaganov (inactive), 2011-09-05 21:02:14]

DBC

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#newcode48
push-to-trunk.sh:48: if [ -n "$(git branch | grep $BRANCHNAME)" ] ; then
Perhaps, you need to check that this isn't the current branch, otherwise
deletion will fail.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode161
push-to-trunk.sh:161: git checkout -b $TRUNKBRANCH svn/trunk
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"

[danno, 2011-09-06 09:28:55]

Nice! I like it.

Please put this in the tools directory.

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#newcode30
push-to-trunk.sh:30: TRUNKBRANCH=trunk-push
For safety sake, shouldn't you delete the trunk-push branch every time, too,
just like the prepare-push branch.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode59
push-to-trunk.sh:59: git checkout -b $BRANCHNAME svn/bleeding_edge
You probably want to check that there are no uncommitted changes on the current
branch before switching to another branch.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode111
push-to-trunk.sh:111: $EDITOR src/version.cc
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?

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode118
push-to-trunk.sh:118: Now working on version $NEWMAJOR.$NEWMINOR.$NEWBUILD."
Error handling in case something goes wrong?

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode123
push-to-trunk.sh:123: git cl upload -r $REVIEWER --send-mail
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.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode128
push-to-trunk.sh:128: read ANSWER
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.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode133
push-to-trunk.sh:133: 
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.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode159
push-to-trunk.sh:159: 
Might be a good idea to show the resulting commit message and ask for
verification of the contents.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode164
push-to-trunk.sh:164: patch -p1 < "$PATCHFILE"
Should probably check for errors applying the patch and abort, just in case.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode177
push-to-trunk.sh:177: git commit -F "$COMMITMSG"
Error handling?

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode188
push-to-trunk.sh:188: git svn dcommit
Error handling, you definitely don't want to make a tag without knowing the
commit worked.

http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode194
push-to-trunk.sh:194: git checkout -f master
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.

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 \
For extra points and the respect of the entire team, automatically update v8rel
using http://code.google.com/apis/spreadsheets/ :-)

[tfarina, 2011-09-06 16:08:51]

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
no spaces between ! and / ?

[wingo, 2011-09-06 16:17:45]

On 2011/09/06 16:08:51, tfarina wrote:
> http://codereview.chromium.org/7835035/diff/1/push-to-trunk.sh#newcode1
> push-to-trunk.sh:1: #! /bin/bash
> no spaces between ! and / ?

A curious mistake, propagated by erroneous documentation:
http://www.in-ulm.de/~mascheck/various/shebang/#blankrequired

While neither being required nor forbidden, most folks now do not put a space
between #! and /.

[Jakob Kummerow, 2011-09-07 15:46:40]

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 ;-)

[danno, 2011-09-08 08:08:14]

LGTM

[Jakob Kummerow, 2011-09-08 08:50:01]

Landed.

As most source code, such a script is never "finished" or set in stone. I'm
happy to further improve it if anyone has more feature/change requests. Or you
could just create CLs yourself ;-)