RESOLVED FIXED 31395
Bugzilla should show images in git patches
https://bugs.webkit.org/show_bug.cgi?id=31395
Summary Bugzilla should show images in git patches
Shinichiro Hamaji
Reported 2009-11-12 00:50:08 PST
It would be difficult to review expected images without this feature.
Attachments
Patch v0 (7.12 KB, patch)
2009-11-12 00:51 PST, Shinichiro Hamaji
ddkilzer: review-
Example patch (64.36 KB, patch)
2009-11-12 00:53 PST, Shinichiro Hamaji
no flags
Example output (114.51 KB, text/html)
2009-11-12 00:54 PST, Shinichiro Hamaji
no flags
Patch v1 (6.20 KB, patch)
2009-12-04 07:43 PST, Shinichiro Hamaji
no flags
Patch v2 (6.22 KB, patch)
2009-12-07 00:15 PST, Shinichiro Hamaji
no flags
Shinichiro Hamaji
Comment 1 2009-11-12 00:51:35 PST
Created attachment 43038 [details] Patch v0
Shinichiro Hamaji
Comment 2 2009-11-12 00:53:55 PST
Created attachment 43039 [details] Example patch
Shinichiro Hamaji
Comment 3 2009-11-12 00:54:53 PST
Created attachment 43040 [details] Example output
Shinichiro Hamaji
Comment 4 2009-11-12 01:01:31 PST
My patch is v0 because I don't think this will work as is. PrettyPatch.rb executes a perl script which is stored in WebKitTools/Scripts. I don't know how we can run scripts in WebKitTools from BugsSite. Could someone tell me how this can be done? I think we need to fix the following line in my patch: GIT_DECODE_PATCH_PATH = "WebKitTools/Scripts/decode-git-binary-patch"
Eric Seidel (no email)
Comment 5 2009-11-12 07:23:21 PST
CCing folks who know something about bugs.webkit.org
Adam Roben (:aroben)
Comment 6 2009-11-12 07:53:10 PST
(In reply to comment #4) > My patch is v0 because I don't think this will work as is. PrettyPatch.rb > executes a perl script which is stored in WebKitTools/Scripts. I don't know how > we can run scripts in WebKitTools from BugsSite. Could someone tell me how this > can be done? I think we need to fix the following line in my patch: > > GIT_DECODE_PATCH_PATH = "WebKitTools/Scripts/decode-git-binary-patch" You're right that this won't work on bugs.webkit.org. On the server, we only have access to the BugsSite directory. Maybe we could change the server to also have a checkout of WebKitTools/Scripts in a known location. I'll defer to Mark and Bill on that one.
Shinichiro Hamaji
Comment 7 2009-11-12 20:40:52 PST
Thanks Eric for adding CC and thanks Adam for your comment. > You're right that this won't work on bugs.webkit.org. On the server, we only > have access to the BugsSite directory. > > Maybe we could change the server to also have a checkout of WebKitTools/Scripts > in a known location. I'll defer to Mark and Bill on that one. I see. Checking out WebKitTools/Scripts would be nice. With this change, we may be able to run check-webkit-style in bugzilla, too. It will help people to notice trivial style issues without waiting reviewers' comments.
Shinichiro Hamaji
Comment 8 2009-11-18 21:49:43 PST
Ping? I (and maybe most of git users) really want to use git even for patches with images. Maybe I should talk in IRC, but it's not easy for me to be there due to time difference.
Eric Seidel (no email)
Comment 9 2009-11-26 21:26:06 PST
You can try emailing Mark or Bill, but those are the decision makers here if you require server-side changes.
Adam Barth
Comment 10 2009-11-30 12:24:06 PST
Attachment 43038 [details] passed the style-queue
David Kilzer (:ddkilzer)
Comment 11 2009-12-02 11:44:36 PST
Comment on attachment 43038 [details] Patch v0 Rather than writing a new decode-git-binary-patch script which uses VCSUtils::decodeGitBinaryPatch(), I think the PrettyPatch.rb script should instead use a git command to do its decoding work directly. We can guarantee that git will be installed on bugs.webkit.org, so we don't have to rely on a Perl implementation of the git decoding algorithm to get the image data. > diff --git a/BugsSite/PrettyPatch/PrettyPatch.rb b/BugsSite/PrettyPatch/PrettyPatch.rb > lines_with_contents = lines[startOfSections...lines.length] > @sections = DiffSection.parse(lines_with_contents) unless @binary > - @image_url = "data:image/png;base64," + lines_with_contents.join if @image > + if @image > + @image_url = "data:image/png;base64," + lines_with_contents.join > + elsif @git_image > + begin > + stdin, stdout, stderr = *Open3.popen3(GIT_DECODE_PATCH_PATH) > + stdin.puts(lines) > + stdin.close > + @image_urls = stdout.readlines > + stdout.close > + error = stderr.read > + stderr.close > + if error != '' > + @image_error = "Failed to decode git binary patch<pre>#{CGI.escapeHTML(error)}</pre>" > + end > + rescue > + @image_error = "Exception raised during decoding git binary patch: #{$!}" The error message, $!, needs to be HTML-escaped as well. r- to fix the HTML-escaping issue noted and to use a git command directly to decode the binary patch.
Shinichiro Hamaji
Comment 12 2009-12-04 07:43:01 PST
Created attachment 44312 [details] Patch v1
WebKit Review Bot
Comment 13 2009-12-04 07:44:33 PST
style-queue ran check-webkit-style on attachment 44312 [details] without any errors.
Shinichiro Hamaji
Comment 14 2009-12-04 07:52:11 PST
I updated my patch. The new patch uses git as suggested by David (thanks!). I couldn't find a git command to extract a file from git binary patch, so my patch does: 1. Use Tempfile to create a unique name. 2. Append ".bin" to the filename so that the file doesn't exist. 3. Create a git patch which creates a new file using the filename we've got. 4. Use git-apply to extract the image file. 5. Read the contents of the image created. 6. Remove the file we've created. So, this patch depends /tmp is writable. Can PrettyPrint.rb create a file in /tmp? If not, I will need to figure out another way...
William Siegrist
Comment 15 2009-12-04 07:56:57 PST
Yes, /tmp is writable.
Shinichiro Hamaji
Comment 16 2009-12-04 08:13:11 PST
(In reply to comment #15) > Yes, /tmp is writable. Nice. Thanks for your reply. Another concern is the version of Ruby. I checked my script is working with ruby-1.8.6 on Leopard and ruby-1.8.7 on Linux.
William Siegrist
Comment 17 2009-12-04 08:18:14 PST
PrettyPatch uses the stock ruby (Leopard, 1.8.6).
David Kilzer (:ddkilzer)
Comment 18 2009-12-04 17:41:37 PST
Comment on attachment 44312 [details] Patch v1 > + def self.extract_contents_from_git_binary_chunk(encoded_chunk, git_index) > + # We use Tempfile we need a unique file among processes. > + tempfile = Tempfile.new("PrettyPatch") > + # We need a filename which doesn't exist to apply a patch > + # which creates a new file. Append a suffix so filename > + # doesn't exist. > + filename = File.basename(tempfile.path) + '.bin' Prepending ".bin" to the unique filename may (potentially) lead to a symlink attack because there is a short time between the patch file being written and the patch being applied. It would probably be best to generate the "bin" filename as well. > + patch = FileDiff.git_new_file_binary_patch(filename, encoded_chunk, git_index) > + > + # Apply the git binary patch using git-apply. > + Dir.chdir(File.dirname(tempfile.path)) do > + stdin, stdout, stderr = *Open3.popen3(GIT_PATH + " apply") > + begin > + stdin.puts(patch) > + stdin.close > + > + error = stderr.read > + raise error if error != "" > + > + contents = File.read(filename) > + ensure > + stdin.close unless stdin.closed? > + stdout.close > + stderr.close > + File.unlink(filename) if File.exists?(filename) > + end > + > + return nil if contents.empty? > + return "data:image/png;base64," + [contents].pack("m") > + end > + end > end I don't see tempfile being unlinked anywhere, or is that implicit when its object is deleted? If not, it needs to be removed as well. r=me if you fix the above two issues.
Shinichiro Hamaji
Comment 19 2009-12-04 22:18:51 PST
Thanks for your review. I understand your concern, but I think they are not real issues. I'd like to go as is if there are no other concerns. David, can you check my comments below? If there are something unclear, please ask again. > Prepending ".bin" to the unique filename may (potentially) lead to a symlink > attack because there is a short time between the patch file being written and > the patch being applied. It would probably be best to generate the "bin" > filename as well. If we create "bin" filename, we need to calculate git index, which is probably sha1sum and depends on the filename, to create the patch. We don't need to calculate the git index if the original file is not existed. We can just use 0000000000000000000000000000000000000000 as the git index of the original file. That's why my patch doesn't just use tempfile but append ".bin" to the filename. Anyway, it seems git-apply doesn't modify the file if there is already a file or a symlink and the attempt just fails. So, I think there are no security risk. > I don't see tempfile being unlinked anywhere, or is that implicit when its > object is deleted? If not, it needs to be removed as well. Yeah, the tempfile will be removed when the object is deleted. Thanks!
David Kilzer (:ddkilzer)
Comment 20 2009-12-05 03:25:07 PST
(In reply to comment #19) > Thanks for your review. I understand your concern, but I think they are not > real issues. I'd like to go as is if there are no other concerns. David, can > you check my comments below? If there are something unclear, please ask again. Okay, sounds good. r=me
Shinichiro Hamaji
Comment 21 2009-12-06 22:42:19 PST
Comment on attachment 44312 [details] Patch v1 Clearing flags on attachment: 44312 Committed r51748: <http://trac.webkit.org/changeset/51748>
Shinichiro Hamaji
Comment 22 2009-12-06 22:42:29 PST
All reviewed patches have been landed. Closing bug.
Shinichiro Hamaji
Comment 23 2009-12-06 23:02:01 PST
Hmm... my patch raises an exception. As I have no fix for now, I'll revert my change. The exception was: Exception raised during decoding git binary patch: /System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/open3.rb:67: warning: Insecure world writable dir /private/tmp/ in PATH, mode 041777
Shinichiro Hamaji
Comment 24 2009-12-07 00:15:00 PST
Created attachment 44390 [details] Patch v2
WebKit Review Bot
Comment 25 2009-12-07 00:18:19 PST
style-queue ran check-webkit-style on attachment 44390 [details] without any errors.
Shinichiro Hamaji
Comment 26 2009-12-07 00:20:30 PST
I think This issue happened because Bugzilla.pm sets PATH="". When PATH environment value is empty, the current directory is considered in PATH. As the Patch v1 changes directory to /tmp, Ruby produced the warning when it runs an external command. As there is a command line flag --directory in git-apply, using this flag would be the best solution.
David Kilzer (:ddkilzer)
Comment 27 2009-12-07 10:50:57 PST
Comment on attachment 44390 [details] Patch v2 r=me
Shinichiro Hamaji
Comment 28 2009-12-07 18:50:48 PST
Comment on attachment 44390 [details] Patch v2 Clearing flags on attachment: 44390 Committed r51826: <http://trac.webkit.org/changeset/51826>
Shinichiro Hamaji
Comment 29 2009-12-07 18:51:01 PST
All reviewed patches have been landed. Closing bug.
Shinichiro Hamaji
Comment 30 2009-12-07 18:56:32 PST
Thanks for you review. OK, now it seems working! https://bugs.webkit.org/attachment.cgi?id=43039&action=prettypatch Thanks again folks for several suggestions.
Eric Seidel (no email)
Comment 31 2009-12-08 10:24:48 PST
The pretty-patch link you reference has this in it: LayoutTests/platform/mac/fast/css/corrupted.png Exception raised during decoding git binary patch: error: corrupt binary patch at line 15: junk_line ./PrettyPatch/PrettyPatch.rb:343:in `extract_contents_from_git_binary_chunk' ./PrettyPatch/PrettyPatch.rb:258:in `initialize' ./PrettyPatch/PrettyPatch.rb:257:in `collect' ./PrettyPatch/PrettyPatch.rb:257:in `initialize' ./PrettyPatch/PrettyPatch.rb:309:in `new' ./PrettyPatch/PrettyPatch.rb:309:in `parse' ./PrettyPatch/PrettyPatch.rb:309:in `collect' ./PrettyPatch/PrettyPatch.rb:309:in `parse' ./PrettyPatch/PrettyPatch.rb:15:in `prettify' PrettyPatch/prettify.rb:26
David Kilzer (:ddkilzer)
Comment 32 2009-12-08 10:29:34 PST
(In reply to comment #31) > The pretty-patch link you reference has this in it: > > LayoutTests/platform/mac/fast/css/corrupted.png > > Exception raised during decoding git binary patch: > error: corrupt binary patch at line 15: junk_line I'm pretty sure that was done on purpose for error handling.
Shinichiro Hamaji
Comment 33 2009-12-08 19:22:33 PST
> I'm pretty sure that was done on purpose for error handling. Yeah, I intentionally inserted this junk_line to see how corrupt patch is handled.
Note You need to log in before you can comment on or make changes to this bug.