Bug 27204

Yeah, this has nothing really to do with bugzilla-tool. It's more svn-apply. Although I guess we'd have to make git format-patch understand executable bit too...

Mark Rowe tells me that git already understands the executable bit. That's what the modes are telling you in git patch files.

Ok. The problem is still on the patch-application end I would assume. I know that svn-create-patch includes this kind of information. So we just need to make svn-apply smart enough to use it (I'm kinda surprised it is not already).

Turns out that this is actually already listed as a "fixme" in svn-apply: # Missing features: # # Handle property changes.

Bit us again in http://trac.webkit.org/changeset/51475

Committed r51477: <http://trac.webkit.org/changeset/51477>

Hit again for bug 32919 in r52646. Fixed in r52658. <http://trac.webkit.org/changeset/52646> <http://trac.webkit.org/changeset/52658>

Created attachment 52933 [details] Patch As a holdover to the complete refactoring or complete rewrite of svn-apply/unapply, I have put together a patch to add executable bit support.

Comment on attachment 52933 [details] Patch I don't see any "complete-rewrites" as imminent. :) Did you mean to mark this for review?

Created attachment 52937 [details] Patch with unit test

Comment on attachment 52937 [details] Patch with unit test > +sub isSVNProperty($) > +{ > + my ($patch) = @_; > + > + return 1 if $patch =~/\n(Added|Deleted): svn:executable\n/; > + return 0; > +} Nit: Space between "=~" and "/" (this occurs in many different places) Nit: I just looked it up and =~ returns true/false (1 or 0) automatically: http://perldoc.perl.org/perlrequick.html#Simple-word-matching Thus the last two lines could just be: return $patch =~/\n(Added|Deleted): svn:executable\n/; > +++ WebKitTools/Scripts/webkitperl/VCSUtils_unittest/appendSVNExecutableBitChangeToPatch.pl (revision 0) > @@ -0,0 +1,68 @@ > +#!/usr/bin/perl > +# > +# Copyright (C) Research in Motion Limited 2010. All rights reserved. > +# > +# Redistribution and use in source and binary forms, with or without > +# modification, are permitted provided that the following conditions are > +# met: > +# > +# * Redistributions of source code must retain the above copyright > +# notice, this list of conditions and the following disclaimer. > +# * Redistributions in binary form must reproduce the above > +# copyright notice, this list of conditions and the following disclaimer > +# in the documentation and/or other materials provided with the > +# distribution. > +# * Neither the name of Google Inc. nor the names of its > +# contributors may be used to endorse or promote products derived from > +# this software without specific prior written permission. Nit: Probably want to change "Google" in this copyright. In order to check the regex's for git I had to make a mini repo and see what the output was on a simple diff. You have a test for SVN, would it be possible to create a test for reading a git diff? That would also make it easy for a reviewer to see the format of a git diff for such a case, and how the regexes match it.

Created attachment 52944 [details] Patch with unit tests Updated patch based on Eric's suggestions. Separated the parsing of the Git file mode into its own subroutine parseGitFileMode and added a unit test for it.

Oops, I should say Joseph Pecoraro's suggestions, not Eric's.

Whoops, I wasn't CC'd. The new patch looks better to me. I'd still prefer someone who really works with perl / scripts more often to look it over for a review. Adam or Eric? Thanks for addressing my comments Daniel!

Comment on attachment 52944 [details] Patch with unit tests This look good. But why is adding/removing the property not symmetric? Why can't we just have a "toggle" method for the executable property? Does adding the property not cause svn to chmod +x the file?

Created attachment 53061 [details] Patch with unit tests Removed extraneous function unapplySVNPropertyChange.

Comment on attachment 53061 [details] Patch with unit tests Seems we should add a FIXME in this method about making it more generic: 125 sub isSVNProperty($) 126 { 127 my ($patch) = @_; 128 return $patch =~ /\n(Added|Deleted): svn:executable\n/; We need a comment here to explain why this can't just be part of the _git2svn filter stuff. appendSVNExecutableBitChangeToPatch We should probably write a FIXME that these modes are fragile: $$patchRef .= "Added: svn:executable\n" if $fileMode eq "100755"; 139 $$patchRef .= "Deleted: svn:executable\n" if $fileMode eq "100644"; This line needs a comment: 179 $propertyChangePath = "" if ($propertyChangePath && /^ (\+|-) \*$/); Since the if is generic, maybe this should be a generically named subroutine: 392 # Change executable bit. 393 if ($patch =~ /Added: svn:executable\n/) { 394 scmAddExecutableProperty($fullPath); 395 } elsif ($patch =~ /Deleted: svn:executable\n/) { 396 scmRemoveExecutableProperty($fullPath); 397 } applySVNPropertyChange? This needs a comment, or better yet to be turned into some sort of subroutine with a descriptive name: 146 $propertyChangePath = "" if ($propertyChangePath && /^ (\+|-) \*$/); Given that we do this check 3? 4? times, mabye this shoudl also be a subroutine: 185 unless ($patch =~ m|^Index: ([^\r\n]+)| || $patch =~ m|^Property changes on: ([^\r\n]+)|) { isStartOfPatchOrPropertyChange($line)? This code is duplicated and should be shared: # Reverse change of executable bit. 247 if ($patch =~ /Added: svn:executable\n/) { 248 scmRemoveExecutableProperty($fullPath); 249 } elsif ($patch =~ /Deleted: svn:executable\n/) { 250 scmAddExecutableProperty($fullPath); 251 } maybe togglePropertyChange($patch, isApply=False) or something? In general I think this looks fantastic! I would love to see it cleaned up a little more though.

(In reply to comment #17) > (From update of attachment 53061 [details]) > Seems we should add a FIXME in this method about making it more generic: > 125 sub isSVNProperty($) > 126 { > 127 my ($patch) = @_; > 128 return $patch =~ /\n(Added|Deleted): svn:executable\n/; > > > We need a comment here to explain why this can't just be part of the _git2svn > filter stuff. > appendSVNExecutableBitChangeToPatch > Given that we do this check 3? 4? times, mabye this shoudl also be a > subroutine: > 185 unless ($patch =~ m|^Index: ([^\r\n]+)| || $patch =~ m|^Property > changes on: ([^\r\n]+)|) { > isStartOfPatchOrPropertyChange($line)? Just a side note regarding this patch. I'm sorry I never got around to landing this-- https://bugs.webkit.org/show_bug.cgi?id=34033 That patch refactors svn-apply and svn-unapply to use a unit-tested parsePatch() method that returns a patch "object" (the method and unit tests have already been checked in): http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/VCSUtils.pm?rev=56472#L549 If that were landed, then some of the logic in this patch could go into the parseDiffHeader() method: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/VCSUtils.pm#L387 and the diff object could be adjusted to have a property representing whether the executable bit should be set or unset. I should really get around to landing that, which will require additional changes once this lands.

Created attachment 53067 [details] Patch with unit tests

Attachment 53067 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/Scripts/svn-unapply:135: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/svn-unapply:136: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/svn-unapply:187: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/svn-unapply:249: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/svn-apply:163: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/svn-apply:164: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/svn-apply:395: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:59: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:72: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:79: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:131: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:137: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:138: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:139: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:144: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:145: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:150: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:151: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:153: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:154: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:155: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:156: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:157: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:159: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:160: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:161: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:162: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:163: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils.pm:174: Line contains tab character. [whitespace/tab] [5] Total errors found: 29 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 53067 [details] Patch with unit tests Tab city batman! You're right, parseStartOfPatchOrPropertyChange is not really a win. + if (my $filePath = parseStartOfPatchOrPropertyChange($_)) { + if (/^Index: (.+)/) { + $indexPath = $filePath; + } else { + $propertyChangePath = $filePath; + } would hav ebeen cleaner as just: if (index thingy) { } else (property thingy) { } instead of the double-check inside the function. Not sure. I mean, you could have done two functions instead. if (my filePath = parseStartOfPatch()) { } else (my propertyName = prasePropertyChange()) { } Either way, I don't need to see this again. But consider the above.

Created attachment 53071 [details] Patch for landing

Created attachment 53072 [details] Patch for landing

Comment on attachment 53072 [details] Patch for landing Clearing flags on attachment: 53072 Committed r57440: <http://trac.webkit.org/changeset/57440>

All reviewed patches have been landed. Closing bug.

Rolled out change committed in change set 57440 in change set 57453 <http://trac.webkit.org/changeset/57453> as it did not handle Git patches that included both file and property changes to the same file. Rolling this change out while I look into this.

Created attachment 53149 [details] Patch with unit tests Updated patch to work correctly when patching a file that also has a property change. Refactored bookkeeping code to apply/unapply a patch and added a unit test Scripts/webkitperl/VCSUtils_unittest/processDiffAndPropertyChange.pl.

(In reply to comment #27) > Created an attachment (id=53149) [details] FYI, Dan and I discussed this report at the webkit-meeting. We decided that it would make sense to land this patch first: https://bugs.webkit.org/show_bug.cgi?id=34033 And then work together on incorporating the code in this report into the new unit-tested svn-apply code. I should have an updated patch for bug 34033 shortly (within a couple days).

Comment on attachment 53149 [details] Patch with unit tests From Chris's comment above, sounds like this should be marked r-.

*** This bug has been marked as a duplicate of bug 39409 ***