You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2014/04/17 13:12:15 UTC
Tonight's svn-role merges - bug analysis
There were 9 merges tonight (r1588145:r1588153). The log messages were
all correct, but the Nth merge removed from STATUS the (2N)th merge's
entry. As a result, the first merge (N=0) was correct, but the others
either removed the wrong STATUS entry or removed no entry.
I think this is due to a bug in the relatively new $parno handling. I
believe the parno code wasn't on the VM until yesterday, even though
it's been in HEAD for a while.
The code stows $parno in the %entry structure at parse time, and then
loops over the entries in Approved and merges them. The first merge
deletes the ($entries[0]->{parno})th paragraph from STATUS.¹ The second
merge then deletes the ($entries[1]->{parno})th paragraph from the
file... which is wrong; it should be deleting the ($entries[1]->{parno} - 1)th
paragraph instead, since the first merge has happened, and
$entries[0]->{parno} < $entries[1]->{parno}.
The fix would be to change the $parno accounting as follows: before
$entry{parno} is used, subtract from it the number of entries already
merged in this run. (Entries are currently merged in file order, so
none of the already-merged-in-this-run entries would have a ->{parno}
greater than $entry{parno} of the entry about to be merged.)
Daniel
P.S. It would be a good idea to have some sort of test suite for
backport.pl...
¹ There is no @entries array; that was just pseudosyntax for "the first entry".
Re: Tonight's svn-role merges - bug analysis
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Thu, Apr 17, 2014 at 11:25:52 +0000:
> @@ -312,7 +315,7 @@ if $sh[$MAY_COMMIT]; then
> # Remove the approved entry. The sentinel guarantees the right number of blank
> # lines is removed, which prevents spurious '--renormalize' commits tomorrow.
> echo "sentinel" >> $STATUS
> - $VIM -e -s -n -N -i NONE -u NONE -c ':0normal! $entry{parno}\x{7d}kdap' -c wq $STATUS
> + $VIM -e -s -n -N -i NONE -u NONE -c ':0normal! $parno\x{7d}kdap' -c wq $STATUS
> $VIM -e -s -n -N -i NONE -u NONE -c '\$d' -c wq $STATUS
> $SVNq commit -F $logmsg_filename
Reading the code, I wonder if the sentinel trick if the last line in the
file is "Votes:\n +1: foo, bar, baz" with no newline after baz.
The fix would be easy:
-echo "sentinel" >> $STATUS
+printf "\n\nsentinel" >> $STATUS
...
-$VIM -e -s -n -N -i NONE -u NONE -c '\$d' -c wq $STATUS
+$VIM -e -s -n -N -i NONE -u NONE -c '\$normal! dap' -c wq $STATUS
i.e., force the sentinel to occupy its own paragraph.
Re: Tonight's svn-role merges - bug analysis
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Thu, Apr 24, 2014 at 23:57:32 +0100:
> The patched script gets the first two merges right (r4 and r8) but the
> final merge (r5) is still wrong: the log message contains "sentinel"
> and the wrong lines are removed from STATUS:
>
..
> Approved changes:
> -=================
> -
> - * r5
Thanks for looking into this. This is exactly the issue I mentioned in
my other email (<http://mid.gmane.org/20140417113242.GD1802%40tarsus.local2>).
The attached patch should fix it.
I'll commit it if I find time to test it, but feel free to beat me to it.
Re: Tonight's svn-role merges - bug analysis
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Daniel Shahaf wrote on Thu, Apr 17, 2014 at 11:12:15 +0000:
>> The fix would be to change the $parno accounting as follows: before
>> $entry{parno} is used, subtract from it the number of entries already
>> merged in this run.
>
> Untested patch:
>
> [[[
> Index: tools/dist/backport.pl
> ===================================================================
> --- tools/dist/backport.pl (revision 1588205)
> +++ tools/dist/backport.pl (working copy)
> @@ -80,6 +80,8 @@ $ENV{LC_ALL} = "C"; # since we parse 'svn info' o
>
> # Globals.
> my %ERRORS = ();
> +# TODO: can $MERGED_SOMETHING be removed and references to it replaced by scalar(@MERGES_TODAY) ?
> +my @MERGES_TODAY;
> my $MERGED_SOMETHING = 0;
> my $SVNq;
>
> @@ -255,6 +257,7 @@ sub my_tempfile {
>
> sub merge {
> my %entry = @_;
> + my $parno = $entry{parno} - scalar grep { $_ < $entry{parno} } @MERGES_TODAY;
>
> my ($logmsg_fh, $logmsg_filename) = my_tempfile();
> my (@mergeargs);
> @@ -312,7 +315,7 @@ if $sh[$MAY_COMMIT]; then
> # Remove the approved entry. The sentinel guarantees the right number of blank
> # lines is removed, which prevents spurious '--renormalize' commits tomorrow.
> echo "sentinel" >> $STATUS
> - $VIM -e -s -n -N -i NONE -u NONE -c ':0normal! $entry{parno}\x{7d}kdap' -c wq $STATUS
> + $VIM -e -s -n -N -i NONE -u NONE -c ':0normal! $parno\x{7d}kdap' -c wq $STATUS
> $VIM -e -s -n -N -i NONE -u NONE -c '\$d' -c wq $STATUS
> $SVNq commit -F $logmsg_filename
> elif ! $sh[$YES]; then
> @@ -325,6 +328,11 @@ elif ! $sh[$YES]; then
> fi
> EOF
>
> + if ($MAY_COMMIT) {
> + # STATUS has been edited and the change has been committed
> + push @MERGES_TODAY, $entry{parno} if $MAY_COMMIT;
> + }
> +
> $script .= <<"EOF" if $entry{branch};
> reinteg_rev=\`$SVN info $STATUS | sed -ne 's/Last Changed Rev: //p'\`
> if $sh[$MAY_COMMIT]; then
> @@ -354,6 +362,8 @@ EOF
> $MERGED_SOMETHING++;
> open SHELL, '|-', qw#/bin/sh# or die "$! (in '$entry{header}')";
> print SHELL $script;
> + # TODO: s/warn/die/ in the $MAY_COMMIT case? (since we don't know
> + # whether to update @MERGES_TODAY)
> close SHELL or warn "$0: sh($?): $! (in '$entry{header}')";
> $ERRORS{$entry{id}} = [\%entry, "sh($?): $!"] if $?;
I created the attached repository to test this:
Re: Tonight's svn-role merges - bug analysis
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Thu, Apr 17, 2014 at 11:12:15 +0000:
> The fix would be to change the $parno accounting as follows: before
> $entry{parno} is used, subtract from it the number of entries already
> merged in this run.
Untested patch:
[[[
Index: tools/dist/backport.pl
===================================================================
--- tools/dist/backport.pl (revision 1588205)
+++ tools/dist/backport.pl (working copy)
@@ -80,6 +80,8 @@ $ENV{LC_ALL} = "C"; # since we parse 'svn info' o
# Globals.
my %ERRORS = ();
+# TODO: can $MERGED_SOMETHING be removed and references to it replaced by scalar(@MERGES_TODAY) ?
+my @MERGES_TODAY;
my $MERGED_SOMETHING = 0;
my $SVNq;
@@ -255,6 +257,7 @@ sub my_tempfile {
sub merge {
my %entry = @_;
+ my $parno = $entry{parno} - scalar grep { $_ < $entry{parno} } @MERGES_TODAY;
my ($logmsg_fh, $logmsg_filename) = my_tempfile();
my (@mergeargs);
@@ -312,7 +315,7 @@ if $sh[$MAY_COMMIT]; then
# Remove the approved entry. The sentinel guarantees the right number of blank
# lines is removed, which prevents spurious '--renormalize' commits tomorrow.
echo "sentinel" >> $STATUS
- $VIM -e -s -n -N -i NONE -u NONE -c ':0normal! $entry{parno}\x{7d}kdap' -c wq $STATUS
+ $VIM -e -s -n -N -i NONE -u NONE -c ':0normal! $parno\x{7d}kdap' -c wq $STATUS
$VIM -e -s -n -N -i NONE -u NONE -c '\$d' -c wq $STATUS
$SVNq commit -F $logmsg_filename
elif ! $sh[$YES]; then
@@ -325,6 +328,11 @@ elif ! $sh[$YES]; then
fi
EOF
+ if ($MAY_COMMIT) {
+ # STATUS has been edited and the change has been committed
+ push @MERGES_TODAY, $entry{parno} if $MAY_COMMIT;
+ }
+
$script .= <<"EOF" if $entry{branch};
reinteg_rev=\`$SVN info $STATUS | sed -ne 's/Last Changed Rev: //p'\`
if $sh[$MAY_COMMIT]; then
@@ -354,6 +362,8 @@ EOF
$MERGED_SOMETHING++;
open SHELL, '|-', qw#/bin/sh# or die "$! (in '$entry{header}')";
print SHELL $script;
+ # TODO: s/warn/die/ in the $MAY_COMMIT case? (since we don't know
+ # whether to update @MERGES_TODAY)
close SHELL or warn "$0: sh($?): $! (in '$entry{header}')";
$ERRORS{$entry{id}} = [\%entry, "sh($?): $!"] if $?;
]]]
> Daniel
>
> P.S. It would be a good idea to have some sort of test suite for
> backport.pl...