Bug 27204

Summary: svn-apply doesn't understand the executable bit
Product: WebKit Reporter: Adam Barth <abarth>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
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    
Description Flags
Patch with unit test
Patch with unit tests
Patch with unit tests
Patch with unit tests
Patch for landing
Patch for landing
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.

Comment 8 Daniel Bates 2010-04-08 22:25:24 PDT
Created attachment 52933 [details]

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]

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:

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.

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             }


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]+)|) {

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--


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):


If that were landed, then some of the logic in this patch could go into the parseDiffHeader() method:


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:


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 ***