Bug 108983

Summary: webkit-patch upload regenerates the WebCore ChangeLog every time it's called
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, rniwa, timloh, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Description Julien Chaffraix 2013-02-05 14:34:09 PST
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.
Comment 1 Timothy Loh 2013-02-05 15:51:30 PST
(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).
Comment 2 Ryosuke Niwa 2013-02-05 16:33:16 PST
(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.
Comment 3 Timothy Loh 2013-02-05 23:11:51 PST
(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.
Comment 4 Ryosuke Niwa 2013-02-05 23:16:44 PST
(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.
Comment 5 Ryosuke Niwa 2013-02-05 23:22:15 PST
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.
Comment 6 Julien Chaffraix 2013-02-06 07:36:56 PST
> 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.
Comment 7 Timothy Loh 2013-02-07 15:42:48 PST
(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.
Comment 8 Timothy Loh 2013-02-07 15:51:07 PST
Created attachment 187190 [details]
Patch
Comment 9 Ryosuke Niwa 2013-02-11 23:53:30 PST
I'm hitting the bug where webkit-patch upload duplicates change log entries when I manually edit them :(
Comment 10 WebKit Review Bot 2013-02-11 23:59:41 PST
Comment on attachment 187190 [details]
Patch

Clearing flags on attachment: 187190

Committed r142589: <http://trac.webkit.org/changeset/142589>
Comment 11 WebKit Review Bot 2013-02-11 23:59:45 PST
All reviewed patches have been landed.  Closing bug.