Bug 33447 – Create a function for svn-apply and svn-unapply to parse diff headers

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

Create a unit-tested parseDiffHeader() subroutine that can eventually replace this code in svn-apply (and similar code in svn-unapply):

    if ($indexPath) {
        # Fix paths on diff, ---, and +++ lines to match preceding Index: line.
        s/\S+$/$indexPath/ if /^diff/;
        s/^--- \S+/--- $indexPath/;
        if (/^--- .+\(from (\S+):(\d+)\)$/) {
            $copiedFromPath = $1;
            $copiedFiles{$indexPath} = $copiedFromPath;
            $versions{$copiedFromPath} = $2 if ($2 != 0);
        }
        elsif (/^--- .+\(revision (\d+)\)$/) {
            $versions{$indexPath} = $1 if ($1 != 0);
        }
        if (s/^\+\+\+ \S+/+++ $indexPath/) {
            $indexPath = "";
        }
    }

[Chris Jerdonek]

Created attachment 46231 [details]
Proposed patch

cq- since parseDiffHeader.pl probably needs the allow-tabs property.

I have a couple questions regarding this patch that maybe you can help me with:

(1) Is the last line of this code ever needed in svn-apply (I have included similar code again in my patch):

> if ($indexPath) {
>     # Fix paths on diff, ---, and +++ lines to match preceding Index: line.
>     s/\S+$/$indexPath/ if /^diff/;

Note that $_ is Git-formatted by the time this line executes.

(2) In svn-apply, we extract the "version" of a diff by matching with--

> /^--- .+\(revision (\d+)\)$/

For consistency, is there any reason not to also record this version number in the case of a file move (in which case there is also a "from" clause after the "revision" portion), e.g.--

> --- WebKitTools/Scripts/webkitpy/move_test.py (revision 53048) (from WebKitTools/Scripts/webkitpy/initial.py:53048)

(3) Finally, in the case of a file move, can the "revision" number ever be different from the revision number that appears in the "from" clause portion?

[WebKit Review Bot]

Attachment 46231 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:99:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:100:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:108:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:109:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:126:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:127:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:135:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:136:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:153:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:154:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:162:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:163:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:180:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:181:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:189:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:190:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 16

[Chris Jerdonek]

(In reply to comment #1)
> (1) Is the last line of this code ever needed in svn-apply (I have included
> similar code again in my patch):
> 
> > if ($indexPath) {
> >     # Fix paths on diff, ---, and +++ lines to match preceding Index: line.
> >     s/\S+$/$indexPath/ if /^diff/;
> 
> Note that $_ is Git-formatted by the time this line executes.

I meant to say SVN-formatted.

A variation of this question is -- what ways are there to generate a diff that has a line beginning with "diff" in the header block (after the leading "Index:" line and before the ending "+++" line)?

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> (In reply to comment #1)
> > (1) Is the last line of this code ever needed in svn-apply (I have included
> > similar code again in my patch):
> > 
> > > if ($indexPath) {
> > >     # Fix paths on diff, ---, and +++ lines to match preceding Index: line.
> > >     s/\S+$/$indexPath/ if /^diff/;
> > 
> > Note that $_ is Git-formatted by the time this line executes.
> 
> I meant to say SVN-formatted.
> 
> A variation of this question is -- what ways are there to generate a diff that
> has a line beginning with "diff" in the header block (after the leading
> "Index:" line and before the ending "+++" line)?

This code has been in svn-apply since its first revision (see svn/git annotate).  I don't think svn patches will ever have a "diff" line, nor will git patches ever have an "Index" line (unless they've been converted).  this was probably to cover all bases at the time.  :)

[Chris Jerdonek]

Created attachment 46237 [details]
Proposed patch 2

Minor clean-ups: some renames, code comments, etc.

Also added two more diff tests: "SVN: new file" and "Git: unrecognized lines".

[WebKit Review Bot]

Attachment 46237 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:99:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:100:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:108:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:109:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:126:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:127:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:135:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:136:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:153:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:154:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:162:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:163:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:180:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:181:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:189:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:190:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:207:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:208:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:216:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:217:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 20

[Chris Jerdonek]

(In reply to comment #4)
> This code has been in svn-apply since its first revision (see svn/git
> annotate).  I don't think svn patches will ever have a "diff" line, nor will
> git patches ever have an "Index" line (unless they've been converted).  this
> was probably to cover all bases at the time.  :)

Thanks. I will remove that line the next time I update the patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> (2) In svn-apply, we extract the "version" of a diff by matching with--
> 
> > /^--- .+\(revision (\d+)\)$/
> 
> For consistency, is there any reason not to also record this version number in
> the case of a file move (in which case there is also a "from" clause after the
> "revision" portion), e.g.--
> 
> > --- WebKitTools/Scripts/webkitpy/move_test.py (revision 53048) (from WebKitTools/Scripts/webkitpy/initial.py:53048)

Bug 12023 introduced this code in r18513.  If I remember correctly, we record the revision number in svn-create-patch so that we can repeat the operation in svn-apply if done at a later time.  I'm not quite sure what you're proposing to do differently, though.  Can you give a more explicit example or ask it a different way (like #1 :)?

<http://trac.webkit.org/changeset/18513>

> (3) Finally, in the case of a file move, can the "revision" number ever be
> different from the revision number that appears in the "from" clause portion?

I think the revision numbers could be different, but you'd want to test using an actual Subversion repository by moving some files around.  Subversion lets you grab revisions of files that aren't necessarily the local, up-to-date revisions and commit them to trunk, so I don't think you can say that the revision numbers always match.

[Chris Jerdonek]

(In reply to comment #8)
> (In reply to comment #1)
> > (2) In svn-apply, we extract the "version" of a diff by matching with--
> > 
> > > /^--- .+\(revision (\d+)\)$/
> > 
> > For consistency, is there any reason not to also record this version number in
> > the case of a file move (in which case there is also a "from" clause after the
> > "revision" portion), e.g.--
> > 
> > > --- WebKitTools/Scripts/webkitpy/move_test.py (revision 53048) (from WebKitTools/Scripts/webkitpy/initial.py:53048)
> 
> Bug 12023 introduced this code in r18513.  If I remember correctly, we record
> the revision number in svn-create-patch so that we can repeat the operation in
> svn-apply if done at a later time.  I'm not quite sure what you're proposing to
> do differently, though.  Can you give a more explicit example or ask it a
> different way (like #1 :)?

Sure.  I realized only after I posted that the questions I wrote were a bit unclear. :) But I also didn't want to overwhelm with too much detail in the first go-round.

For the purposes of the question, I'm going to call the two numbers the "revision" number and the "from" number, respectively.

> --- new.py (revision 53048) (from old.py:53048)

Currently, if there is a "from" number (e.g. if the diff is a file copy), svn-apply looks at the "from" number but not the "revision" number:

> if (/^--- .+\(from (\S+):(\d+)\)$/) {
>     $copiedFromPath = $1;
>     $copiedFiles{$indexPath} = $copiedFromPath;
>     $versions{$copiedFromPath} = $2 if ($2 != 0);
> }

And if there is a "revision" number but no "from" number, svn-apply looks at the "revision" number:

> elsif (/^--- .+\(revision (\d+)\)$/) {
>    $versions{$indexPath} = $1 if ($1 != 0);

It uses these two values as the values in the $versions hash (as opposed to using the "revision" number in both cases).

I guess my original question (2) has a few sub-questions--

In the new code, would it make sense to store the "revision" number as a property of the header in both cases, or is the revision number meaningless for our purposes in the case of a file copy? (Note that the new code just parses and stores the values for later, so it won't affect the behavior of svn-apply, etc.) And in either case, what would be the best name for these numbers -- viewed as properties of the diff header?

Some possible permutations of the values are:

$header{copiedFromVersion}: don't use
$header{version}: "from" in the case of a copy; "revision" otherwise

$header{copiedFromVersion}: "from" in the case of a copy; undef otherwise
$header{version}: undef in the case of a copy; "revision" otherwise

$header{copiedFromVersion}: "from" in the case of a copy; undef otherwise
$header{version}: "revision" in both cases

Secondly, what do we lose in our current code by not taking into account the "revision" number in the case of a file copy?  Perhaps I'm just asking how we knew or why we chose in our current code to use the "from" number in that case rather than the "revision" number.

My original question (3) was basically asking in what circumstances the "from" and "revision" numbers could be different.  I was asking that to shed light on (2).  If the two numbers are always the same, then it simplifies many of the questions above.

The couple times I tried, the numbers were the same.  But maybe I need to try copying a file that was changed in a later revision that I hadn't yet obtained.

[Chris Jerdonek]

*** Bug 33134 has been marked as a duplicate of this bug. ***

[Chris Jerdonek]

(In reply to comment #9)
> My original question (3) was basically asking in what circumstances the "from"
> and "revision" numbers could be different.  I was asking that to shed light on
> (2).  If the two numbers are always the same, then it simplifies many of the
> questions above.
> 
> The couple times I tried, the numbers were the same.  But maybe I need to try
> copying a file that was changed in a later revision that I hadn't yet obtained.

In starting to look at the earlier report you mentioned (thanks for that, by the way)--

https://bugs.webkit.org/show_bug.cgi?id=12023

It looks like it's possible the two numbers are always the same (in which case the header would contain redundant information):

+sub manufacturePatchForAdditionWithHistory($$)
+{
+    my ($file, $isBinary) = @_;
+    print "Index: ${file}\n";
+    print "=" x 67, "\n";
+    my ($sourceFile, $sourceRevision) = findSourceFileAndRevision($file);
+    print "--- ${file}\t(revision ${sourceRevision})\t(from ${sourceFile}:${sourceRevision})\n"; ###
+    print "+++ ${file}\t(working copy)\n";

I don't know yet if there are other places where a diff header of this form can get generated, or if there are cases in the future where we might want to allow the two numbers to be different.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> (In reply to comment #9)
> > My original question (3) was basically asking in what circumstances the "from"
> > and "revision" numbers could be different.  I was asking that to shed light on
> > (2).  If the two numbers are always the same, then it simplifies many of the
> > questions above.
> > 
> > The couple times I tried, the numbers were the same.  But maybe I need to try
> > copying a file that was changed in a later revision that I hadn't yet obtained.
> 
> In starting to look at the earlier report you mentioned (thanks for that, by
> the way)--
> 
> https://bugs.webkit.org/show_bug.cgi?id=12023
> 
> It looks like it's possible the two numbers are always the same (in which case
> the header would contain redundant information):
> 
> +sub manufacturePatchForAdditionWithHistory($$)
> +{
> +    my ($file, $isBinary) = @_;
> +    print "Index: ${file}\n";
> +    print "=" x 67, "\n";
> +    my ($sourceFile, $sourceRevision) = findSourceFileAndRevision($file);
> +    print "--- ${file}\t(revision ${sourceRevision})\t(from
> ${sourceFile}:${sourceRevision})\n"; ###
> +    print "+++ ${file}\t(working copy)\n";
> 
> I don't know yet if there are other places where a diff header of this form can
> get generated, or if there are cases in the future where we might want to allow
> the two numbers to be different.

Okay, after rereading Bug 12023 and playing around with svn, I now remember that the "(from Path/To/File.cpp:NNNNN)" information in the svn-create-patch output for copied/moved files was created solely by me for the purposes of communicating source information in patches from svn-create-patch to svn-apply (and svn-unapply).  That means the revisions will always match due to the code in svn-create-patch that you quoted above.  The revisions are "redundant" because I felt it was useful to include the @revision information with the path in the annotation.

So the revisions are redundant, but I don't think they hurt anything.  I'm more inclined to leave the format as-is unless there is a compelling reason to change it (other than duplicate information) since there are a number of patches on bugs.webkit.org that use that format.

[David Kilzer (:ddkilzer)]

Comment on attachment 46237 [details]
Proposed patch 2

> Index: WebKitTools/Scripts/VCSUtils.pm
> +# Parse the next diff header from the given file handle, and advance
> +# the file handle so the last line read is the first line after the
> +# parsed header block.
> +#
> +# This subroutine dies if given leading junk.
> +#
> +# Args:
> +#   $fileHandle: advanced so the last line read is the first line of the
> +#                next diff header. For SVN-formatted diffs, this is the
> +#                "Index: " line.
> +#   $lastReadLine: the last line read from $fileHandle

Nit:  This is named $line, not $lastReadLine in the method.

> +# Returns ($headerHashRef, $foundHeaderEnding, $lastReadLine):
> +#   $headerHashRef: a hash reference representing a diff header
> +#     copiedFromPath:
> +#     copiedFromVersion:
> +#     indexPath:
> +#     svnConvertedText: header text converted to SVN format. Unrecognized
> +#                       Git lines are left alone.
> +#     version:
> +#   $foundHeaderEnding: whether the last header block line was found.
> +#                       This is a line beginning with "+++".
> +#   $lastReadLine:

I think $lastReadLine's description is on the previous line?  Or is there just no description?  Capitalization and indenting make this hard to read.  I think all the various should have some kind of a description.

> Index: WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl
> +{
> +    # New test
> +    diffName => "Git: unrecognized lines",
> +    inputText => <<'END',
> +diff --git a/LayoutTests/http/tests/security/listener/xss-inactive-closure.html b/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> +new file mode 100644
> +index 0000000..3c9f114
> +--- /dev/null
> ++++ b/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> +@@ -0,0 +1,34 @@
> ++<html>
> +END
> +    # Header keys to check
> +    svnConvertedText => <<'END',
> +Index: LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> +new file mode 100644
> +index 0000000..3c9f114
> +--- LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> ++++ LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> +END
> +    copiedFromPath => undef,
> +    copiedFromVersion => undef,
> +    indexPath => "LayoutTests/http/tests/security/listener/xss-inactive-closure.html",
> +    version => undef,
> +    # Other values to check
> +    foundHeaderEnding => 1,
> +    lastReadLine => "@@ -0,0 +1,34 @@\n",
> +    nextLine => "+<html>\n",
> +},
> +);

Heh, this test output will fail to apply.  Is that why you're writing these tests (to fix that)?  :)

> +plan tests => @diffTests * 8;

I've never really been a fan of missing parenthesis in Perl.  I think this would read better as:

plan(tests => @diffTests * 8);

It's up to you, though.  It also took me a while to get used to warn/die statements without parenthesis.

r=me, but it would be nice if the method documentation issues were addressed.

[Chris Jerdonek]

(In reply to comment #12)
> Okay, after rereading Bug 12023 and playing around with svn, I now remember
> that the "(from Path/To/File.cpp:NNNNN)" information in the svn-create-patch
> output for copied/moved files was created solely by me for the purposes of
> communicating source information in patches from svn-create-patch to svn-apply
> (and svn-unapply).  That means the revisions will always match due to the code
> in svn-create-patch that you quoted above.  The revisions are "redundant"
> because I felt it was useful to include the @revision information with the path
> in the annotation.
> 
> So the revisions are redundant, but I don't think they hurt anything.  I'm more
> inclined to leave the format as-is unless there is a compelling reason to
> change it (other than duplicate information) since there are a number of
> patches on bugs.webkit.org that use that format.

That's fine -- thanks.  I was asking more for the purposes of understanding and writing the code rather than changing the svn-create-patch "API".  I will document the code accordingly and perhaps adjust how I'm storing and naming the values in the header hash (I might do this in a subsequent revision).

[Chris Jerdonek]

(In reply to comment #13)
> > Index: WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl
> > +{
> > +    # New test
> > +    diffName => "Git: unrecognized lines",
> > +    inputText => <<'END',
> > +diff --git a/LayoutTests/http/tests/security/listener/xss-inactive-closure.html b/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > +new file mode 100644
> > +index 0000000..3c9f114
> > +--- /dev/null
> > ++++ b/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > +@@ -0,0 +1,34 @@
> > ++<html>
> > +END
> > +    # Header keys to check
> > +    svnConvertedText => <<'END',
> > +Index: LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > +new file mode 100644
> > +index 0000000..3c9f114
> > +--- LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > ++++ LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > +END
> > +    copiedFromPath => undef,
> > +    copiedFromVersion => undef,
> > +    indexPath => "LayoutTests/http/tests/security/listener/xss-inactive-closure.html",
> > +    version => undef,
> > +    # Other values to check
> > +    foundHeaderEnding => 1,
> > +    lastReadLine => "@@ -0,0 +1,34 @@\n",
> > +    nextLine => "+<html>\n",
> > +},
> > +);
> 
> Heh, this test output will fail to apply.

I didn't know that. :) I just observed by inspection that this is how the current corresponding svn-apply code behaves.  I thought that perhaps svn-apply/unapply relied on this "pass-through" behavior, or else behaved loosely so as not to preempt use of the --force flag.

> Is that why you're writing these tests (to fix that)?  :)

The test case was to document interesting behavior that I thought was supposed to be preserved -- to help me while refactoring.  In a later patch, I can change the method to suppress unrecognized lines if that's what's preferred.

Thanks for reading and thinking about these.

> > +plan tests => @diffTests * 8;
> 
> I've never really been a fan of missing parenthesis in Perl.  I think this
> would read better as:
> 
> plan(tests => @diffTests * 8);

That's fine.  I prefer parentheses, too.  I was just following the examples in the Test::Simple/Test::More documentation (and frankly, didn't even notice that this was a construct in which parentheses could be used).

[Chris Jerdonek]

(In reply to comment #13)
> > +# Returns ($headerHashRef, $foundHeaderEnding, $lastReadLine):
> > +#   $headerHashRef: a hash reference representing a diff header
> > +#     copiedFromPath:
> > +#     copiedFromVersion:
> > +#     indexPath:
> > +#     svnConvertedText: header text converted to SVN format. Unrecognized
> > +#                       Git lines are left alone.
> > +#     version:
> > +#   $foundHeaderEnding: whether the last header block line was found.
> > +#                       This is a line beginning with "+++".
> > +#   $lastReadLine:
> 
> I think $lastReadLine's description is on the previous line?  Or is there just
> no description?  Capitalization and indenting make this hard to read.

Let me know if you have any suggestions for formatting (multi-valued) return values in general (accommodating hash key documentation, etc).  I lifted and modified this format from some WebKit Python code (hence the indenting).

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> (In reply to comment #13)
> > > +# Returns ($headerHashRef, $foundHeaderEnding, $lastReadLine):
> > > +#   $headerHashRef: a hash reference representing a diff header
> > > +#     copiedFromPath:
> > > +#     copiedFromVersion:
> > > +#     indexPath:
> > > +#     svnConvertedText: header text converted to SVN format. Unrecognized
> > > +#                       Git lines are left alone.
> > > +#     version:
> > > +#   $foundHeaderEnding: whether the last header block line was found.
> > > +#                       This is a line beginning with "+++".
> > > +#   $lastReadLine:
> > 
> > I think $lastReadLine's description is on the previous line?  Or is there just
> > no description?  Capitalization and indenting make this hard to read.
> 
> Let me know if you have any suggestions for formatting (multi-valued) return
> values in general (accommodating hash key documentation, etc).  I lifted and
> modified this format from some WebKit Python code (hence the indenting).

I'm just expecting text (a description) after every "variable:" line.  If there is no text, does the next line with a description cover it?  I'm just not sure how to parse this format when I read it.

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> (In reply to comment #13)
> > > Index: WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl
> > > +{
> > > +    # New test
> > > +    diffName => "Git: unrecognized lines",
> > > +    inputText => <<'END',
> > > +diff --git a/LayoutTests/http/tests/security/listener/xss-inactive-closure.html b/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > > +new file mode 100644
> > > +index 0000000..3c9f114
> > > +--- /dev/null
> > > ++++ b/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > > +@@ -0,0 +1,34 @@
> > > ++<html>
> > > +END
> > > +    # Header keys to check
> > > +    svnConvertedText => <<'END',
> > > +Index: LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > > +new file mode 100644
> > > +index 0000000..3c9f114
> > > +--- LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > > ++++ LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> > > +END
> > > +    copiedFromPath => undef,
> > > +    copiedFromVersion => undef,
> > > +    indexPath => "LayoutTests/http/tests/security/listener/xss-inactive-closure.html",
> > > +    version => undef,
> > > +    # Other values to check
> > > +    foundHeaderEnding => 1,
> > > +    lastReadLine => "@@ -0,0 +1,34 @@\n",
> > > +    nextLine => "+<html>\n",
> > > +},
> > > +);
> > 
> > Heh, this test output will fail to apply.
> 
> I didn't know that. :) I just observed by inspection that this is how the
> current corresponding svn-apply code behaves.  I thought that perhaps
> svn-apply/unapply relied on this "pass-through" behavior, or else behaved
> loosely so as not to preempt use of the --force flag.

I think this may be covered by Bug 32834, although maybe not explicitly.

[Chris Jerdonek]

Created attachment 46250 [details]
Proposed patch 3

[WebKit Review Bot]

Attachment 46250 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:99:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:100:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:108:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:109:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:126:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:127:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:135:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:136:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:153:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:154:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:162:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:163:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:180:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:181:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:189:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:190:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:207:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:208:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:216:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:217:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 20

[David Kilzer (:ddkilzer)]

Comment on attachment 46250 [details]
Proposed patch 3

r=me

I assume there isn't anything else you wanted to change?

[Chris Jerdonek]

(In reply to comment #21)
> (From update of attachment 46250 [details])
> r=me
> 
> I assume there isn't anything else you wanted to change?

Not in this patch.  Could you do me a favor and commit this for me?  I believe it won't commit otherwise since I think the unit test file needs the allow-tabs property.  Thanks a lot!

[David Kilzer (:ddkilzer)]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 46250 [details] [details])
> > r=me
> > 
> > I assume there isn't anything else you wanted to change?
> 
> Not in this patch.  Could you do me a favor and commit this for me?  I believe
> it won't commit otherwise since I think the unit test file needs the allow-tabs
> property.  Thanks a lot!

Will do this tomorrow unless someone feels like landing it overnight.  Thanks!

[David Kilzer (:ddkilzer)]

Committed r53076: <http://trac.webkit.org/changeset/53076>