svn-apply can exit(0) even on patch failure

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

svn-apply can exit(0) even on patch failure

This is bad for the commit-queue/bugzilla-tool as it depends on svn-apply exiting non-zero on patch failure.  It's possible that bugzilla-tool could land an incomplete patch as-is.

Some examples from the code:

sub scmCopy($$)
{
    my ($source, $destination) = @_;
    if ($isSVN) {
        system "svn", "copy", $source, $destination;
    } elsif ($isGit) {
        system "cp", $source, $destination;
        system "git", "add", $destination;
    }
}

sub scmAdd($)
{
    my ($path) = @_;
    if ($isSVN) {
        system "svn", "add", $path;
    } elsif ($isGit) {
        system "git", "add", $path;
    }
}

sub scmRemove($)
{
    my ($path) = @_;
    if ($isSVN) {
        # SVN is very verbose when removing directories.  Squelch all output except the last line.
        my $svnOutput;
        open SVN, "svn rm --force '$path' |" or die "svn rm --force '$path' failed!";
        # Only print the last line.  Subversion outputs all changed statuses below $dir
        while (<SVN>) {
            $svnOutput = $_;
        }
        close SVN;
        print $svnOutput if $svnOutput;
    } elsif ($isGit) {
        system "git", "rm", "--force", $path;
    }
}

[Eric Seidel (no email)]

Created attachment 40081 [details]
log from another silent failure

[Mark Mentovai]

git diffs don't have Index: lines?

[Eric Seidel (no email)]

I think the log message from svn-apply is misleading.

[Eric Seidel (no email)]

svn-apply uses this magic function to convert git diffs to svn diffs before processing:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/svn-apply#L316

Also, I probably should have split out these two bugs.  The fact that that patch failed to apply is likely a separate bug from the fact that svn-apply can error out silently (thus causing badness!)

[Eric Seidel (no email)]

Created attachment 41766 [details]
Hopefully catch all cases

[Eric Seidel (no email)]

Created attachment 41767 [details]
Hopefully catch all cases

[Adam Barth]

Comment on attachment 41767 [details]
Hopefully catch all cases

die die die!

[WebKit Commit Bot]

Comment on attachment 41767 [details]
Hopefully catch all cases

Clearing flags on attachment: 41767

Committed r50137: <http://trac.webkit.org/changeset/50137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.