Bug 147248 – Call fixChangeLogPatch when generating patches from webkit-patch

• Status: REOPENED
• Component: Tools / Tests

[Basile Clement]

Call fixChangeLogPatch when generating patches from webkit-patch

[Basile Clement]

Created attachment 257419 [details]
Patch

[Geoffrey Garen]

Comment on attachment 257419 [details]
Patch

r=me

[Basile Clement]

Created attachment 257464 [details]
Patch for landing

[Basile Clement]

Comment on attachment 257464 [details]
Patch for landing

"Reviewed by NOBODY (OOPS!)"

[Basile Clement]

(In reply to comment #4)
> Comment on attachment 257464 [details]
> Patch for landing
> 
> "Reviewed by NOBODY (OOPS!)"

We should really make "webkit-patch land -g commitish" correctly handle the reviewer it got from bugzilla...

[Basile Clement]

Created attachment 257465 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 257465 [details]
Patch for landing

Clearing flags on attachment: 257465

Committed r187357: <http://trac.webkit.org/changeset/187357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to comment #8)
> All reviewed patches have been landed.  Closing bug.

This patch is incorrect because it makes the assumption that VCSUtils::fixChangeLogPatch() can be called with the stringified content of an arbitrary patch. But VCSUtils::fixChangeLogPatch() assumes its argument represents a ChangeLog patch as stated in an inline comment in the first line of the function body [1] and implied in the description above VCSUtils::fixChangeLogPatch() [2]. As a result of this bad assumption, webkit-patch may generate an incorrect patch file in the worst case and only adjust the diff of the first ChangeLog file in a patch in the best case.

Additional remarks:

An incorrect patch is only generated when its contents has the form of two consecutive ChangeLog entries that have one or more identical lines. Looking at incorrect patches generated with this patch. In the best case, this patch will have webkit-patch generate ChangeLog diffs that can only be applied with a fuzz factor > 3. One example of this can be seen in the WebCore ChangeLog diff in attachment #258971 [details], which can only be applied with fuzz factor >= 5 (the fuzz factor of the generated patch is proportional to the number of lines in the diff chunk range for the source ChangeLog file whose ChangeLog entry was adjusted). In the worst case, it can generate an incorrect patch that is not faithful to the actual changes/intentions of the author and may be difficult to identify as incorrect by eye, especially if such a patch applies without warnings/errors and a person or reviewer is not looking out for such incorrect patches.

[1] <http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm?rev=185710#L1599>
[2] <http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm?rev=185710#L1570>

[Daniel Bates]

(In reply to comment #7)
> Comment on attachment 257465 [details]
> Patch for landing
> 
> Clearing flags on attachment: 257465
> 
> Committed r187357: <http://trac.webkit.org/changeset/187357>

I propose that we roll out this change.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148032