Bug 111668 - webkitpy: change git_commit_from_svn_revision() to work in a pure git checkout
Summary: webkitpy: change git_commit_from_svn_revision() to work in a pure git checkout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-06 19:27 PST by Dirk Pranke
Modified: 2013-11-07 01:19 PST (History)
5 users (show)

See Also:


Attachments
Proposed patch (1.78 KB, patch)
2013-11-04 07:48 PST, Peter Molnar
no flags Details | Formatted Diff | Diff
Patch updated (1.78 KB, patch)
2013-11-04 12:48 PST, Peter Molnar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2013-03-06 19:27:59 PST
right now we call git svn find-rev, which won't work.
Comment 1 Dirk Pranke 2013-03-07 14:29:35 PST
See the motivating bug 110839 for this ... we should just change it to call _most_recent_log_matching .
Comment 2 Ryosuke Niwa 2013-04-08 21:35:32 PDT
Why is this won't fix? I don't think so.
Comment 3 Tim 'mithro' Ansell 2013-04-08 22:04:23 PDT
I closed it WONTFIX because I won't be fixing it as I've been asked to concentrated on Blink.
Comment 4 Ryosuke Niwa 2013-04-08 22:05:48 PDT
(In reply to comment #3)
> I closed it WONTFIX because I won't be fixing it as I've been asked to concentrated on Blink.

That's not a good reason to close this bug as WONTFIX since this bug is not specific to Chromium at all.
Comment 5 Dirk Pranke 2013-04-08 22:38:34 PDT
(In reply to comment #3)
> I closed it WONTFIX because I won't be fixing it as I've been asked to concentrated on Blink.

In this case you should just un-assign yourself from the bug (use the Reset Assignee to default checkbox), rather than closing the bugs as WontFix.
Comment 6 Peter Molnar 2013-11-04 07:48:03 PST
Created attachment 215920 [details]
Proposed patch
Comment 7 Ryosuke Niwa 2013-11-04 10:26:29 PST
Comment on attachment 215920 [details]
Proposed patch

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

Did you run test-webkitpy webkitpy.common.checkout.scm.scm_unittest?

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:313
> +        git_log = self._run_git(['log', '-1', '--grep=' + '^\s*git-svn-id:.*@%s ' % svn_revision])

What's the point of +?
Comment 8 Peter Molnar 2013-11-04 12:25:04 PST
(In reply to comment #7)
> (From update of attachment 215920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215920&action=review
> 
> Did you run test-webkitpy webkitpy.common.checkout.scm.scm_unittest?

Yes, in fact I ran "test-webkitpy --all" that covers all SCM tests as far as I know. All webkitpy tests were passing.

> 
> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:313
> > +        git_log = self._run_git(['log', '-1', '--grep=' + '^\s*git-svn-id:.*@%s ' % svn_revision])
> 
> What's the point of +?

As mentioned earlier in that file:
# We use '--grep=' + foo rather than '--grep', foo because
# git 1.7.0.4 (and earlier) didn't support the separate arg.
Comment 9 Ryosuke Niwa 2013-11-04 12:28:39 PST
Comment on attachment 215920 [details]
Proposed patch

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

>>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:313
>>> +        git_log = self._run_git(['log', '-1', '--grep=' + '^\s*git-svn-id:.*@%s ' % svn_revision])
>> 
>> What's the point of +?
> 
> As mentioned earlier in that file:
> # We use '--grep=' + foo rather than '--grep', foo because
> # git 1.7.0.4 (and earlier) didn't support the separate arg.

I get that but why don't you just do '--grep=^\s*git-svn-id:.*@%s ' instead?  It seems like ' + ' is a useless noise.
Comment 10 Peter Molnar 2013-11-04 12:48:32 PST
Created attachment 215947 [details]
Patch updated

I just thought it was easier to read that way. New patch attached.
Comment 11 WebKit Commit Bot 2013-11-07 01:19:13 PST
Comment on attachment 215947 [details]
Patch updated

Clearing flags on attachment: 215947

Committed r158828: <http://trac.webkit.org/changeset/158828>
Comment 12 WebKit Commit Bot 2013-11-07 01:19:16 PST
All reviewed patches have been landed.  Closing bug.