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

[bugzilla-daemon at webkit.org]

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
patches
https://bugs.webkit.org/show_bug.cgi?id=67628

Attachment 106845: Patch
https://bugs.webkit.org/attachment.cgi?id=106845&action=review

------- 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
patch
> +	   we may download the previous revision from trac to get the image
data.

"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
GIT_DELTA_FORMAT.match(encoded_chunk[0])

That might be clearer.