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; > }
Created attachment 55153 [details] Proposed patch
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?
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.
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.
(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.
(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.
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.
Comment on attachment 55153 [details] Proposed patch Clearing flags on attachment: 55153 Committed r58929: <http://trac.webkit.org/changeset/58929>
All reviewed patches have been landed. Closing bug.