Bug 53569 - include svn revisions in git diffs for the code review tool to use
Summary: include svn revisions in git diffs for the code review tool to use
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-01 20:28 PST by Ojan Vafai
Modified: 2011-02-07 20:30 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.21 KB, patch)
2011-02-01 20:28 PST, Ojan Vafai
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-02-01 20:28:06 PST
include svn revisions in git diffs for the code review tool to use
Comment 1 Ojan Vafai 2011-02-01 20:28:51 PST
Created attachment 80879 [details]
Patch
Comment 2 Adam Barth 2011-02-01 20:36:17 PST
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.
Comment 3 Adam Barth 2011-02-01 20:37:10 PST
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)?
Comment 4 Ojan Vafai 2011-02-01 20:39:18 PST
(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.
Comment 5 Ojan Vafai 2011-02-01 20:39:40 PST
aroben, you mind looking over the ruby side of this patch?
Comment 6 Adam Barth 2011-02-01 20:51:12 PST
> lol. That was your doing! Happy to fix that though.

Yeah, well...  Um...  Thanks.  :)
Comment 7 Adam Roben (:aroben) 2011-02-02 05:21:02 PST
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".
Comment 8 Ojan Vafai 2011-02-07 20:26:44 PST
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.
Comment 9 Ojan Vafai 2011-02-07 20:30:13 PST
Committed r77889: <http://trac.webkit.org/changeset/77889>