Bug 53569 – include svn revisions in git diffs for the code review tool to use

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

include svn revisions in git diffs for the code review tool to use

[Ojan Vafai]

Created attachment 80879 [details]
Patch

[Adam Barth]

Comment on attachment 80879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80879&action=review

The webkitpy parts look great.

> Tools/Scripts/webkitpy/common/checkout/scm.py:751
> +        num_tries = 0

nit: We prefer to use full words in variable names.

[Adam Barth]

So, unrelated to this bug, but I'm going to spam it here anyway.  When you load up a review, the page jumps down one line after a moment because it's filling in the "j/k" help message.  Can we put that on the same line as the basic help message (and maybe gray it out also)?

[Ojan Vafai]

(In reply to comment #3)
> So, unrelated to this bug, but I'm going to spam it here anyway.  When you load up a review, the page jumps down one line after a moment because it's filling in the "j/k" help message.  Can we put that on the same line as the basic help message (and maybe gray it out also)?

lol. That was your doing! Happy to fix that though.

[Ojan Vafai]

aroben, you mind looking over the ruby side of this patch?

[Adam Barth]

> lol. That was your doing! Happy to fix that though.

Yeah, well...  Um...  Thanks.  :)

[Adam Roben (:aroben)]

Comment on attachment 80879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80879&action=review

> Tools/Scripts/webkitpy/common/checkout/scm.py:758
> +        while not revision and num_tries < 10:
> +            # If the git checkout is not tracking an SVN repo, then svn_revision_from_git_commit throws.
> +            try:
> +                revision = self.svn_revision_from_git_commit('HEAD~' + str(num_tries))
> +            except:
> +                return diff
> +            num_tries += 1

Would using "git svn info" be faster than this loop?

> Tools/Scripts/webkitpy/common/checkout/scm.py:763
> +        return "SVN Revision: " + str(revision) + '\n' + diff

Would "Subversion" be better than "SVN"?

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:22
> +            match = /^SVN\ Revision: (.*)$/.match(line)

Would it be better to use \d instead of .?

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:23
> +            if (not match.nil?)

I don't think you need the parentheses here. You can also just use "unless" instead of "if not".

[Ojan Vafai]

Comment on attachment 80879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80879&action=review

Thanks for the review.

>> Tools/Scripts/webkitpy/common/checkout/scm.py:758
>> +            num_tries += 1
> 
> Would using "git svn info" be faster than this loop?

It certainly would be faster. But that doesn't necessarily tell us which svn revision this branch is merged to, right?

>> Tools/Scripts/webkitpy/common/checkout/scm.py:763

> 
> Would "Subversion" be better than "SVN"?

Done

>> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:22
>> +            match = /^SVN\ Revision: (.*)$/.match(line)
> 
> Would it be better to use \d instead of .?

yes! :)

>> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:23
>> +            if (not match.nil?)
> 
> I don't think you need the parentheses here. You can also just use "unless" instead of "if not".

crazy language. thx.

[Ojan Vafai]

Committed r77889: <http://trac.webkit.org/changeset/77889>