Bug 32582 – svn-create-patch should print a warning for large patches

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

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

[Zoltan Horvath]

Created attachment 50895 [details]
Patch

[Darin Adler]

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

[Zoltan Horvath]

Created attachment 50913 [details]
Patch

[Zoltan Horvath]

Yes, it sounds much friendly. I filed the new patch.

[Daniel Bates]

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

[Eric Seidel (no email)]

Comment on attachment 50913 [details]
Patch

Recommending people break up their patches is good.  But this looks OK.

[Zoltan Horvath]

Comment on attachment 50913 [details]
Patch

Clearing flags on attachment: 50913

Committed r56524: <http://trac.webkit.org/changeset/56524>

[Zoltan Horvath]

All reviewed patches have been landed.  Closing bug.

[Zoltan Horvath]

Created attachment 53008 [details]
fix 2 issues patch

Fix 2 issues what was introduced by the change.

[Darin Adler]

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?

[Alexey Proskuryakov]

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.

[Zoltan Horvath]

Created attachment 53012 [details]
fix 2 issues patch vol2.

[Alexey Proskuryakov]

Comment on attachment 53012 [details]
fix 2 issues patch vol2.

r=me

[Zoltan Horvath]

Comment on attachment 53012 [details]
fix 2 issues patch vol2.

Clearing flags on attachment: 53012

Committed r57380: <http://trac.webkit.org/changeset/57380>

[Zoltan Horvath]

All reviewed patches have been landed.  Closing bug.