Bug 37489 – Use pretty patch for confirming webkit-patch diffs

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Use pretty patch for confirming webkit-patch diffs

[Adam Barth]

Created attachment 53224 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 53224 [details]
Patch

+        except OSError, e:
+            # No pretty diff.  :(
+            self._tool.user.page(diff)

Do we need to show an error message?
Do we need to catch ScriptError as well?  Should we be logging inside that function and returning a bool instead?

Why + instead of %?

+        url = "file://" + urllib.quote(self._pretty_diff_file.name)

Seems kinda hacky:
+        if url.startswith("file://"):
+            log("MOCK: user.open_url: file://...")
+            return

Why not just change what the tmp path is to make it consistent?

Sometimes I really had 80c:
+        args = [
+            "ruby",
+            "-I",
+            pretty_patch_path,
+            prettify_path,
+        ]

Seems we may want to explicitly close/delete the temporary file:
+    def pretty_diff_file(self, diff):
+        pretty_diff = self.pretty_diff(diff)
+        diff_file = tempfile.NamedTemporaryFile(suffix=".html")
+        diff_file.write(pretty_diff)
+        diff_file.flush()
+        return diff_file

[Adam Barth]

> Do we need to show an error message?

Fixed.

> Do we need to catch ScriptError as well?

When would a ScriptError be thrown?  It seems like that would only be in case of a bug in webkit-patch or PrettyPatch.

> Should we be logging inside that function and returning a bool instead?

Done.  We actually need to return the file because we need to keep it alive until the user is done viewing the diff.

> Why + instead of %?

Fixed.

> Seems kinda hacky:
> +        if url.startswith("file://"):
> +            log("MOCK: user.open_url: file://...")
> +            return
> 
> Why not just change what the tmp path is to make it consistent?

I couldn't find a clean way of doing this.  The problem is ConfirmDiff uses PrettyPatch statically, and PrettyPatch uses tempfile statically.

> Sometimes I really had 80c:

+dpranke :)

> Seems we may want to explicitly close/delete the temporary file:

I've added an explicit close.  Python should do the deleting for us.  In fact, we have to work around the fact that Python is super eager to delete the file.

[Adam Barth]

Created attachment 53332 [details]
Patch

[Eric Seidel (no email)]

ScriptError would be thrown in the case that PrettyPatch/ruby returns non-zero.  Which might happen for a different ruby version or on windows, or who knows what.  Somehow various Chromium folks and bots seem to be triggering it. :)

[Adam Barth]

Created attachment 53336 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 53336 [details]
Patch

I think a more natural API would be to close the file in the patch-prep function and return just a path.  The problem is we'd have to move away from a temp function which deletes automatically on close (which I think woudl be a good thing).

Is:
args = ["ruby","-I", pretty_patch_path, prettify_path]
not PEP8 compliant?  Personally I find such a short command easier to read on one line.

Please consider making pretty_diff_file return a path instead of a file, I think that woudl be much cleaner (and better supports future usecases of having a "webkit-patch pretty-diff" command!  The current tmp-hack isn't very useful.

[Adam Barth]

If we represented the file as a path, how would we delete it securely?  You need to keep temp files as handles to avoid someone else on the system making the path point to something nasty that you don't want to unlink.

[Eric Seidel (no email)]

How do we end up leaving the temp file open after termination then?  We need to for commands like:

webkit-patch pretty-diff

which would just open your current diff as a diff in a web browser and exit.

[Adam Barth]

> Is:
> args = ["ruby","-I", pretty_patch_path, prettify_path]
> not PEP8 compliant?  Personally I find such a short command easier to read on
> one line.

It's probably fine.  In other cases when we've made an args array, we've ended
up tweaking it over time and eventually moved to the multiline version.  I can
change to back to single line if you like.

> How do we end up leaving the temp file open after termination then?  We need to
> for commands like:
> 
> webkit-patch pretty-diff
> 
> which would just open your current diff as a diff in a web browser and exit.

I'm inclined to worry about that when we create that command.

[Eric Seidel (no email)]

Worrying about it later is OK.  I'm just ready to write the command as soon as you and I agree how. :)  Or You can write it.  Either way, it should be 10 lines of python once we figure out how to keep the temp file around.

class PrettyDiff(AbstractDeclarativeCommand):
    name = "pretty-diff"
    help_text = "Open an HTML version of your current diff in a web browser."

    def execute(self, options, args, tool):
        diff = tool.scm().create_patch()
        pretty_diff = PrettyPatch().pretty_diff(diff)
        # Stuff it in a file somehow?
        User.open_url("file:///%s" % path_to_diff)

[Adam Barth]

(In reply to comment #11)
> Worrying about it later is OK.  I'm just ready to write the command as soon as
> you and I agree how. :)  Or You can write it.  Either way, it should be 10
> lines of python once we figure out how to keep the temp file around.
> 
> class PrettyDiff(AbstractDeclarativeCommand):
>     name = "pretty-diff"
>     help_text = "Open an HTML version of your current diff in a web browser."
> 
>     def execute(self, options, args, tool):
>         diff = tool.scm().create_patch()
>         pretty_diff = PrettyPatch().pretty_diff(diff)
>         # Stuff it in a file somehow?
>         User.open_url("file:///%s" % path_to_diff)

Right, the question is whether we want to leak the temp file.  IMHO, we might want to leak the temp file when you invoke pretty-diff directly but clean up the file when you run upload/post.  Another option is to use something like run-safari, which seems to know how to block (because that's how the LayoutTests work).  Another option is to re-use the mechanism for storing / cleaning up layout test results (which I'm not sure what it is).

[Eric Seidel (no email)]

I guess I don't worry about leaking the temp file.  Yes, we could use run-safari which blocks if you're worried about the leak.

Either way is fine with me.

The layout-test-results don't clean themselves up.  They just store in a known location in /tmp/

[Adam Barth]

Comment on attachment 53336 [details]
Patch

Ok.  Let's land this like this and make the pretty-diff command leak the temp file.

[WebKit Commit Bot]

Comment on attachment 53336 [details]
Patch

Clearing flags on attachment: 53336

Committed r57587: <http://trac.webkit.org/changeset/57587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.