[webkit-reviews] review granted: [Bug 67628] PrettyPatch should handle delta patch mechanism in git binary patches : [Attachment 106845] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 9 06:01:55 PDT 2011

Adam Roben (:aroben) <aroben at apple.com> has granted Ben Wells
<benwells at chromium.org>'s request for review:
Bug 67628: PrettyPatch should handle delta patch mechanism in git binary

Attachment 106845: Patch

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106845&action=review

It would be nice if we could avoid touching the disk. Is there a way to run
git-apply on stdin?

> Websites/bugs.webkit.org/ChangeLog:14
> +	   When reconstructing the images from the patches, if we have a delta
> +	   we may download the previous revision from trac to get the image

"from trac" is no longer accurate.

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:23
> +	   $git_svn_revision = 0

This is just a Subversion revision number. It has nothing to do with Git (even
though it's only present for git patches). I'd just call it $svn_revision.

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:672
> +		   to_file << open(from_path) { |from_file| from_file.read}

Missing a space before }

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:678
> +	       svn_uri =  get_svn_uri(repository_path)

Extra space after =

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:719
> +	       # For literal encoded, simply reconstruct. For delta encoded,
download from svn

Missing a period at the end of this comment.

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:723
> +	       if (GIT_LITERAL_FORMAT.match(encoded_chunk[0])) then
> +		   return
extract_contents_from_git_binary_literal_chunk(encoded_chunk, git_index)
> +	       elsif (GIT_DELTA_FORMAT.match(encoded_chunk[0])) then
> +		   return download_from_revision_from_svn(repository_path)

Ruby, like Python, typically omits parentheses around conditionals like these.

WebKit coding style says not to say "else" after "return". So these could both
just be "if"s. No need for "elsif".

Like in Perl, you can use syntax like this:

return extract_contents_from_git_binary_literal_chunk(encoded_chunk, git_index)
if GIT_LITERAL_FORMAT.match(encoded_chunk[0])
return download_from_revision_from_svn(repository_path) if

That might be clearer.

More information about the webkit-reviews mailing list