Bug 108983 – webkit-patch upload regenerates the WebCore ChangeLog every time it's called

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

How to reproduce:

* WebKit revision r141929 (git checkout FWIW)
* Apply https://bug-108975-attachments.webkit.org/attachment.cgi?id=186706
* Remove the ChangeLogs diffs (git checkout LayoutTests/ChangeLog Source/WebCore/ChangeLog)
* Call prepare-ChangeLog -b 108975
* Edit the WebCore ChangeLog and paste back the one from the attachment.
* Enjoy calling webkit-patch upload and set it regenerate the ChangeLog every time it's called.

[Timothy Loh]

(In reply to comment #0)
> How to reproduce:
> 
> * WebKit revision r141929 (git checkout FWIW)
> * Apply https://bug-108975-attachments.webkit.org/attachment.cgi?id=186706
> * Remove the ChangeLogs diffs (git checkout LayoutTests/ChangeLog Source/WebCore/ChangeLog)
> * Call prepare-ChangeLog -b 108975
> * Edit the WebCore ChangeLog and paste back the one from the attachment.
> * Enjoy calling webkit-patch upload and set it regenerate the ChangeLog every time it's called.

Did you manually remove "(WebCore):" and "(RenderBox):" from the list, because if so, this is *technically* a feature (albeit an annoying one), and we can work out some heuristics to allow this behaviour to pass (maybe allow users to delete (\w*): from .cpp and .h files).

[Ryosuke Niwa]

(In reply to comment #1)
> Did you manually remove "(WebCore):" and "(RenderBox):" from the list, because if so, this is *technically* a feature (albeit an annoying one), and we can work out some heuristics to allow this behaviour to pass (maybe allow users to delete (\w*): from .cpp and .h files).

I would have preferred for features like this one to run only when developers told the tool to do so. Automatically adding things back into change log seems overly aggressive.

[Timothy Loh]

(In reply to comment #2)
> (In reply to comment #1)
> > Did you manually remove "(WebCore):" and "(RenderBox):" from the list, because if so, this is *technically* a feature (albeit an annoying one), and we can work out some heuristics to allow this behaviour to pass (maybe allow users to delete (\w*): from .cpp and .h files).
> 
> I would have preferred for features like this one to run only when developers told the tool to do so. Automatically adding things back into change log seems overly aggressive.

The main point of the feature is to add to the change log entry new touched files/functions. At the moment there's really no way to tell the difference between what's been just added, or what's been removed. The way I see it is that adding functionality to see what the user has removed is unnecessary since the simplest solution is to just leave in the lines like (WebCore) in the change log entries (which I think most people would be doing anyway), which makes this is a non-issue.

Another option is to revert to the previous behaviour (i.e. don't update the change logs with the new list) by default, and have the new behaviour behind a flag, but I'm not sure that this would be preferred by other developers.

[Ryosuke Niwa]

(In reply to comment #3)
> The main point of the feature is to add to the change log entry new touched files/functions. At the moment there's really no way to tell the difference between what's been just added, or what's been removed.

That's why I'm suggesting to make this an opt-in feature. It's annoying for a tool to enforce a particular change when that change is not necessarily desirable.

>The way I see it is that adding functionality to see what the user has removed is unnecessary since the simplest solution is to just leave in the lines like (WebCore) in the change log entries (which I think most people would be doing anyway), which makes this is a non-issue.

That's not an option. The reason a developer deletes such an entry is because it's incorrect or unnecessary.

> Another option is to revert to the previous behaviour (i.e. don't update the change logs with the new list) by default, and have the new behaviour behind a flag, but I'm not sure that this would be preferred by other developers.

I think that's a much better behavior. The the patch made webkit-patch useless in many situations until we added --no-prepare-changelogs.

[Ryosuke Niwa]

In particular, prepare-ChangeLogs is terrible at generating change logs in non-C++ languages.

e.g.
function Class() {
    this.method = function () {...}
}

results in:

(Class):
(this.method)

While I appreciate your contribution to the project, prepare-ChangeLogs does not generate the most appropriate change logs at the moment, and replacing change logs humans manually edited is not going to help it.

[Julien Chaffraix]

> Did you manually remove "(WebCore):" and "(RenderBox):" from the list, because if so, this is *technically* a feature (albeit an annoying one), and we can work out some heuristics to allow this behaviour to pass (maybe allow users to delete (\w*): from .cpp and .h files).

Good point, I indeed removed the namespace / empty class entries as I find that they don't add anything to the ChangeLog. I also usually advise people in reviews to do the same.

If we fix this, this would probably solve my pain point with dealing with webkit-patch upload. Though there is still the possibility of shooting yourself in the foot that would need to be solved. A (really bad) idea would be to add some OOPS so that the commit hook can detect duplicated entries.

[Timothy Loh]

(In reply to comment #4)
> (In reply to comment #3)
> > The main point of the feature is to add to the change log entry new touched files/functions. At the moment there's really no way to tell the difference between what's been just added, or what's been removed.
> 
> That's why I'm suggesting to make this an opt-in feature. It's annoying for a tool to enforce a particular change when that change is not necessarily desirable.

OK. I'll put a patch to revert the default behaviour to pre-r140511 (i.e. Bug 74358), with the new behaviour available behind --update-changelogs.

[Timothy Loh]

Created attachment 187190 [details]
Patch

[Ryosuke Niwa]

I'm hitting the bug where webkit-patch upload duplicates change log entries when I manually edit them :(

[WebKit Review Bot]

Comment on attachment 187190 [details]
Patch

Clearing flags on attachment: 187190

Committed r142589: <http://trac.webkit.org/changeset/142589>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.