Bug 39170 – Add support function to parse SVN single-line and multi-line property value

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

As part of the infrastructure to parse SVN property changes, we need to be able to parse single-line and multi-line property values.

[Daniel Bates]

Created attachment 56171 [details]
Patch with unit tests

[Eric Seidel (no email)]

 I wonder as I read this:  doesn't svn have some sort of xml output mode?  Would that be easier to make our scripts use?  Or am I just drinking the xml koolaid and making non-sequitor comments on bugs?  ;)

[Chris Jerdonek]

Comment on attachment 56171 [details]
Patch with unit tests

Thanks, Dan!  Comments below.  r- to consider them:

> Index: WebKitTools/ChangeLog
> +        Add function parseSvnPropertyPlusOrMinus to parse single-line and multi-line
> +        property values.

Would parseSvnPropertyValue() be more direct?

> +
> +        * Scripts/VCSUtils.pm:
> +            Added function parseSvnPropertyPlusOrMinus. We will use this as part of
> +            Bug #38885.

It might be helpful to include a bug URL for clickability.

> Index: WebKitTools/Scripts/VCSUtils.pm
>  
>  my $gitDiffStartRegEx = qr#^diff --git (\w/)?(.+) (\w/)?([^\r\n]+)#;
>  my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> +my $svnPropertyChangeStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $1 is the name of the property
> +my $svnPropertyValueRegEx = qr#^   (\+|-) ([^\r\n]+)#; # $2 is the start of the property's value (which may span multiple lines).

Would $svnPropertyValueStartRegEx be better since it is just the first line?

> +# Parse the property value of an SVN property from the given file handle, and advance
> +# the handle so the last line read is the first line after the property value.

I would abbreviate this to, "Parse the value of an SVN property change from..."
and then in a separate paragraph include that an SVN property change is
a section in the footer of an SVN diff with one or two property value 
sub-sections.

> +# This subroutine dies if the first line is not a valid SVN property value line
> +# (i.e. a line that begins with "   +" or "   -").

"...not the beginning of a valid..."

> +#   $fileHandle: advanced so the last line read from the handle is the first
> +#                line of the property value to parse.  This should be a line
> +#                beginning with "   +" or "   -".

You already said this above.  Probably just include it in one of the two
places.

> +# FIXME: This method is unused as of (05/15/2010). We will call this function
> +#        as part of parsing a property from the SVN footer. See Bug #38885.

Maybe include bug URL.

> +sub parseSvnPropertyPlusOrMinus($$)
> +{
> +    my ($fileHandle, $line) = @_;
> +
> +    $_ = $line;
> +
> +    my $propertyValue;
> +    if (/$svnPropertyValueRegEx/) {
> +        $propertyValue = $2; # Does not include the end-of-line character(s)
> +    } else {
> +        die("Failed to find '+' or '-' property: \"$_\".");

Maybe "Failed to find property value beginning with '+' or '-': ..."?

> +    }
> +
> +    $_ = <$fileHandle>;
> +    $propertyValue .= $POSTMATCH if defined($_); # Preserve the end-of-line of the first line in a multi-line property value.

It looks like you're including the end-of-line characters even if the
property value has just one line.  Also, could you possibly rework things
so that the $POSTMATCH value is read from closer to the reg ex (e.g. right
after where $propertyValue is set to $2?

> +# The array of test cases.

This is probably redundant (even though it's in the other files).

> +my @testCaseHashRefs = (
> +{
> +    # New test

Is it necessary to write "# New test" at the beginning of each test case?

[Daniel Bates]

(In reply to comment #2)
>  I wonder as I read this:  doesn't svn have some sort of xml output mode?  Would that be easier to make our scripts use?  Or am I just drinking the xml koolaid and making non-sequitor comments on bugs?  ;)

I think we should look into this further.

From the SVN book, it appears XML output was added somewhere after SVN 1.4 (appears in 1.5, http://svnbook.red-bean.com/en/1.5/svn.ref.svn.c.diff.html). Last I recall, Leopard 10.5.8 ships with 1.4.4, which may or may not support it. I doubt Tiger ships with a version of SVN with such support.

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 56171 [details])
> Thanks, Dan!  Comments below.  r- to consider them:
> 
> > Index: WebKitTools/ChangeLog
> > +        Add function parseSvnPropertyPlusOrMinus to parse single-line and multi-line
> > +        property values.
> 
> Would parseSvnPropertyValue() be more direct?

Will change.

> 
> > +
> > +        * Scripts/VCSUtils.pm:
> > +            Added function parseSvnPropertyPlusOrMinus. We will use this as part of
> > +            Bug #38885.
> 
> It might be helpful to include a bug URL for clickability.

Will change.

> 
> > Index: WebKitTools/Scripts/VCSUtils.pm
> >  
> >  my $gitDiffStartRegEx = qr#^diff --git (\w/)?(.+) (\w/)?([^\r\n]+)#;
> >  my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> > +my $svnPropertyChangeStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $1 is the name of the property
> > +my $svnPropertyValueRegEx = qr#^   (\+|-) ([^\r\n]+)#; # $2 is the start of the property's value (which may span multiple lines).
> 
> Would $svnPropertyValueStartRegEx be better since it is just the first line?

Will change.

> 
> > +# Parse the property value of an SVN property from the given file handle, and advance
> > +# the handle so the last line read is the first line after the property value.
> 
> I would abbreviate this to, "Parse the value of an SVN property change from..."
> and then in a separate paragraph include that an SVN property change is
> a section in the footer of an SVN diff with one or two property value 
> sub-sections.
> 
> > +# This subroutine dies if the first line is not a valid SVN property value line
> > +# (i.e. a line that begins with "   +" or "   -").
> 
> "...not the beginning of a valid..."

Will change.

> 
> > +#   $fileHandle: advanced so the last line read from the handle is the first
> > +#                line of the property value to parse.  This should be a line
> > +#                beginning with "   +" or "   -".
> 
> You already said this above.  Probably just include it in one of the two
> places.
> 
> > +# FIXME: This method is unused as of (05/15/2010). We will call this function
> > +#        as part of parsing a property from the SVN footer. See Bug #38885.
> 
> Maybe include bug URL.
> 
> > +sub parseSvnPropertyPlusOrMinus($$)
> > +{
> > +    my ($fileHandle, $line) = @_;
> > +
> > +    $_ = $line;
> > +
> > +    my $propertyValue;
> > +    if (/$svnPropertyValueRegEx/) {
> > +        $propertyValue = $2; # Does not include the end-of-line character(s)
> > +    } else {
> > +        die("Failed to find '+' or '-' property: \"$_\".");
> 
> Maybe "Failed to find property value beginning with '+' or '-': ..."?
> 
> > +    }
> > +
> > +    $_ = <$fileHandle>;
> > +    $propertyValue .= $POSTMATCH if defined($_); # Preserve the end-of-line of the first line in a multi-line property value.
> 
> It looks like you're including the end-of-line characters even if the
> property value has just one line.  Also, could you possibly rework things
> so that the $POSTMATCH value is read from closer to the reg ex (e.g. right
> after where $propertyValue is set to $2?
> 
> > +# The array of test cases.
> 
> This is probably redundant (even though it's in the other files).

Will remove, note it appears in parseDiff.pl, parseGitDiffHeader.pl and parseSvnDiffHeader.pl.

> 
> > +my @testCaseHashRefs = (
> > +{
> > +    # New test
> 
> Is it necessary to write "# New test" at the beginning of each test case?

I think it is easier to identify the test cases using this comment, although we should find a better way to demarcate the test cases, if possible.

[Daniel Bates]

Created attachment 56172 [details]
Patch with unit tests

[Daniel Bates]

Created attachment 56173 [details]
Patch with unit tests

Added additional unit tests: "multi-line '-' change followed by '+' single-line change" and "multi-line '-' change followed by '+' multi-line change".

[Chris Jerdonek]

Comment on attachment 56173 [details]
Patch with unit tests

LGTM.  Thanks for the changes.

[Daniel Bates]

Comment on attachment 56173 [details]
Patch with unit tests

Clearing flags on attachment: 56173

Committed r59566: <http://trac.webkit.org/changeset/59566>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Chris Jerdonek]

FYI, I just discovered that prepare-ChangeLog has some code to parse property changes from an SVN diff.  It would be worth studying that to see what code can be shared, etc:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/prepare-ChangeLog?rev=58327#L1342

[Daniel Bates]

(In reply to comment #11)
> FYI, I just discovered that prepare-ChangeLog has some code to parse property changes from an SVN diff.  It would be worth studying that to see what code can be shared, etc:
> 
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/prepare-ChangeLog?rev=58327#L1342

Notice, this code is used to determine the names of the properties that have been added, deleted, or modified on a file. But, it doesn't parse a change for its value (*), which is necessary to apply or unapply the change using svn-apply and svn-unapply, respectively.

(*) Disregarding the "Name" and "Modified" type properties, this code would work to determine the value of the svn:executable bit since the svn:executable property has no defined value as per <http://svnbook.red-bean.com/en/1.5/svn.advanced.props.file-portability.html#svn.advanced.props.special.executable>. Hence, we can infer whether it was added or removed from its property type. Clearly, this code cannot detect the value of the svn:executable property for SVN version < 1.5. Notice, Leopard 10.5.8 ships with SVN 1.4.4.

[Chris Jerdonek]

(In reply to comment #12)
> Notice, this code is used to determine the names of the properties that have been added, deleted, or modified on a file. But, it doesn't parse a change for its value (*), which is necessary to apply or unapply the change using svn-apply and svn-unapply, respectively.

Would it make sense for prepare-ChangeLog to use the new code that you're writing?  Maybe you can add a FIXME to prepare-ChangeLog.