Bug 38425 – svn-apply: Add a parseGitDiffHeader() method to VCSUtils.pm

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

This method should be analogous to the existing parseDiffHeader() but add the feature of setting an "executableBitDelta" value in the returned diff hash.

[Chris Jerdonek]

Created attachment 54872 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 54873 [details]
Proposed patch 2

Adjusted code comments.

[Chris Jerdonek]

Created attachment 54877 [details]
Proposed patch 3

Fixed ternary expression and regular expression errors along with two test cases.

[Kenneth Rohde Christiansen]

Are you going to handle moves/renames as well?

[Chris Jerdonek]

(In reply to comment #4)
> Are you going to handle moves/renames as well?

For git diffs?  I'm not personally planning on doing that in the near future, though the refactoring I recently completed probably makes implementing that easier:

http://trac.webkit.org/changeset/58495

Is there a bug number?

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> (In reply to comment #4)
> > Are you going to handle moves/renames as well?
> 
> For git diffs?  I'm not personally planning on doing that in the near future,
> though the refactoring I recently completed probably makes implementing that
> easier:
> 
> http://trac.webkit.org/changeset/58495
> 
> Is there a bug number?

32834 :-) and there are already comments from you.

[Daniel Bates]

Comment on attachment 54877 [details]
Proposed patch 3

This patch looks good. I have a couple of stylistic nits.

> +sub isExecutable($)
> +{
> +    my $fileMode = shift;
> +
> +    return ($fileMode % 2 != 0)

The last line can be written: return $fileMode % 2;

> +}
> +
> +# 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.
> +#
> +# This subroutine dies if given leading junk.
> +#
> +# Args:
> +#   $fileHandle: advanced so the last line read is the first line of the
> +#                header.  This is a line beginning with "diff --git ...."

When I first read this I was a bit confused. I believe what you mean is:

$fileHandle: advanced so the last line read is the first line of the
             header in the next diff. This is a line beginning with "diff --git ....".

Notice, I added both the phrase "in the next diff" at the end of the first sentence and a period at the end of the last sentence.

> +#   $line: the line last read from $fileHandle
[...]
> +sub parseGitDiffHeader($$)
> +{
> +    my ($fileHandle, $line) = @_;
> +
> +    $_ = $line;
> +
> +    my $headerStartRegEx = qr/^diff --git /;
> +
> +    if (!/$headerStartRegEx/) {
> +        die("First line of Git diff does not begin with \"diff --git \": \"$_\"");
> +    }

We can write this last if-statement in one line (and hence eliminate the curly-braces):

die("First line of Git diff does not begin with \"diff --git \": \"$_\"") if !/$headerStartRegEx/;

OR

die("First line of Git diff does not begin with \"diff --git \": \"$_\"") unless /$headerStartRegEx/;

> +
> +    my $foundHeaderEnding;
> +    my $indexPath;
> +    my $newExecutableBit = 0;
> +    my $oldExecutableBit = 0;
> +    my $svnConvertedText;
> +    while (1) {
> +        # Temporarily strip off any end-of-line characters to simplify
> +        # regex matching below.
> +        s/([\n\r]+)$//;
> +        my $eol = $1;
> +
> +        if (m#^diff --git \w/(.+) \w/([^\r\n]+)#) {
> +            $indexPath = $1;
> +            $_ = "Index: $indexPath"; # Convert to SVN format.
> +        } elsif (/^(deleted file|old) mode ([0-9]{6})/) {
> +            $oldExecutableBit = (isExecutable($2) ? 1 : 0);
> +        } elsif (/^new( file)? mode ([0-9]{6})/) {
> +            $newExecutableBit = (isExecutable($2) ? 1 : 0);
> +        } elsif (/^--- \S+/) {
> +            $_ = "--- $indexPath"; # Convert to SVN format.
> +        } elsif (/^\+\+\+ \S+/) {
> +            $_ = "+++ $indexPath"; # Convert to SVN format.
> +            $foundHeaderEnding = 1;
> +        } elsif (/^GIT binary patch$/ ) {
> +            $foundHeaderEnding = 1;
> +        }
> +
> +        $svnConvertedText .= "$_$eol"; # Also restore end-of-line characters.
> +
> +        $_ = <$fileHandle>; # Not defined if end-of-file reached.
> +
> +        if (!defined($_) || /$headerStartRegEx/ || $foundHeaderEnding) {
> +            last;
> +        }

Similarly, we can write the last if-statement in one line:

last if (!defined($_) || /$headerStartRegEx/ || $foundHeaderEnding);

> +}

[Daniel Bates]

Forgot to mention, this patch need to be rebased as it no longer applies cleanly since the passing of change set 58663 <http://trac.webkit.org/changeset/58663>.

[Chris Jerdonek]

(In reply to comment #7)

Thanks for the comments, Daniel.

> > +#   $fileHandle: advanced so the last line read is the first line of the
> > +#                header.  This is a line beginning with "diff --git ...."
> 
> $fileHandle: advanced so the last line read is the first line of the
>              header in the next diff. This is a line beginning with "diff --git
> ....".
> 
> Notice, I added both the phrase "in the next diff" at the end of the first
> sentence and a period at the end of the last sentence.

I usually do three periods in an ellipsis in the middle of a sentence and four when the ellipsis is at the end (the fourth doing double-duty as the sentence-ending period).  In high school, I learned that periods and commas at the end of a quotation are supposed to go inside the quotes (while semi-colons and colons go outside).  However, I sometimes violate this when I'm quoting an identifier and want to emphasize that the terminating period is not part of the identifier.

> > +    if (!/$headerStartRegEx/) {
> > +        die("First line of Git diff does not begin with \"diff --git \": \"$_\"");
> > +    }
> 
> We can write this last if-statement in one line (and hence eliminate the
> curly-braces):

This may be a personal judgment call, but I think it may be easier to read in this case if one's eyes don't need to pass over the entire message to find out whether the die message is relevant.

[Daniel Bates]

(In reply to comment #9)
> (In reply to comment #7)
> 
> Thanks for the comments, Daniel.
> 
> > > +#   $fileHandle: advanced so the last line read is the first line of the
> > > +#                header.  This is a line beginning with "diff --git ...."
> > 
> > $fileHandle: advanced so the last line read is the first line of the
> >              header in the next diff. This is a line beginning with "diff --git
> > ....".
> > 
> > Notice, I added both the phrase "in the next diff" at the end of the first
> > sentence and a period at the end of the last sentence.
> 
> I usually do three periods in an ellipsis in the middle of a sentence and four
> when the ellipsis is at the end (the fourth doing double-duty as the
> sentence-ending period).  In high school, I learned that periods and commas at
> the end of a quotation are supposed to go inside the quotes (while semi-colons
> and colons go outside).  However, I sometimes violate this when I'm quoting an
> identifier and want to emphasize that the terminating period is not part of the
> identifier.

This is all a matter of taste. So, I'll leave it up to you to decide what you think reads best.

I agree that the placement of the period should be inside the quotation for non-technical English writing. For technical documentation meant for a technical audience, I would place the period outside the quote to demarcate the truncation of the string literal and the end of the sentence: This is a line beginning with "diff --git ...".

Alternatively, maybe rewrite the sentence entirely.

For your reference, see Hacker Writing Style of the The New Hacker's Dictionary <http://catb.org/jargon/html/writing-style.html>.

> 
> > > +    if (!/$headerStartRegEx/) {
> > > +        die("First line of Git diff does not begin with \"diff --git \": \"$_\"");
> > > +    }
> > 
> > We can write this last if-statement in one line (and hence eliminate the
> > curly-braces):
> 
> This may be a personal judgment call, but I think it may be easier to read in
> this case if one's eyes don't need to pass over the entire message to find out
> whether the die message is relevant.

Also a matter of taste. I'm fine with the way you have it written now.

[Chris Jerdonek]

Created attachment 54911 [details]
Proposed patch 4

[Chris Jerdonek]

(In reply to comment #10)
> For technical documentation meant for a
> technical audience, I would place the period outside the quote to demarcate the
> truncation of the string literal and the end of the sentence: This is a line
> beginning with "diff --git ...".

Yes, I would normally agree, but this isn't a string literal (the line doesn't actually contain three dots).  In any case, I excluded the ellipsis entirely since "beginning with 'diff ...'" is redundant.

[Daniel Bates]

Comment on attachment 54911 [details]
Proposed patch 4

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 54911 [details]
Proposed patch 4

Clearing flags on attachment: 54911

Committed r58681: <http://trac.webkit.org/changeset/58681>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.