Bug 38320 – svn-apply: Add an "isBinary" property to the hashes returned by parseDiff() and parseDiffHeader()

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

[Chris Jerdonek]

This will allow us to address part of this FIXME in both svn-apply and svn-unapply:

>    # FIXME: This information should be extracted from the diff file as
>    #        part of the parsing stage, i.e. the call to parsePatch().
>    $addition = 1 if ($patch =~ /\n--- .+\(revision 0\)\r?\n/ || $patch =~ /\n@@ -0,0 .* @@/) && !exists($copiedFiles{$fullPath});
>    $deletion = 1 if $patch =~ /\n@@ .* \+0,0 @@/;
>    $isBinary = 1 if $patch =~ /\nCannot display: file marked as a binary type\./;
>    $isGitBinary = 1 if $patch =~ /\nGIT binary patch\n/;
 
This can be fixed by adjusting this area of VCSUtils's parseDiffHeader():

>        } elsif (s/^\+\+\+ \S+/+++ $indexPath/ ||
>                 /^Cannot display: file marked as a binary type.$/ || # SVN binary
>                 /^GIT binary patch$/) {
>            # +++
>            $foundHeaderEnding = 1;
>        }

[Chris Jerdonek]

Created attachment 55153 [details]
Proposed patch

[Chris Jerdonek]

I didn't know until now that svn-unapply didn't support binary files in Git diffs.  How is the commit-queue, etc. able to get away with that?

[Eric Seidel (no email)]

The commit-queue does not use svn-unapply.

svn-unapply is basically quilt-like functionality which Darin Adler and others built in WebKit to work around SVN's short-commings.

[Daniel Bates]

How did you come to the decision to remove scmFormat and instead use two different keys isSvn and isGit? I take it to get rid of doing a string comparison.

[Chris Jerdonek]

(In reply to comment #4)
> How did you come to the decision to remove scmFormat and instead use two
> different keys isSvn and isGit? I take it to get rid of doing a string
> comparison.

Yes, more or less.  I originally thought that a single key-value for the type would be simplest.  But in the end the code turned out to be cleaner with two key-value pairs isGit and isSvn.

[Chris Jerdonek]

(In reply to comment #5)
> (In reply to comment #4)
> > How did you come to the decision to remove scmFormat and instead use two
> > different keys isSvn and isGit? I take it to get rid of doing a string
> > comparison.
> 
> Yes, more or less.  I originally thought that a single key-value for the type
> would be simplest.  But in the end the code turned out to be cleaner with two
> key-value pairs isGit and isSvn.

Also, I'm now convinced that using "key not present" is acceptable and even desirable to represent false for our diffHash-related objects.  Otherwise, each time we add a specialized property we would have to update every unit test, which isn't scalable.

Given that "key not present" is acceptable, having two key-values isGit and isSvn becomes more acceptable since then only one would ever have to be set.

[Daniel Bates]

Comment on attachment 55153 [details]
Proposed patch

Looks good to me.

By the way, I've noticed that we interchange between "SVN", "Svn", and "svn" throughout VCSUtils. Similarly, we interchange between "git" and "Git". We should consider standardizing around one. This does not need to be addressed in this patch, just wanted to point it out.

[Chris Jerdonek]

Comment on attachment 55153 [details]
Proposed patch

Clearing flags on attachment: 55153

Committed r58929: <http://trac.webkit.org/changeset/58929>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.