Bug 37406 - Add PrettyPatch links to new-run-webkit-tests output
Summary: Add PrettyPatch links to new-run-webkit-tests output
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-11 00:10 PDT by Eric Seidel (no email)
Modified: 2010-04-12 01:10 PDT (History)
4 users (show)

See Also:


Attachments
Patch (14.02 KB, patch)
2010-04-11 00:15 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-04-11 00:10:29 PDT
Add PrettyPatch links to new-run-webkit-tests output
Comment 1 Eric Seidel (no email) 2010-04-11 00:15:23 PDT
Created attachment 53079 [details]
Patch
Comment 2 Adam Barth 2010-04-11 00:23:54 PDT
Comment on attachment 53079 [details]
Patch

Ok.  I miss the pretty diffs.  Glad it's coming back.
Comment 3 WebKit Commit Bot 2010-04-11 00:42:03 PDT
Comment on attachment 53079 [details]
Patch

Clearing flags on attachment: 53079

Committed r57446: <http://trac.webkit.org/changeset/57446>
Comment 4 WebKit Commit Bot 2010-04-11 00:42:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Dirk Pranke 2010-04-11 11:35:35 PDT
Comment on attachment 53079 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index b1e0733..33893f6 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,27 @@
> +2010-04-11  Eric Seidel  <eric@webkit.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add PrettyPatch links to new-run-webkit-tests output
> +        https://bugs.webkit.org/show_bug.cgi?id=37406
> +

Does this patch introduce a dependency on Ruby into the test system? I don't think we have
Ruby on Chromium windows (and am not sure about Linux, although it's probably there).

Also, do we really need three different kinds of diffs?

> @@ -519,6 +519,7 @@ class Port(object):
>                 '--end-insert=##WDIFF_END##',
>                 actual_filename,
>                 expected_filename]
> +        # FIXME: Why not just check os.exists(executable) once?
>          global _wdiff_available

See the comments in base.py's Port.wdiff_text()
Comment 6 Eric Seidel (no email) 2010-04-11 11:37:46 PDT
I need to add a check for Ruby, as it looks like the Linux canary doesn't have ruby.

Personally I find PrettyPatch diffs prettier than wdiff. :)  wdiff doesn't exist on Mac systems, and more importantly is the same prettydiff that we use on bugs.webkti.org (the "Formatted Diff" link next to any patch is a PrettyPatch diff).
Comment 7 Dirk Pranke 2010-04-11 11:43:14 PDT
(In reply to comment #6)
> I need to add a check for Ruby, as it looks like the Linux canary doesn't have
> ruby.
> 
> Personally I find PrettyPatch diffs prettier than wdiff. :)  wdiff doesn't
> exist on Mac systems, and more importantly is the same prettydiff that we use
> on bugs.webkti.org (the "Formatted Diff" link next to any patch is a
> PrettyPatch diff).

Sorry, I should have been clearer; I'm not wedded to wdiff either, except that it shows you intraline diffs, and I don't know that anyone else is, either. Perhaps we should replace wdiff with PrettyPatch, instead of having three diffs?
Comment 8 Adam Barth 2010-04-11 11:53:59 PDT
> Perhaps we should replace wdiff with PrettyPatch, instead of having three
> diffs?

That sounds like a good plan to me.  That's consistent with how the tool has worked historically and with how bugs.webkit.org works.
Comment 9 Eric Seidel (no email) 2010-04-11 18:47:48 PDT
I wont be able to fix the script for pllatforms missing Ruby until tomorrow morning. If that's blocking the Gardner from getting work done, feel free to roll this out.
Comment 10 Eric Seidel (no email) 2010-04-12 01:10:09 PDT
Fixed systems missing ruby in bug 37441.