Bug 21222 - Formatted Diff for attachment 23920 [details] is mangled
Summary: Formatted Diff for attachment 23920 is mangled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL: https://bugs.webkit.org/attachment.cg...
Keywords:
: 21291 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-09-29 15:00 PDT by Mark Rowe (bdash)
Modified: 2011-03-10 16:57 PST (History)
6 users (show)

See Also:


Attachments
patch (2.26 KB, patch)
2011-03-08 20:50 PST, Caio Marcelo de Oliveira Filho
aroben: review-
Details | Formatted Diff | Diff
patch v2, moved normalization to lower layer (2.27 KB, patch)
2011-03-09 11:04 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2008-09-29 15:00:20 PDT
The formatted diff for attachment 23920 [details] is mangled.  The raw diff is at <https://bugs.webkit.org/attachment.cgi?id=23920&action=view>, with the mangled formatting at <https://bugs.webkit.org/attachment.cgi?id=23920&action=prettypatch>.
Comment 1 Mark Rowe (bdash) 2008-10-01 18:48:10 PDT
*** Bug 21291 has been marked as a duplicate of this bug. ***
Comment 2 Caio Marcelo de Oliveira Filho 2011-03-08 20:50:30 PST
Created attachment 85129 [details]
patch
Comment 3 Eric Seidel (no email) 2011-03-08 21:14:33 PST
Comment on attachment 85129 [details]
patch

Shouldn't this be done at a lower layer?  Or shouldn't prettyify ASSERT that it has the proper line endings?
Comment 4 Eric Seidel (no email) 2011-03-08 21:14:59 PST
How do we unit test this?  (I'm not sure we have unit tests for the ruby code?)
Comment 5 Adam Roben (:aroben) 2011-03-09 05:23:04 PST
Comment on attachment 85129 [details]
patch

I agree with Eric that the line-ending normalization should happen in PrettyPatch itself. prettify.rb is just supposed to be a thin wrapper around PrettyPatch.
Comment 6 Adam Roben (:aroben) 2011-03-09 05:23:43 PST
(In reply to comment #4)
> How do we unit test this?  (I'm not sure we have unit tests for the ruby code?)

We don't have unit tests. It would be pretty easy to add some, though!
Comment 7 Caio Marcelo de Oliveira Filho 2011-03-09 11:04:50 PST
Created attachment 85197 [details]
patch v2, moved normalization to lower layer
Comment 8 Caio Marcelo de Oliveira Filho 2011-03-09 11:13:06 PST
(In reply to comment #6)
> We don't have unit tests. It would be pretty easy to add some, though!

What kind of tests are you thinking about, one that gets a set of patches to run prettify and compare with some expected files? Or something different?

Any guidance on where to put those is welcome too :)
Comment 9 Eric Seidel (no email) 2011-03-09 11:14:20 PST
(In reply to comment #8)
> (In reply to comment #6)
> > We don't have unit tests. It would be pretty easy to add some, though!
> 
> What kind of tests are you thinking about, one that gets a set of patches to run prettify and compare with some expected files? Or something different?
> 
> Any guidance on where to put those is welcome too :)

I was thinking more along the lines of the python unit tests:
*_unittest.py under http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/
Comment 10 WebKit Commit Bot 2011-03-10 16:57:03 PST
Comment on attachment 85197 [details]
patch v2, moved normalization to lower layer

Clearing flags on attachment: 85197

Committed r80795: <http://trac.webkit.org/changeset/80795>
Comment 11 WebKit Commit Bot 2011-03-10 16:57:07 PST
All reviewed patches have been landed.  Closing bug.