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
Created attachment 50895 [details] Patch
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."
Created attachment 50913 [details] Patch
Yes, it sounds much friendly. I filed the new patch.
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 on attachment 50913 [details] Patch Recommending people break up their patches is good. But this looks OK.
Comment on attachment 50913 [details] Patch Clearing flags on attachment: 50913 Committed r56524: <http://trac.webkit.org/changeset/56524>
All reviewed patches have been landed. Closing bug.
Created attachment 53008 [details] fix 2 issues patch Fix 2 issues what was introduced by the change.
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 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.
Created attachment 53012 [details] fix 2 issues patch vol2.
Comment on attachment 53012 [details] fix 2 issues patch vol2. r=me
Comment on attachment 53012 [details] fix 2 issues patch vol2. Clearing flags on attachment: 53012 Committed r57380: <http://trac.webkit.org/changeset/57380>