Bug 108788 – Don't update author info in PrepareChangeLog and allow users to skip the PrepareChangeLog step entirely.

• Component: New Bugs
• Status: RESOLVED FIXED

[Timothy Loh]

Don't update author info in PrepareChangeLog and allow users to skip the PrepareChangeLog step entirely.

[Timothy Loh]

Created attachment 186300 [details]
Patch

[Ryosuke Niwa]

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]

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

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

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

Created attachment 186306 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186306 [details]
Patch

Clearing flags on attachment: 186306

Committed r141829: <http://trac.webkit.org/changeset/141829>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

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.