RESOLVED FIXED 108788
Don't update author info in PrepareChangeLog and allow users to skip the PrepareChangeLog step entirely.
https://bugs.webkit.org/show_bug.cgi?id=108788
Summary Don't update author info in PrepareChangeLog and allow users to skip the Prep...
Timothy Loh
Reported 2013-02-03 21:28:07 PST
Don't update author info in PrepareChangeLog and allow users to skip the PrepareChangeLog step entirely.
Attachments
Patch (9.60 KB, patch)
2013-02-03 21:43 PST, Timothy Loh
no flags
Patch (9.82 KB, patch)
2013-02-03 22:17 PST, Timothy Loh
no flags
Timothy Loh
Comment 1 2013-02-03 21:43:37 PST
Ryosuke Niwa
Comment 2 2013-02-03 21:50:13 PST
Comment on attachment 186300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186300&action=review > Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:87 > - new_date_line = new_entry.date_line() > - old_date_line = old_entry.date_line() > - if new_date_line != old_date_line: > - final_entry = final_entry.replace(old_date_line, new_date_line) > + final_entry = final_entry.replace(old_entry.date(), new_entry.date(), 1) Why are we making this change? This patch needs per-function change log. > Tools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py:99 > + ((4, 0, 0, 0), (0, 0, 0, 0), (0, 0, 0, 0)), > + ((5, 0, 0, 0), (0, 0, 0, 0), (1, 0, 0, 0)), > + ((0, 0, 0, 0), (4, 0, 0, 0), (4, 0, 0, 0)), > + ((1, 0, 0, 0), (4, 0, 0, 0), (5, 0, 0, 0)), What the heck is this mysterious matrix?
Timothy Loh
Comment 3 2013-02-03 21:53:46 PST
(In reply to comment #2) > (From update of attachment 186300 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186300&action=review > > > Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:87 > > - new_date_line = new_entry.date_line() > > - old_date_line = old_entry.date_line() > > - if new_date_line != old_date_line: > > - final_entry = final_entry.replace(old_date_line, new_date_line) > > + final_entry = final_entry.replace(old_entry.date(), new_entry.date(), 1) > > Why are we making this change? This patch needs per-function change log. I'm not sure what you're asking here. > > Tools/Scripts/webkitpy/tool/steps/preparechangelog_unittest.py:99 > > + ((4, 0, 0, 0), (0, 0, 0, 0), (0, 0, 0, 0)), > > + ((5, 0, 0, 0), (0, 0, 0, 0), (1, 0, 0, 0)), > > + ((0, 0, 0, 0), (4, 0, 0, 0), (4, 0, 0, 0)), > > + ((1, 0, 0, 0), (4, 0, 0, 0), (5, 0, 0, 0)), > > What the heck is this mysterious matrix? It looks a bit odd without context, but basically each 4-tuple represents a ChangeLog entry and each line is (new entry, old entry, expected result entry).
Ryosuke Niwa
Comment 4 2013-02-03 22:01:39 PST
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 186300 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=186300&action=review > > > > > Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:87 > > > - new_date_line = new_entry.date_line() > > > - old_date_line = old_entry.date_line() > > > - if new_date_line != old_date_line: > > > - final_entry = final_entry.replace(old_date_line, new_date_line) > > > + final_entry = final_entry.replace(old_entry.date(), new_entry.date(), 1) > > > > Why are we making this change? This patch needs per-function change log. > > I'm not sure what you're asking here. Exactly what I said. It's not obvious why this code change is necessary or desirable to fix the bug. This is why I'm suggesting to add per-function description in your change log so that others can understand why you're making each code change.
Timothy Loh
Comment 5 2013-02-03 22:07:45 PST
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 186300 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=186300&action=review > > > > > > > Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:87 > > > > - new_date_line = new_entry.date_line() > > > > - old_date_line = old_entry.date_line() > > > > - if new_date_line != old_date_line: > > > > - final_entry = final_entry.replace(old_date_line, new_date_line) > > > > + final_entry = final_entry.replace(old_entry.date(), new_entry.date(), 1) > > > > > > Why are we making this change? This patch needs per-function change log. > > > > I'm not sure what you're asking here. > > Exactly what I said. It's not obvious why this code change is necessary or desirable to fix the bug. This is why I'm suggesting to add per-function description in your change log so that others can understand why you're making each code change. Ah, I was confused because the patch is about change logs so I was trying to interpret the sentence differently.
Timothy Loh
Comment 6 2013-02-03 22:17:47 PST
WebKit Review Bot
Comment 7 2013-02-04 16:31:56 PST
Comment on attachment 186306 [details] Patch Clearing flags on attachment: 186306 Committed r141829: <http://trac.webkit.org/changeset/141829>
WebKit Review Bot
Comment 8 2013-02-04 16:32:00 PST
All reviewed patches have been landed. Closing bug.
Julien Chaffraix
Comment 9 2013-02-04 16:47:34 PST
FWIW I was looking into who 'regressed' webkit-patch upload today as automatically regenerating the *whole* ChangeLog more or less every time you touch something without a way of opt-out was annoying me. I shot myself in the foot because of this behavior and don't see a good way to prevent it from happening regularly unless opting out (merging the new entries after the old ones makes it hard to see them IMO). See http://trac.webkit.org/changeset/141810/trunk/Source/WebCore/ChangeLog where the ChangeLog landed duplicated because I didn't pay close attention.
Note You need to log in before you can comment on or make changes to this bug.