Bug 48075

Summary: webkit-patch doesn't get along with renamed files
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: Tools / TestsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, dbates, eric, evan, gavinp, jknotten, kamaji, mihaip, simonjam, tommyw, tonyg, wcarss, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Tony Gentilcore
Reported 2010-10-21 10:05:03 PDT
https://bug-47560-attachments.webkit.org/attachment.cgi?id=71447 adds files in two different ways: # new file: WebCore/dom/IgnoreDestructiveWriteCountIncrementer.h # renamed: LayoutTests/fast/dom/HTMLScriptElement/defer-double-defer-wr\ ite-expected.txt -> LayoutTests/fast/dom/HTMLScriptElement/write-after-ignored-w\ rite-expected.txt In a git diff, both show up as "create mode" and diff as expected. However, webkit-patch pretty-diff only shows the "new file", not the "renamed". I suspect this is related to: https://bugs.webkit.org/show_bug.cgi?id=47940
Attachments
Patch (5.16 KB, patch)
2011-06-28 15:38 PDT, Wyatt Carss 		no flags
DetailsFormatted DiffDiff
Patch (6.22 KB, patch)
2011-08-08 00:41 PDT, jochen 		no flags
DetailsFormatted DiffDiff
Patch (6.17 KB, patch)
2011-08-08 01:09 PDT, jochen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2010-10-21 11:48:12 PDT
Hum... Maybe changed_files() doesn't find them? The other approach is to remove the "run stat less often" optimization. It seems to be causing more problems than its worth.
Tony Gentilcore
Comment 2 2010-11-08 15:32:11 PST
I think this bit James in https://bugs.webkit.org/show_bug.cgi?id=48919.
Gavin Peters
Comment 3 2010-11-29 07:25:45 PST
Burned me in https://bugs.webkit.org/show_bug.cgi?id=35404 too, had to run git diff --binary manually (and of course I got it wrong the first time)
Eric Seidel (no email)
Comment 4 2011-04-18 19:08:21 PDT
*** Bug 49154 has been marked as a duplicate of this bug. ***
Eric Seidel (no email)
Comment 5 2011-04-18 19:08:30 PDT
*** Bug 57686 has been marked as a duplicate of this bug. ***
Eric Seidel (no email)
Comment 6 2011-04-18 19:08:50 PDT
This is a really bad bug. We need to remove the changed_files optimization if it's broken.
Tony Gentilcore
Comment 7 2011-06-23 05:42:18 PDT
Another victim: https://bugs.webkit.org/show_bug.cgi?id=63122
Wyatt Carss
Comment 8 2011-06-28 15:38:52 PDT
Created attachment 98981 [details] Patch
Adam Barth
Comment 9 2011-06-28 16:33:32 PDT
Comment on attachment 98981 [details] Patch Is this going to cause us to lose history when we commit? I assume not since you haven't changed the commit command...
Wyatt Carss
Comment 10 2011-06-29 13:54:10 PDT
(In reply to comment #9) > (From update of attachment 98981 [details]) > Is this going to cause us to lose history when we commit? I assume not since you haven't changed the commit command... I'm not sure what sort of history you mean.. On further investigation, this probably isn't a good enough fix. It will show the deletion of the previous file and the creation of the new file, but it can't highlight changes between the two the same way svn does. So, this fixes a symptom, but I think it more or less bypasses the actual problem: the script probably just isn't interpreting git's rename and copy information correctly. As far as a temporary improvement, this *does* make it so that git won't lose files / muck up rename operations, but it makes close review difficult.
Adam Barth
Comment 11 2011-06-29 13:55:43 PDT
> On further investigation, this probably isn't a good enough fix. It will show the deletion of the previous file and the creation of the new file, but it can't highlight changes between the two the same way svn does. So, this fixes a symptom, but I think it more or less bypasses the actual problem: the script probably just isn't interpreting git's rename and copy information correctly. That's how this used to work before we broke it, so that's probably a step in the right direction.
Eric Seidel (no email)
Comment 12 2011-07-06 14:03:42 PDT
OK. So dbates has landed his amazing "make the scm.py tests 4x faster" patch, so it should be relatively easy to unit test this bugger. Once it has a unit test, I would like evmar to sign-off on the git changes. Then it's good to go!
Eric Seidel (no email)
Comment 13 2011-07-06 14:04:22 PDT
Comment on attachment 98981 [details] Patch r- for lack of tests. Should be easy to do now that one can actually run the scm unit tests. try test-webkitpy --all
Adam Roben (:aroben)
Comment 14 2011-07-06 15:01:09 PDT
Comment on attachment 98981 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98981&action=review > Tools/ChangeLog:13 > + ultimately the behaviour we want. The old file is shown deleted, > + then the new file is shown added, followed by any changes that > + occurred. Also gets rid of the problem where deleting one file This presentation will be worse for reviewing patches. I wonder if we can teach the review tool to reconstruct the rename patch?
jochen
Comment 15 2011-08-08 00:41:06 PDT
Created attachment 103214 [details] Patch
Adam Barth
Comment 16 2011-08-08 00:43:19 PDT
Eric should review this patch.
jochen
Comment 17 2011-08-08 01:09:04 PDT
Created attachment 103219 [details] Patch
Evan Martin
Comment 18 2011-08-08 07:54:18 PDT
(git review) The patch seems to do what the review says it does, though I'm a little confused as to why your tools don't support renames.
Eric Seidel (no email)
Comment 19 2011-08-08 09:07:51 PDT
Comment on attachment 103219 [details] Patch Seems reasonable. Would be nice to have more testing.
WebKit Review Bot
Comment 20 2011-08-08 11:03:14 PDT
Comment on attachment 103219 [details] Patch Clearing flags on attachment: 103219 Committed r92609: <http://trac.webkit.org/changeset/92609>
WebKit Review Bot
Comment 21 2011-08-08 11:03:20 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.