| Summary: | Use pretty patch for confirming webkit-patch diffs | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | commit-queue, dbates, eric, mjs | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Other | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Adam Barth
2010-04-12 22:21:53 PDT
Created attachment 53224 [details]
Patch
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
> 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. Created attachment 53332 [details]
Patch
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. :) Created attachment 53336 [details]
Patch
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.
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. 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. > 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. 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)
(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). 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/ Comment on attachment 53336 [details]
Patch
Ok. Let's land this like this and make the pretty-diff command leak the temp file.
Comment on attachment 53336 [details] Patch Clearing flags on attachment: 53336 Committed r57587: <http://trac.webkit.org/changeset/57587> All reviewed patches have been landed. Closing bug. |