Bug 48075 – webkit-patch doesn't get along with renamed files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Tony Gentilcore]

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

[Adam Barth]

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]

I think this bit James in https://bugs.webkit.org/show_bug.cgi?id=48919.

[Gavin Peters]

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)]

*** Bug 49154 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 57686 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

This is a really bad bug.  We need to remove the changed_files optimization if it's broken.

[Tony Gentilcore]

Another victim: https://bugs.webkit.org/show_bug.cgi?id=63122

[Wyatt Carss]

Created attachment 98981 [details]
Patch

[Adam Barth]

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]

(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]

> 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)]

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 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 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?

Created attachment 103214 [details]
Patch

[Adam Barth]

Eric should review this patch.

Created attachment 103219 [details]
Patch

[Evan Martin]

(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 on attachment 103219 [details]
Patch

Seems reasonable.  Would be nice to have more testing.

[WebKit Review Bot]

Comment on attachment 103219 [details]
Patch

Clearing flags on attachment: 103219

Committed r92609: <http://trac.webkit.org/changeset/92609>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.