Bug 74358 – prepare-Changelog should support updating the list of changed files

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ojan Vafai]

It's really annoying when the list of files in the ChangeLog entry changes and you need to delete the whole entry and start over. Am I doing it wrong?

Specifically, I'd like the script to:
1. Update the one-line description if the bug description has changed.
2. Update the list of changed files.
3. Update the date in the log entry.

[Eric Seidel (no email)]

It's presumable that we could write a smarter prepare-ChangeLog script now that our ChangeLog parsing in webkitpy is quite fancy (thanks to ryosuke), but I think that would be a relatively large endeavor.

The current recommended method is to just re-make the entry.  prepare-ChagneLog does not know how to parse its own output. :)

[Darin Adler]

(In reply to comment #0)
> 1. Update the one-line description if the bug description has changed.

This is new territory for prepare-ChangeLog, since currently it does not do anything with the description.

> 2. Update the list of changed files.

I presume you mean both he changed files and functions.

If it did this, it would need to highlight the newly added items, for people like me who add comments to each function in the list. Otherwise, it would be easy to miss something.

> 3. Update the date in the log entry.

Great idea.

[Ojan Vafai]

(In reply to comment #2)
> (In reply to comment #0)
> > 1. Update the one-line description if the bug description has changed.
> 
> This is new territory for prepare-ChangeLog, since currently it does not do anything with the description.

Yes. I'm also OK with it being a different script or a special command-line flag, or even just being part of "webkit-patch upload".

> > 2. Update the list of changed files.
> 
> I presume you mean both he changed files and functions.

Yes.

> If it did this, it would need to highlight the newly added items, for people like me who add comments to each function in the list. Otherwise, it would be easy to miss something.

Oh, that's a great idea. Not sure how to do that though. It would be important to keep old comments I think. Some ideas:
1. Add a * at the beginning of the line.
2. Add an [OOPS new changelog line] to the line
3. Instead of interleaving, when the list changes, just add both sets in their entirety (one above the other) and the let the developer resolve it.

1 and 2 seem awful. 3 actually doesn't seem that bad to me.

[Timothy Loh]

Created attachment 184108 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 184108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184108&action=review

> Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:65
> +    @staticmethod
> +    def _resolve_existing_entry(changelog_path):

No need for statics.  We really don't use @staticmethod in webkitpy.  Maybe we should, but historically it's been a huge testing pain.

[Timothy Loh]

(In reply to comment #5)
> (From update of attachment 184108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184108&action=review
> 
> > Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:65
> > +    @staticmethod
> > +    def _resolve_existing_entry(changelog_path):
> 
> No need for statics.  We really don't use @staticmethod in webkitpy.  Maybe we should, but historically it's been a huge testing pain.

Whoops, you'd think I'd remember after getting rid of some yesterday.

[Timothy Loh]

Created attachment 184122 [details]
Patch

[Eric Seidel (no email)]

I should be clear that my prejudice against statics is just that.  If you have python documentation which says that staticmethods are awesome and we should use them (and explains how nicely to test/mock them) then great.  But my limited experiance in python has taught me to fear @staticmethod, and very very strongly prefer instance methods when possible.  Anytime I can't seem to avoid staticmethod, I consider making a single shared instance of a class and defining all its members as instances instead.

[Eric Seidel (no email)]

Comment on attachment 184122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184122&action=review

This looks OK to me.  There are some small mentions below.

> Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:64
> +    def _resolve_existing_entry(self, changelog_path):

This function is a bit long for my tastes.  I'm not sure how I would break it up, possibly into _update_date_line and _update_description functions.

> Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:65
> +        """When this is called, the top entry in the ChangeLog has just been

It's better to assert instead of document via comment IMO.

> Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:74
> +            entries_gen = ChangeLog.parse_entries_from_file(changelog_file)

self. will make this easier to mock/override when we finally make this class ues Filesystem and thus be more testable.

[Timothy Loh]

(In reply to comment #9)
> (From update of attachment 184122 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184122&action=review
> 
> This looks OK to me.  There are some small mentions below.
> 
> > Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:64
> > +    def _resolve_existing_entry(self, changelog_path):
> 
> This function is a bit long for my tastes.  I'm not sure how I would break it up, possibly into _update_date_line and _update_description functions.

I'll split this into reading/writing the ChangeLog and merging the entries.

> > Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:74
> > +            entries_gen = ChangeLog.parse_entries_from_file(changelog_file)
> 
> self. will make this easier to mock/override when we finally make this class ues Filesystem and thus be more testable.

We're in PrepareChangeLog here, not ChangeLog.

[Timothy Loh]

Created attachment 184147 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 184147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184147&action=review

OK.

> Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:31
> +import codecs
>  import logging
> +import os

codecs and os imports are always "bad signs" that code is un-mockable. :)

> Tools/Scripts/webkitpy/tool/steps/preparechangelog.py:33
>  import sys

So is sys!  Clearly this file pre-dates our good FileSystem/Executive mocks.

[WebKit Review Bot]

Comment on attachment 184147 [details]
Patch

Clearing flags on attachment: 184147

Committed r140511: <http://trac.webkit.org/changeset/140511>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Does this command run automatically on webkit-patch upload or webkit-patch land? If so, we need an option to disable it. I don't want these commands to do random stuff on my change log.

[Eric Seidel (no email)]

That seems like a very conservative viewpoint.  I hear you saying "I don't trust this code, I want it off."  An alternate view might be "I don't want this code to break stuff, we better make sure it's correct/predictable/understandable" :)

[Ryosuke Niwa]

(In reply to comment #16)
> That seems like a very conservative viewpoint.  I hear you saying "I don't trust this code, I want it off."  An alternate view might be "I don't want this code to break stuff, we better make sure it's correct/predictable/understandable" :)

I just don't want my change logs to get updated. There are times I apply someone's patch and upload that again so I need an option for the script not to mess with the change logs I already have.

[Timothy Loh]

(In reply to comment #15)
> Does this command run automatically on webkit-patch upload or webkit-patch land? If so, we need an option to disable it. I don't want these commands to do random stuff on my change log.

This runs on prepare, upload and land-cowboy.

(In reply to comment #17)
> (In reply to comment #16)
> > That seems like a very conservative viewpoint.  I hear you saying "I don't trust this code, I want it off."  An alternate view might be "I don't want this code to break stuff, we better make sure it's correct/predictable/understandable" :)
> 
> I just don't want my change logs to get updated. There are times I apply someone's patch and upload that again so I need an option for the script not to mess with the change logs I already have.

I think the only thing this patch would make webkit-patch do that you won't want is replace the name/email with yours. The rest of the entry should stay the same unless the changed files/functions list or the bug description has changed. I didn't have your use case in mind here, so it doesn't quite do what you'd like. Both adding an option and not replacing the name/email should be straightforward changes, so I guess it's up to what you'd rather have.

[Eric Seidel (no email)]

I see.  Well, updating the author is perhaps a bit extreme, and probably suggests we should ask for confirmation.  It's very easy to do, and we just need to respect the --non-interactive bit.

[Ryosuke Niwa]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > That seems like a very conservative viewpoint.  I hear you saying "I don't trust this code, I want it off."  An alternate view might be "I don't want this code to break stuff, we better make sure it's correct/predictable/understandable" :)
> > 
> > I just don't want my change logs to get updated. There are times I apply someone's patch and upload that again so I need an option for the script not to mess with the change logs I already have.
> 
> I think the only thing this patch would make webkit-patch do that you won't want is replace the name/email with yours. The rest of the entry should stay the same unless the changed files/functions list or the bug description has changed. I didn't have your use case in mind here, so it doesn't quite do what you'd like. Both adding an option and not replacing the name/email should be straightforward changes, so I guess it's up to what you'd rather have.

Adding an option to not update the change log will be good. There are cases where you want to intentionally omit some files e.g. a patch to rebaseline thousands of expected results.

[Timothy Loh]

(In reply to comment #19)
> I see.  Well, updating the author is perhaps a bit extreme, and probably suggests we should ask for confirmation.  It's very easy to do, and we just need to respect the --non-interactive bit.

What's do you think is the right thing to do here if we are in non-interactive mode? I think leaving the author unchanged seems a reasonable default, but I'm not sure it makes sense to call this with --non-interactive given the rest of the added behaviour in PrepareChangeLog (i.e. adding the changed list below).

[Eric Seidel (no email)]

(In reply to comment #21)
> (In reply to comment #19)
> > I see.  Well, updating the author is perhaps a bit extreme, and probably suggests we should ask for confirmation.  It's very easy to do, and we just need to respect the --non-interactive bit.
> 
> What's do you think is the right thing to do here if we are in non-interactive mode? I think leaving the author unchanged seems a reasonable default, but I'm not sure it makes sense to call this with --non-interactive given the rest of the added behaviour in PrepareChangeLog (i.e. adding the changed list below).

I see, my point was more to notify you of the existance of the --non-interactive option, which all commands need to be aware to avoid trying to prompt a user when no user exists. :)

It's fine to remove the update-the-author functionality all together, it seems unlikely to be wanted very often.

The date and the changed files seem useful.

If folks don't want the list of changed files to include all files when there are more than N of them, then we should might code those heuristics into prepare-ChangeLog.

I guess I don't feel particularly strongly about any of this.  It seems that udating the list of changed files is a good default for webkit-patch.  If rniwa wants an option to be able to disable it, it is possible add an option.

[Timothy Loh]

(In reply to comment #22)
> I see, my point was more to notify you of the existance of the --non-interactive option, which all commands need to be aware to avoid trying to prompt a user when no user exists. :)
> 
> It's fine to remove the update-the-author functionality all together, it seems unlikely to be wanted very often.
> 
> The date and the changed files seem useful.
> 
> If folks don't want the list of changed files to include all files when there are more than N of them, then we should might code those heuristics into prepare-ChangeLog.
> 
> I guess I don't feel particularly strongly about any of this.  It seems that udating the list of changed files is a good default for webkit-patch.  If rniwa wants an option to be able to disable it, it is possible add an option.

I've created a patch for this over at Bug 108788.