Attachment 348961 Details for Bug 14590 – Patch v6

[patch] Patch v6

bug-14590-20180905135126.patch (text/plain), 17.29 KB, created by David Kilzer (:ddkilzer) on 2018-09-05 13:51:27 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: David Kilzer (:ddkilzer)

Created: 2018-09-05 13:51:27 PDT

Size: 17.29 KB

patch

obsolete

>Index: Tools/ChangeLog
>===================================================================
>--- Tools/ChangeLog	(revision 235620)
>+++ Tools/ChangeLog	(working copy)
>@@ -1,3 +1,38 @@
>+2018-09-05  David Kilzer  <ddkilzer@apple.com>
>+
>+        svn-create-patch fails when svn mv is used on directory trees
>+        <https://webkit.org/b/14590>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * Scripts/VCSUtils.pm: Export parseSvnDiffStartLine() for
>+        svn-create-patch.
>+        (parseDiffStartLine): Use parseSvnDiffStartLine().
>+        (parseGitDiffStartLine): Document a prerequisite.  Fix a bug
>+        when parsing git patches using `git diff --no-prefix` non-native
>+        line endings.  Found by new tests written for
>+        Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl.
>+        (parseSvnDiffStartLine): Add.  Extract logic from
>+        parseDiffStartLine() for use with svn-create-patch.
>+
>+        * Scripts/svn-create-patch: Update copyright and license.
>+        (generateDiff): Return early for moved directories since
>+        individual files within the directory are handled separately.
>+        (generateFileList): Keep track of moved directories in the
>+        @additionWithHistoryDirectories array, then process this array
>+        in reverse order to create delete/add patches for each file in
>+        a moved directory.  This also prevents duplicate patches.
>+        (manufacturePatchForAdditionWithHistory): Fix a long-standing
>+        bug where the path used to describe property changes contained
>+        the original (moved-from) path instead of the new (moved-to)
>+        path. This could cause svn-apply to fail mysteriously when
>+        trying to apply an empty patch created by the property change
>+        containing the moved-from path.
>+
>+        * Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl: Add.
>+        Tests for parseDiffStartLine(), parseGitDiffStartLine() and
>+        parseSvnDiffStartLine().
>+
> 2018-09-04  Jer Noble  <jer.noble@apple.com>
> 
>         REGRESSION (r234081): TestWebKitAPI.VideoControlsManager.VideoControlsManagerAudioElementFollowingUserInteraction is a flaky timeout
>Index: Tools/Scripts/VCSUtils.pm
>===================================================================
>--- Tools/Scripts/VCSUtils.pm	(revision 235584)
>+++ Tools/Scripts/VCSUtils.pm	(working copy)
>@@ -1,4 +1,4 @@
>-# Copyright (C) 2007-2013, 2015 Apple Inc.  All rights reserved.
>+# Copyright (C) 2007-2018 Apple Inc.  All rights reserved.
> # Copyright (C) 2009, 2010 Chris Jerdonek (chris.jerdonek@gmail.com)
> # Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.
> # Copyright (C) 2012 Daniel Bates (dbates@intudata.com)
>@@ -82,6 +82,7 @@ BEGIN {
>         &parseDiffStartLine
>         &parseFirstEOL
>         &parsePatch
>+        &parseSvnDiffStartLine
>         &pathRelativeToSVNRepositoryRootForPath
>         &possiblyColored
>         &prepareParsedPatch
>@@ -723,14 +724,14 @@ sub isExecutable($)
> # Parses an SVN or Git diff header start line.
> #
> # Args:
>-#   $line: "Index: " line or "diff --git" line
>+#   $line: "Index: " line or "diff --git" line.
> #
> # Returns the path of the target file or undef if the $line is unrecognized.
> sub parseDiffStartLine($)
> {
>     my ($line) = @_;
>-    return $1 if $line =~ /$svnDiffStartRegEx/;
>     return parseGitDiffStartLine($line) if $line =~ /$gitDiffStartRegEx/;
>+    return parseSvnDiffStartLine($line);
> }
> 
> # Parse the Git diff header start line.
>@@ -738,6 +739,9 @@ sub parseDiffStartLine($)
> # Args:
> #   $line: "diff --git" line.
> #
>+# Prerequisites:
>+#   $line argument matches /$gitDiffStartRegEx/.
>+#
> # Returns the path of the target file.
> sub parseGitDiffStartLine($)
> {
>@@ -752,13 +756,26 @@ sub parseGitDiffStartLine($)
>         die("Could not find '/' in \"diff --git\" line: \"$line\"; only non-prefixed git diffs (i.e. not generated with --no-prefix) that move a top-level directory file are supported.");
>     }
>     my $pathPrefix = $1;
>-    if (!/^diff --git \Q$pathPrefix\E.+ (\Q$pathPrefix\E.+)$/) {
>+    if (!/^diff --git \Q$pathPrefix\E.+ (\Q$pathPrefix\E[^\r\n]+)/) {
>         # FIXME: Moving a file through sub directories of top directory is not supported (e.g diff --git A/B.txt C/B.txt).
>         die("Could not find '/' in \"diff --git\" line: \"$line\"; only non-prefixed git diffs (i.e. not generated with --no-prefix) that move a file between top-level directories are supported.");
>     }
>     return $1;
> }
> 
>+# Parses an SVN diff header start line.
>+#
>+# Args:
>+#   $line: "Index: " line.
>+#
>+# Returns the path of the target file or undef if the $line is unrecognized.
>+sub parseSvnDiffStartLine($)
>+{
>+    my ($line) = @_;
>+    return $1 if $line =~ /$svnDiffStartRegEx/;
>+    return undef;
>+}
>+
> # Parse the next Git diff header from the given file handle, and advance
> # the handle so the last line read is the first line after the header.
> #
>Index: Tools/Scripts/svn-create-patch
>===================================================================
>--- Tools/Scripts/svn-create-patch	(revision 235584)
>+++ Tools/Scripts/svn-create-patch	(working copy)
>@@ -1,30 +1,26 @@
> #!/usr/bin/env perl
> 
>-# Copyright (C) 2005, 2006 Apple Inc.  All rights reserved.
>+# Copyright (C) 2005-2018 Apple Inc.  All rights reserved.
> #
> # Redistribution and use in source and binary forms, with or without
> # modification, are permitted provided that the following conditions
> # are met:
>-#
> # 1.  Redistributions of source code must retain the above copyright
>-#     notice, this list of conditions and the following disclaimer. 
>+#     notice, this list of conditions and the following disclaimer.
> # 2.  Redistributions in binary form must reproduce the above copyright
> #     notice, this list of conditions and the following disclaimer in the
>-#     documentation and/or other materials provided with the distribution. 
>-# 3.  Neither the name of Apple Inc. ("Apple") nor the names of
>-#     its contributors may be used to endorse or promote products derived
>-#     from this software without specific prior written permission. 
>+#     documentation and/or other materials provided with the distribution.
> #
>-# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
>+# THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY
> # EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> # WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
>-# DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
>+# DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> # DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> # (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
>-# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
>-# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>-# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
>-# THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>+# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
>+# ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>+# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> 
> # Extended "svn diff" script for WebKit Open Source Project, used to make patches.
> 
>@@ -35,10 +31,7 @@
> #   Handles binary files (encoded as a base64 chunk of text).
> #   Sorts the diffs alphabetically by text files, then binary files.
> #   Handles copied and moved files.
>-#
>-# Missing features:
>-#
>-#   Handle copied and moved directories.
>+#   Handles copied and moved directories.
> 
> use strict;
> use warnings;
>@@ -252,6 +245,8 @@ sub generateDiff($$)
>     my $patch = "";
>     my $isAdditionWithHistory = $fileData->{modificationType} eq "additionWithHistory";
>     if ($isAdditionWithHistory) {
>+        # Nothing to do for a moved directory since each moved file is handled individually.
>+        return if -d $fileData->{path};
>         manufacturePatchForAdditionWithHistory($fileData, $prefix);
>     }
> 
>@@ -283,6 +278,7 @@ sub generateFileList($\%)
>     my %testDirectories = map { $_ => 1 } qw(LayoutTests);
>     my $escapedStatPath = escapeSubversionPath($statPath);
>     my @deletedFiles;
>+    my @additionWithHistoryDirectories;
> 
>     print STDERR "Performing \"svn stat '$escapedStatPath'\"\n" if $verbose;
> 
>@@ -305,8 +301,6 @@ sub generateFileList($\%)
>             $path = substr($line, 7);
>         }
> 
>-        next if -d $path;
>-
>         my $modificationType = findModificationType($stat);
> 
>         if ($modificationType eq "missing") {
>@@ -343,10 +337,38 @@ sub generateFileList($\%)
>             my ($sourceFile, $sourceRevision) = findSourceFileAndRevision($path);
>             $diffFiles->{$path}->{sourceFile} = $sourceFile;
>             $diffFiles->{$path}->{sourceRevision} = $sourceRevision;
>+            push(@additionWithHistoryDirectories, $path) if -d $path;
>         }
>     }
>     close STAT;
> 
>+    # Handle these in reverse order so that the deepest directory moves are processed first.
>+    # Shallow directory moves include changes for deeper directories, which causes double
>+    # processing and causes the wrong original path to be used if the order is not reversed.
>+    foreach my $directory (reverse @additionWithHistoryDirectories) {
>+        my ($sourceDirectory, $sourceRevision) = findSourceFileAndRevision($directory);
>+        # Gather a hierarchical list of files inside the moved directory.
>+        my $diffOptions = diffOptionsForFile($sourceDirectory);
>+        my $escapedDirectory = escapeSubversionPath($directory);
>+        print STDERR "Performing \"svn diff --diff-cmd diff -x -$diffOptions '$escapedDirectory'\"\n" if $verbose;
>+        open DIFF, "svn diff --diff-cmd diff -x -$diffOptions '$escapedDirectory' |" or die;
>+        while (<DIFF>) {
>+            my $movedFile = parseSvnDiffStartLine($_);
>+            if ($movedFile) {
>+                # Ignore other files added/moved into the moved directory.
>+                next if exists $diffFiles->{$movedFile};
>+                $diffFiles->{$movedFile}->{path} = $movedFile;
>+                $diffFiles->{$movedFile}->{modificationType} = "additionWithHistory";
>+                $diffFiles->{$movedFile}->{isBinary} = isBinaryMimeType($movedFile);
>+                $diffFiles->{$movedFile}->{isTestFile} = exists $testDirectories{(File::Spec->splitdir($movedFile))[0]} ? 1 : 0;
>+                my $relativePath = File::Spec->abs2rel("/" . $movedFile, "/" . $directory);
>+                $diffFiles->{$movedFile}->{sourceFile} = File::Spec->catfile($sourceDirectory, $relativePath);
>+                $diffFiles->{$movedFile}->{sourceRevision} = $sourceRevision;
>+            }
>+        }
>+        close DIFF;
>+    }
>+
>     foreach my $path (@deletedFiles) {
>         my $isInsideDeletedDirectory = 0;
>         foreach my $compare (@deletedFiles) {
>@@ -408,6 +430,7 @@ sub manufacturePatchForAdditionWithHisto
>         while (<DIFF>) {
>             # Skip the diff header, since it was manufactured aboved.
>             next if ++$count < 5;
>+            s/^(Property changes on:\s+)$sourceFile([\r\n]+)/$1$file$2/;
>             print $_;
>         }
>         close DIFF;
>Index: Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl
>===================================================================
>--- Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl	(nonexistent)
>+++ Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl	(working copy)
>@@ -0,0 +1,145 @@
>+#!/usr/bin/env perl
>+
>+# Copyright (C) 2018 Apple Inc.  All rights reserved.
>+#
>+# Redistribution and use in source and binary forms, with or without
>+# modification, are permitted provided that the following conditions
>+# are met:
>+# 1.  Redistributions of source code must retain the above copyright
>+#     notice, this list of conditions and the following disclaimer.
>+# 2.  Redistributions in binary form must reproduce the above copyright
>+#     notice, this list of conditions and the following disclaimer in the
>+#     documentation and/or other materials provided with the distribution.
>+#
>+# THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY
>+# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
>+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
>+# DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS BE LIABLE FOR ANY
>+# DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
>+# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
>+# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
>+# ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>+# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>+
>+# Unit tests for parseDiffStartLine(), parseGitDiffStartLine(), parseSvnDiffStartLine().
>+
>+use strict;
>+use warnings;
>+
>+use constant {
>+    CR => "\r",
>+    CRLF => "\r\n",
>+    LF => "\n",
>+};
>+
>+use Test::More;
>+use VCSUtils;
>+
>+my @testCases = (
>+{
>+    expected => "Source/Makefile.shared",
>+    input => "diff --git a/Source/Makefile.shared b/Source/Makefile.shared",
>+    isGit => 1,
>+    isValid => 1,
>+    testName => "Git",
>+},
>+{
>+    expected => "Source/Makefile.shared",
>+    input => "diff --git Source/Makefile.shared Source/Makefile.shared",
>+    isGit => 1,
>+    isValid => 1,
>+    testName => "Git --no-prefix",
>+},
>+{
>+    expected => "LayoutTests/http/tests/misc/resources/a success.html",
>+    input => "diff --git a/LayoutTests/http/tests/misc/resources/a success.html b/LayoutTests/http/tests/misc/resources/a success.html",
>+    isGit => 1,
>+    isValid => 1,
>+    testName => "Git with space in path",
>+},
>+{
>+    expected => undef,
>+    input => "===================================================================",
>+    isGit => 1,
>+    isValid => 0,
>+    testName => "Git with invalid input",
>+},
>+{
>+    expected => "Source/Makefile.shared",
>+    input => "Index: Source/Makefile.shared",
>+    isSvn => 1,
>+    isValid => 1,
>+    testName => "Svn",
>+},
>+{
>+    expected => "LayoutTests/http/tests/misc/resources/a success.html",
>+    input => "Index: LayoutTests/http/tests/misc/resources/a success.html",
>+    isSvn => 1,
>+    isValid => 1,
>+    testName => "Svn with space in path",
>+},
>+{
>+    expected => undef,
>+    input => "===================================================================",
>+    isSvn => 1,
>+    isValid => 0,
>+    testName => "Svn with invalid input",
>+},
>+);
>+
>+my $testCaseCount = scalar @testCases;
>+plan(tests => 9 * $testCaseCount);
>+
>+foreach my $testCase (@testCases) {
>+    # Make sure future modifications or new test cases are valid.
>+    ok(exists $testCase->{expected}, "'expected' key exists: $testCase->{testName}");
>+    ok(exists $testCase->{input}, "'input' key exists: $testCase->{testName}");
>+    ok(exists $testCase->{isGit} ^ exists $testCase->{isSvn}, "Exactly one of 'isGit' key or 'isSvn' key exists: $testCase->{testName}");
>+    ok(exists $testCase->{isValid}, "'isValid' key exists: $testCase->{testName}");
>+    ok(exists $testCase->{testName}, "'testName' key exists");
>+
>+    my $testCaseName = "parseDiffStartLine(): $testCase->{testName}";
>+    my $got = VCSUtils::parseDiffStartLine($testCase->{input});
>+    is($got, $testCase->{expected}, $testCaseName);
>+
>+    if ($testCase->{isGit}) {
>+        SKIP: {
>+            skip "parseGitDiffStartLine() does not handle invalid input", 3 if !$testCase->{isValid};
>+
>+            $testCaseName = "parseGitDiffStartLine(): CR line ending: $testCase->{testName}";
>+            my $input = $testCase->{input} . CR;
>+            $got = VCSUtils::parseGitDiffStartLine($input);
>+            is($got, $testCase->{expected}, $testCaseName);
>+
>+            $testCaseName = "parseGitDiffStartLine(): CRLF line ending: $testCase->{testName}";
>+            $input = $testCase->{input} . CRLF;
>+            $got = VCSUtils::parseGitDiffStartLine($input);
>+            is($got, $testCase->{expected}, $testCaseName);
>+
>+            $testCaseName = "parseGitDiffStartLine(): LF line ending: $testCase->{testName}";
>+            $input = $testCase->{input} . LF;
>+            $got = VCSUtils::parseGitDiffStartLine($input);
>+            is($got, $testCase->{expected}, $testCaseName);
>+        }
>+    } elsif ($testCase->{isSvn}) {
>+        $testCaseName = "parseSvnDiffStartLine(): CR line ending: $testCase->{testName}";
>+        my $input = $testCase->{input} . CR;
>+        $got = VCSUtils::parseSvnDiffStartLine($input);
>+        is($got, $testCase->{expected}, $testCaseName);
>+
>+        $testCaseName = "parseSvnDiffStartLine(): CRLF line ending: $testCase->{testName}";
>+        $input = $testCase->{input} . CRLF;
>+        $got = VCSUtils::parseSvnDiffStartLine($input);
>+        is($got, $testCase->{expected}, $testCaseName);
>+
>+        $testCaseName = "parseSvnDiffStartLine(): LF line ending: $testCase->{testName}";
>+        $input = $testCase->{input} . LF;
>+        $got = VCSUtils::parseSvnDiffStartLine($input);
>+        is($got, $testCase->{expected}, $testCaseName);
>+    } else {
>+        fail("CR line ending: isGit or isSvn is not set for $testCase->{testName}");
>+        fail("CFLF line ending: isGit or isSvn is not set for $testCase->{testName}");
>+        fail("FL line ending: isGit or isSvn is not set for $testCase->{testName}");
>+    }
>+}

Flags:

dbates: review+
dbates: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 14590: 348766 | 348782 | 348784 | 348853 | 348862 | 348961