Bug 27204

Summary: svn-apply doesn't understand the executable bit
Product: WebKit Reporter: Adam Barth <abarth>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: cjerdonek, commit-queue, dbates, ddkilzer, eric, joepeck, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 34033    
Bug Blocks: 28306    
Attachments:
Description Flags
Patch
none
Patch with unit test
none
Patch with unit tests
none
Patch with unit tests
none
Patch with unit tests
none
Patch for landing
none
Patch for landing
none
Patch with unit tests eric: review-

Description Adam Barth 2009-07-12 22:45:06 PDT
I now land all my patches via bugzilla-tool (in an SVN working copy), but when I land patches with executable files (like perl scripts), bugzilla-tool screws up and forgets the executable bit.  The makes me do a followup patch where I set the executable bit.
Comment 1 Eric Seidel (no email) 2009-07-12 23:08:09 PDT
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...
Comment 2 Adam Barth 2009-07-12 23:24:15 PDT
Mark Rowe tells me that git already understands the executable bit.  That's what the modes are telling you in git patch files.
Comment 3 Eric Seidel (no email) 2009-07-13 00:28:21 PDT
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).
Comment 4 Eric Seidel (no email) 2009-11-11 12:26:55 PST
Turns out that this is actually already listed as a "fixme" in svn-apply:

# Missing features:
#
#   Handle property changes.
Comment 5 Eric Seidel (no email) 2009-11-29 19:19:38 PST
Bit us again in http://trac.webkit.org/changeset/51475
Comment 6 Eric Seidel (no email) 2009-11-29 19:27:13 PST
Committed r51477: <http://trac.webkit.org/changeset/51477>
Comment 7 David Kilzer (:ddkilzer) 2009-12-30 06:17:01 PST
Hit again for bug 32919 in r52646.  Fixed in r52658.

<http://trac.webkit.org/changeset/52646>
<http://trac.webkit.org/changeset/52658>
Comment 8 Daniel Bates 2010-04-08 22:25:24 PDT
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 9 Eric Seidel (no email) 2010-04-08 22:31:31 PDT
Comment on attachment 52933 [details]
Patch

I don't see any "complete-rewrites" as imminent. :)  Did you mean to mark this for review?
Comment 10 Daniel Bates 2010-04-08 23:20:25 PDT
Created attachment 52937 [details]
Patch with unit test
Comment 11 Joseph Pecoraro 2010-04-08 23:50:44 PDT
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.
Comment 12 Daniel Bates 2010-04-09 00:23:58 PDT
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.
Comment 13 Daniel Bates 2010-04-09 00:25:16 PDT
Oops, I should say Joseph Pecoraro's suggestions, not Eric's.
Comment 14 Joseph Pecoraro 2010-04-09 16:43:01 PDT
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 15 Eric Seidel (no email) 2010-04-10 15:20:52 PDT
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?
Comment 16 Daniel Bates 2010-04-10 17:17:29 PDT
Created attachment 53061 [details]
Patch with unit tests

Removed extraneous function unapplySVNPropertyChange.
Comment 17 Eric Seidel (no email) 2010-04-10 17:34:28 PDT
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.
Comment 18 Chris Jerdonek 2010-04-10 18:54:37 PDT
(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.
Comment 19 Daniel Bates 2010-04-10 20:36:13 PDT
Created attachment 53067 [details]
Patch with unit tests
Comment 20 WebKit Review Bot 2010-04-10 20:41:30 PDT
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 21 Eric Seidel (no email) 2010-04-10 20:48:12 PDT
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.
Comment 22 Daniel Bates 2010-04-10 21:26:06 PDT
Created attachment 53071 [details]
Patch for landing
Comment 23 Daniel Bates 2010-04-10 21:35:32 PDT
Created attachment 53072 [details]
Patch for landing
Comment 24 WebKit Commit Bot 2010-04-10 22:46:24 PDT
Comment on attachment 53072 [details]
Patch for landing

Clearing flags on attachment: 53072

Committed r57440: <http://trac.webkit.org/changeset/57440>
Comment 25 WebKit Commit Bot 2010-04-10 22:46:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Daniel Bates 2010-04-11 12:11:27 PDT
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.
Comment 27 Daniel Bates 2010-04-11 22:32:18 PDT
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.
Comment 28 Chris Jerdonek 2010-04-15 05:02:51 PDT
(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 29 Eric Seidel (no email) 2010-04-19 01:32:29 PDT
Comment on attachment 53149 [details]
Patch with unit tests

From Chris's comment above, sounds like this should be marked r-.
Comment 30 Daniel Bates 2010-07-08 10:57:54 PDT

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