Bug 32582

Summary: svn-create-patch should print a warning for large patches
Product: WebKit Reporter: Chris Jerdonek <cjerdonek>
Component: Tools / TestsAssignee: Zoltan Horvath <zoltan>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dbates, eric, zoltan
Priority: P4    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
fix 2 issues patch
ap: review-
fix 2 issues patch vol2. none

Description Chris Jerdonek 2009-12-15 14:33:00 PST
The svn-create-patch script should display a warning for large patches (e.g. > 20K).

This idea is inspired by the following suggestion to modify bugs.webkit.org:

https://bugs.webkit.org/show_bug.cgi?id=25877
Comment 1 Zoltan Horvath 2010-03-17 04:55:27 PDT
Created attachment 50895 [details]
Patch
Comment 2 Darin Adler 2010-03-17 08:42:04 PDT
Comment on attachment 50895 [details]
Patch

> Larger patches than 20k are unlikely to be reviewed!

I've reviewed hundreds of patches that are >20k and had hundreds of >20k patches reviewed, so the phrase above is overstating the case.

Maybe we can word a more positive way and be precise: "Patches 20k or smaller are more likely to be reviewed. Larger patches may sit unreviewed for a long time."
Comment 3 Zoltan Horvath 2010-03-17 09:21:57 PDT
Created attachment 50913 [details]
Patch
Comment 4 Zoltan Horvath 2010-03-17 09:23:41 PDT
Yes, it sounds much friendly. I filed the new patch.
Comment 5 Daniel Bates 2010-03-24 14:27:02 PDT
For your consideration, maybe the message should explicitly suggest that the person split up their patch into smaller sized patches, such as:

"Note, this patch is larger than 20 KB. Consider separating this patch into smaller patches that are more manageable to review."
Comment 6 Eric Seidel (no email) 2010-03-25 00:30:03 PDT
Comment on attachment 50913 [details]
Patch

Recommending people break up their patches is good.  But this looks OK.
Comment 7 Zoltan Horvath 2010-03-25 02:47:06 PDT
Comment on attachment 50913 [details]
Patch

Clearing flags on attachment: 50913

Committed r56524: <http://trac.webkit.org/changeset/56524>
Comment 8 Zoltan Horvath 2010-03-25 02:47:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Zoltan Horvath 2010-04-09 15:55:51 PDT
Created attachment 53008 [details]
fix 2 issues patch

Fix 2 issues what was introduced by the change.
Comment 10 Darin Adler 2010-04-09 15:57:14 PDT
Comment on attachment 53008 [details]
fix 2 issues patch

> Index: WebKitTools/Scripts/svn-create-patch
> ===================================================================
> --- WebKitTools/Scripts/svn-create-patch	(revision 57374)
> +++ WebKitTools/Scripts/svn-create-patch	(working copy)
> @@ -218,10 +218,10 @@
>      my $file = File::Spec->catdir($prefix, $fileData->{path});
>      
>      if ($ignoreChangelogs && basename($file) eq "ChangeLog") {
> -        return;
> +        return 0;
>      }
>      
> -    my $patch;
> +    my $patch = "";
>      if ($fileData->{modificationType} eq "additionWithHistory") {
>          manufacturePatchForAdditionWithHistory($fileData);
>      }
> @@ -233,7 +233,7 @@
>      }
>      close DIFF;
>      $patch = fixChangeLogPatch($patch) if basename($file) eq "ChangeLog";
> -    print $patch if $patch;
> +    

This used to say "print $patch" and now does nothing!

Can that really be right?
Comment 11 Alexey Proskuryakov 2010-04-09 16:03:45 PDT
Comment on attachment 53008 [details]
fix 2 issues patch

> -    print $patch if $patch;
> +    

I don't see how this can be correct - some code needs to print the patch.
Comment 12 Zoltan Horvath 2010-04-09 16:09:59 PDT
Created attachment 53012 [details]
fix 2 issues patch vol2.
Comment 13 Alexey Proskuryakov 2010-04-09 16:15:56 PDT
Comment on attachment 53012 [details]
fix 2 issues patch vol2.

r=me
Comment 14 Zoltan Horvath 2010-04-09 16:19:07 PDT
Comment on attachment 53012 [details]
fix 2 issues patch vol2.

Clearing flags on attachment: 53012

Committed r57380: <http://trac.webkit.org/changeset/57380>
Comment 15 Zoltan Horvath 2010-04-09 16:19:17 PDT
All reviewed patches have been landed.  Closing bug.