Bug 38552 – PrettyPatch.pretty_diff("") should not hang

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Disable pretty patch on Chromium Mac to see if that resolves some hangs seen on the Mac Canary

[Eric Seidel (no email)]

Created attachment 55066 [details]
Patch

[Dirk Pranke]

Comment on attachment 55066 [details]
Patch

Can we just make the Mac port set _pretty_patch_available to false instead of returning a dummy text string?

[Eric Seidel (no email)]

That will cause it to return a different dummy text string, but that's totally doable too.

[Eric Seidel (no email)]

Dirk the change you're suggesting would look like this:

chromium_mac.py:

import base

__init__...
   # FIXME: Remove this hack.
   _log.warn("Disabling pretty patch support due to bug 38552");
   base._pretty_patch_available = False


I'm happy to make such.  But I think it's kinda a wash.  They're both pretty hackish.

[Dirk Pranke]

Ah, I see. It would be nice if there was an overridable way of determining if pretty patch was available, but there isn't. Also, I see that what you return from pretty_diff_text() - an error string -- is different than what we return from wdiff_text()  -- an empty string -- if it isn't available.

So your first change is fine.

[Eric Seidel (no email)]

Just going to fix the pretty_diff hang for now.

[Eric Seidel (no email)]

Created attachment 55085 [details]
Patch

[Dirk Pranke]

Comment on attachment 55085 [details]
Patch

LGTM (although I'm not a reviewer).

[Shinichiro Hamaji]

Comment on attachment 55085 [details]
Patch

Looks like a nice fix.

[WebKit Commit Bot]

Comment on attachment 55085 [details]
Patch

Clearing flags on attachment: 55085

Committed r58803: <http://trac.webkit.org/changeset/58803>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.