Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
152343011:
Add support to automatically search for corresponding architecture ports in merge-to-branch.sh. (Closed)
|
Created:
6 years, 10 months ago by palfia Modified:
6 years, 10 months ago CC:
v8-dev Visibility:
Public. |
DescriptionAdd support to automatically search for corresponding architecture ports in merge-to-branch.sh.
BUG=
R=jkummerow@chromium.org, plind44@gmail.com
Committed: https://code.google.com/p/v8/source/detail?r=19163
Patch Set 1 #
Total comments: 9
Patch Set 2 : Removed MIPS specific parts, fixed mistakes. #
Total comments: 4
Patch Set 3 : Fix FULL_REVISION_LIST restoring. #Messages
Total messages: 12 (0 generated)
This CL adds an additional step to the merge-to-branch.sh, which tries to find all corresponding MIPS ports. If the MIPS ports are already included, then it stays silent. Otherwise, it asks if it should add the missing MIPS ports to the revision list. Example usage: $ tools/merge-to-branch.sh 3.23 18958 >>> Step 0: Preparation >>> Step 1: Create a fresh branch for the patch. Switched to a new branch 'prepare-merge' >>> Step 2: Search for corresponding MIPS ports. Found MIPS port of r18958 -> r18968: MIPS: Fix the context check in LoadGlobalFunctionPrototype Automatically add corresponding MIPS ports (18968)? [Y/n] >>> Step 3: Find the git revisions associated with the patches. >>> Step 4: Apply patches for selected revisions. Or, when the MIPS ports are included: $ tools/merge-to-branch.sh 3.23 18958 18968 >>> Step 0: Preparation >>> Step 1: Create a fresh branch for the patch. Switched to a new branch 'prepare-merge' >>> Step 2: Search for corresponding MIPS ports. Found MIPS port of r18958 -> r18968 (already included): MIPS: Fix the context check in LoadGlobalFunctionPrototype >>> Step 3: Find the git revisions associated with the patches. >>> Step 4: Apply patches for selected revisions. Or: $ tools/merge-to-branch.sh 3.23 18958 18968 18627 >>> Step 0: Preparation >>> Step 1: Create a fresh branch for the patch. Switched to a new branch 'prepare-merge' >>> Step 2: Search for corresponding MIPS ports. Found MIPS port of r18958 -> r18968 (already included): MIPS: Fix the context check in LoadGlobalFunctionPrototype Found MIPS port of r18627 -> r18631: MIPS: Fix Win32 buildbreak (caused by overriden methods that have disappeared while having the patch out for code review). Automatically add corresponding MIPS ports (18631)? [Y/n] >>> Step 3: Find the git revisions associated with the patches. >>> Step 4: Apply patches for selected revisions.
LGTM. PTAL guys. The intent here is to make it easy on you to do back-merges, without having to lookup the MIPS ports. And for us, to ensure they are included when appropriate. This relies on the format of our commit messages where we always include "Port r19xxx". We have followed this consistently in the past year, probably much longer.
Comment on the concept: Are the MIPS port CLs always unique? What happens if you commit a MIPS port for a CL and later realize that you made a mistake and need to add something in a follow up CL? How is the naming convention of the follow up CL? Of course, if we'd get some of the MIPS CLs automatically but miss a few, it would already be an improvement. Note: I am working on a python port for this script (low prio though). Code comments: https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh File tools/merge-to-branch.sh (right): https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:144: MIPS_GIT_HASH=$(git log svn/bleeding_edge -1 --format=%H --grep="Port r$REVISION") Can svn/bleeding_edge be left out here? https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:145: if [ "$MIPS_GIT_HASH" != "" ] ; then I'm not a bash expert. Is this equivalent? Then prefer the latter for consistency: if [ "$MIPS_GIT_HASH" != "" ] ; then if [ -n "$MIPS_GIT_HASH" ] ; then https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:148: FULL_REVISION_LIST=("${FULL_REVISION_LIST[@]}" "$REVISION" "$MIPS_SVN_REVISION") What is with revisions where no MIPS port exists, are they skipped now? E.g. consider 3 revision to be merged out of which two have a MIPS port. Where is the third added to FULL_REVISION_LIST? https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:179: for REVISION in $FULL_REVISION_LIST ; do All state that needs to be transferred from one step to the next should be persisted. E.g. if "git svn find-rev" times out in this step, the script should be callable with the -s option in the same step, where it failed. Look below how REVISION_LIST is persisted.
Bonus points if you make this non-MIPS-specific: Assume there are commits "MIPS: Port r123", "FOOBAR: Port r123", etc. :-) https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh File tools/merge-to-branch.sh (right): https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:144: MIPS_GIT_HASH=$(git log svn/bleeding_edge -1 --format=%H --grep="Port r$REVISION") On 2014/02/05 11:10:52, Michael Achenbach wrote: > Can svn/bleeding_edge be left out here? Definitely not. "git checkout -b" above branches from the branch we're merging to, so "git log" alone would show the history of that branch. I'd like 80col lines, though, here and below :-) https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:145: if [ "$MIPS_GIT_HASH" != "" ] ; then On 2014/02/05 11:10:52, Michael Achenbach wrote: > I'm not a bash expert. Is this equivalent? Then prefer the latter for > consistency: > if [ "$MIPS_GIT_HASH" != "" ] ; then > if [ -n "$MIPS_GIT_HASH" ] ; then +1. AFAIK Bash doesn't handle empty strings very well, so please use the -n/-z tests.
Thank you for the review and your comments! I've uploaded a more sophisticated version, which is MIPS-independent. It can also handle the case, when more port exists for a single commit (for example if the A commit has a B FOO port and a C BAR port). PTAL. https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh File tools/merge-to-branch.sh (right): https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:145: if [ "$MIPS_GIT_HASH" != "" ] ; then On 2014/02/05 12:04:13, Jakob wrote: > On 2014/02/05 11:10:52, Michael Achenbach wrote: > > I'm not a bash expert. Is this equivalent? Then prefer the latter for > > consistency: > > if [ "$MIPS_GIT_HASH" != "" ] ; then > > if [ -n "$MIPS_GIT_HASH" ] ; then > > +1. AFAIK Bash doesn't handle empty strings very well, so please use the -n/-z > tests. Done. https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:148: FULL_REVISION_LIST=("${FULL_REVISION_LIST[@]}" "$REVISION" "$MIPS_SVN_REVISION") On 2014/02/05 11:10:52, Michael Achenbach wrote: > What is with revisions where no MIPS port exists, are they skipped now? E.g. > consider 3 revision to be merged out of which two have a MIPS port. Where is the > third added to FULL_REVISION_LIST? Auch. Thanks for pointing this out! Done. https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... tools/merge-to-branch.sh:179: for REVISION in $FULL_REVISION_LIST ; do On 2014/02/05 11:10:52, Michael Achenbach wrote: > All state that needs to be transferred from one step to the next should be > persisted. E.g. if "git svn find-rev" times out in this step, the script should > be callable with the -s option in the same step, where it failed. Look below how > REVISION_LIST is persisted. Done.
On 2014/02/05 11:10:51, Michael Achenbach wrote: > Comment on the concept: Are the MIPS port CLs always unique? What happens if you > commit a MIPS port for a CL and later realize that you made a mistake and need > to add something in a follow up CL? How is the naming convention of the follow > up CL? > > Of course, if we'd get some of the MIPS CLs automatically but miss a few, it > would already be an improvement. > > Note: I am working on a python port for this script (low prio though). > > Code comments: > > https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh > File tools/merge-to-branch.sh (right): > > https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... > tools/merge-to-branch.sh:144: MIPS_GIT_HASH=$(git log svn/bleeding_edge -1 > --format=%H --grep="Port r$REVISION") > Can svn/bleeding_edge be left out here? > > https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... > tools/merge-to-branch.sh:145: if [ "$MIPS_GIT_HASH" != "" ] ; then > I'm not a bash expert. Is this equivalent? Then prefer the latter for > consistency: > if [ "$MIPS_GIT_HASH" != "" ] ; then > if [ -n "$MIPS_GIT_HASH" ] ; then > > https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... > tools/merge-to-branch.sh:148: FULL_REVISION_LIST=("${FULL_REVISION_LIST[@]}" > "$REVISION" "$MIPS_SVN_REVISION") > What is with revisions where no MIPS port exists, are they skipped now? E.g. > consider 3 revision to be merged out of which two have a MIPS port. Where is the > third added to FULL_REVISION_LIST? > > https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... > tools/merge-to-branch.sh:179: for REVISION in $FULL_REVISION_LIST ; do > All state that needs to be transferred from one step to the next should be > persisted. E.g. if "git svn find-rev" times out in this step, the script should > be callable with the -s option in the same step, where it failed. Look below how > REVISION_LIST is persisted. Currently we don't have a naming convention for the follow up fixes, however if we also tag these commits with "Port rXXX" in the future, then the new script can also cherry-pick these in the correct (date) order. Thanks for the idea!
On 2014/02/05 12:04:12, Jakob wrote: > Bonus points if you make this non-MIPS-specific: Assume there are commits "MIPS: > Port r123", "FOOBAR: Port r123", etc. :-) > > https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh > File tools/merge-to-branch.sh (right): > > https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... > tools/merge-to-branch.sh:144: MIPS_GIT_HASH=$(git log svn/bleeding_edge -1 > --format=%H --grep="Port r$REVISION") > On 2014/02/05 11:10:52, Michael Achenbach wrote: > > Can svn/bleeding_edge be left out here? > > Definitely not. "git checkout -b" above branches from the branch we're merging > to, so "git log" alone would show the history of that branch. > > I'd like 80col lines, though, here and below :-) > > https://codereview.chromium.org/152343011/diff/1/tools/merge-to-branch.sh#new... > tools/merge-to-branch.sh:145: if [ "$MIPS_GIT_HASH" != "" ] ; then > On 2014/02/05 11:10:52, Michael Achenbach wrote: > > I'm not a bash expert. Is this equivalent? Then prefer the latter for > > consistency: > > if [ "$MIPS_GIT_HASH" != "" ] ; then > > if [ -n "$MIPS_GIT_HASH" ] ; then > > +1. AFAIK Bash doesn't handle empty strings very well, so please use the -n/-z > tests. MIPS-specific parts are removed! :-)
LGTM with two more comments. https://codereview.chromium.org/152343011/diff/70001/tools/merge-to-branch.sh File tools/merge-to-branch.sh (right): https://codereview.chromium.org/152343011/diff/70001/tools/merge-to-branch.sh... tools/merge-to-branch.sh:152: while read -r NEXT_GIT_HASH; do for consistency with other loops, can we just use: for NEXT_GIT_HASH in $GIT_HASHES; do ... done or does the while loop provide any particular benefit? https://codereview.chromium.org/152343011/diff/70001/tools/merge-to-branch.sh... tools/merge-to-branch.sh:188: for REVISION in $FULL_REVISION_LIST ; do Before using this variable in a new step, please add: restore_if_unset "FULL_REVISION_LIST"
Thanks, I've addressed your comments. https://codereview.chromium.org/152343011/diff/70001/tools/merge-to-branch.sh File tools/merge-to-branch.sh (right): https://codereview.chromium.org/152343011/diff/70001/tools/merge-to-branch.sh... tools/merge-to-branch.sh:152: while read -r NEXT_GIT_HASH; do On 2014/02/06 09:30:26, Jakob wrote: > for consistency with other loops, can we just use: > > for NEXT_GIT_HASH in $GIT_HASHES; do > ... > done > > or does the while loop provide any particular benefit? The problem here is that the "git log" command returns a newline separated list, which can't be iterated with the for loop with the default IFS value. Of course, if I change IFS to IFS=$'\n' then the for loop can split the string correctly. However, I think that using the while loop is a cleaner way then changing the IFS variable. https://codereview.chromium.org/152343011/diff/70001/tools/merge-to-branch.sh... tools/merge-to-branch.sh:188: for REVISION in $FULL_REVISION_LIST ; do On 2014/02/06 09:30:26, Jakob wrote: > Before using this variable in a new step, please add: > > restore_if_unset "FULL_REVISION_LIST" Thanks, done.
On 2014/02/06 14:12:02, palfia wrote:
> The problem here is that the "git log" command returns a newline separated
list,
> which can't be iterated with the for loop with the default IFS value. Of
course,
> if I change IFS to IFS=$'\n' then the for loop can split the string correctly.
> However, I think that using the while loop is a cleaner way then changing the
> IFS variable.
Agreed; if that's the case on your machine, then keeping the while-loop is fine.
However, when I paste the following into a stand-alone shell script, it seems to
work fine:
GIT_HASHES=$(git log svn/bleeding_edge --reverse \
--format=%H --grep="Port r$REVISION" -5)
for GIT_HASH in $GIT_HASHES; do
echo "found git hash: $GIT_HASH"
done
On 2014/02/06 14:29:23, Jakob wrote: > On 2014/02/06 14:12:02, palfia wrote: > > The problem here is that the "git log" command returns a newline separated > list, > > which can't be iterated with the for loop with the default IFS value. Of > course, > > if I change IFS to IFS=$'\n' then the for loop can split the string correctly. > > However, I think that using the while loop is a cleaner way then changing the > > IFS variable. > > Agreed; if that's the case on your machine, then keeping the while-loop is fine. > > However, when I paste the following into a stand-alone shell script, it seems to > work fine: > > GIT_HASHES=$(git log svn/bleeding_edge --reverse \ > --format=%H --grep="Port r$REVISION" -5) > for GIT_HASH in $GIT_HASHES; do > echo "found git hash: $GIT_HASH" > done Hm, interesting. On my development laptop (ubuntu 12.04 with /bin/bash sha-bang) it returns the following: found git hash: cfcc58651517441d75dcb2a2224f21caafa750ca 5c80d13017705f74738c6d25e442c2ba90b3f650 But on our testing machine (with the same ubuntu version) it returns the correct result. It seems that the while loop is a more portable solution.
Message was sent while issue was closed.
Committed patchset #3 manually as r19163 (presubmit successful). |
