Bug 36477

Summary: webkit-patch reads wrong bug url from unified diff context
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, ddkilzer, eric
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch v0
none
Patch none

Description Hajime Morrita 2010-03-22 21:27:32 PDT
With "post-commits", webkit-patch can read wrong URL from "context" part of diff text

+        We expect https://bugs.webkit.org/show_bug.cgi?id=12345 to be found
+        but,
@@ -23,4 +25,4 @@ ....
           It found this http://webkit.org/b/67890 instead.

How to reproduce:

1. Checkout git.webkit.org tree
2. Ensure last changelog contains http://webkit.org/b/123 style bug  URL
3. make change, with changelog which containshttps://bugs.webkit.org/show_bug.cgi=456 style bug URL
4. try post-commits --dry for that change

Expected:
- the patch would be post for Bug 456
Actual:
- the patch is post for 123

A concrete example of such patch is https://bug-36338-attachments.webkit.org/attachment.cgi?id=51146
Comment 1 Hajime Morrita 2010-03-22 21:58:56 PDT
Created attachment 51393 [details]
patch v0
Comment 2 Eric Seidel (no email) 2010-03-22 22:47:49 PDT
Comment on attachment 51393 [details]
patch v0

This is the wrong fix.  In this case you want to just use the code for pulling the bug from the ChangeLogEntry instead of the diff.  This proposed change would still wrongly grab bug urls added in other parts of the diff besides the ChangeLog.

I recently added a ChangeLogEntry class which exposes a bug_id() method which should fix this case.
Comment 3 Eric Seidel (no email) 2010-03-22 22:48:21 PDT
Thank you for the fix attempt though!  I think we have at least one dup of this bug.
Comment 4 Hajime Morrita 2010-03-22 23:09:14 PDT
Thank you for reviewing quickly!

(In reply to comment #2)
> This is the wrong fix.  In this case you want to just use the code for pulling
> the bug from the ChangeLogEntry instead of the diff.  This proposed change
> would still wrongly grab bug urls added in other parts of the diff besides the
> ChangeLog.
Agreed.

> I recently added a ChangeLogEntry class which exposes a bug_id() method which
> should fix this case.
I couldn't find bug_id() on ChangeLogEntry. It looks not landed yet.
Would you tell me which bug contains that?
I'll watch it and retry this after its landing.

BTW, Bug 33471 and Bug 33197 looks similar. But I'm not sure.
Comment 5 Adam Barth 2010-03-31 02:22:28 PDT
Created attachment 52145 [details]
Patch
Comment 6 Eric Seidel (no email) 2010-03-31 11:24:06 PDT
Comment on attachment 52145 [details]
Patch

I don't think it's right that commit_message_for_this_commit throws.  But that's an old mistake of mine.  Would be nice to have a FIXME in your try-catch, but this is fine as is.
Comment 7 WebKit Commit Bot 2010-03-31 13:28:27 PDT
Comment on attachment 52145 [details]
Patch

Clearing flags on attachment: 52145

Committed r56863: <http://trac.webkit.org/changeset/56863>
Comment 8 WebKit Commit Bot 2010-03-31 13:28:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Eric Seidel (no email) 2010-04-02 00:21:56 PDT
*** Bug 33471 has been marked as a duplicate of this bug. ***