Bug 48075 - webkit-patch doesn't get along with renamed files
Summary: webkit-patch doesn't get along with renamed files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
: 49154 57686 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-10-21 10:05 PDT by Tony Gentilcore
Modified: 2011-08-08 11:03 PDT (History)
14 users (show)

See Also:


Attachments
Patch (5.16 KB, patch)
2011-06-28 15:38 PDT, Wyatt Carss
no flags Details | Formatted Diff | Diff
Patch (6.22 KB, patch)
2011-08-08 00:41 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (6.17 KB, patch)
2011-08-08 01:09 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 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
Comment 1 Adam Barth 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.
Comment 2 Tony Gentilcore 2010-11-08 15:32:11 PST
I think this bit James in https://bugs.webkit.org/show_bug.cgi?id=48919.
Comment 3 Gavin Peters 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)
Comment 4 Eric Seidel (no email) 2011-04-18 19:08:21 PDT
*** Bug 49154 has been marked as a duplicate of this bug. ***
Comment 5 Eric Seidel (no email) 2011-04-18 19:08:30 PDT
*** Bug 57686 has been marked as a duplicate of this bug. ***
Comment 6 Eric Seidel (no email) 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.
Comment 7 Tony Gentilcore 2011-06-23 05:42:18 PDT
Another victim: https://bugs.webkit.org/show_bug.cgi?id=63122
Comment 8 Wyatt Carss 2011-06-28 15:38:52 PDT
Created attachment 98981 [details]
Patch
Comment 9 Adam Barth 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...
Comment 10 Wyatt Carss 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.
Comment 11 Adam Barth 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.
Comment 12 Eric Seidel (no email) 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!
Comment 13 Eric Seidel (no email) 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
Comment 14 Adam Roben (:aroben) 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?
Comment 15 jochen 2011-08-08 00:41:06 PDT
Created attachment 103214 [details]
Patch
Comment 16 Adam Barth 2011-08-08 00:43:19 PDT
Eric should review this patch.
Comment 17 jochen 2011-08-08 01:09:04 PDT
Created attachment 103219 [details]
Patch
Comment 18 Evan Martin 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.
Comment 19 Eric Seidel (no email) 2011-08-08 09:07:51 PDT
Comment on attachment 103219 [details]
Patch

Seems reasonable.  Would be nice to have more testing.
Comment 20 WebKit Review Bot 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>
Comment 21 WebKit Review Bot 2011-08-08 11:03:20 PDT
All reviewed patches have been landed.  Closing bug.