Bug 74358 - prepare-Changelog should support updating the list of changed files
Summary: prepare-Changelog should support updating the list of changed files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: ToolsHitList
Depends on: 107478
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-12 15:17 PST by Ojan Vafai
Modified: 2013-02-03 21:47 PST (History)
11 users (show)

See Also:


Attachments
Patch (10.28 KB, patch)
2013-01-22 19:27 PST, Timothy Loh
no flags Details | Formatted Diff | Diff
Patch (10.29 KB, patch)
2013-01-22 20:52 PST, Timothy Loh
no flags Details | Formatted Diff | Diff
Patch (10.18 KB, patch)
2013-01-22 22:51 PST, Timothy Loh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-12-12 15:17:10 PST
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.
Comment 1 Eric Seidel (no email) 2011-12-12 15:25:34 PST
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. :)
Comment 2 Darin Adler 2011-12-12 15:31:25 PST
(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.
Comment 3 Ojan Vafai 2011-12-12 16:05:06 PST
(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.
Comment 4 Timothy Loh 2013-01-22 19:27:44 PST
Created attachment 184108 [details]
Patch
Comment 5 Eric Seidel (no email) 2013-01-22 20:43:49 PST
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.
Comment 6 Timothy Loh 2013-01-22 20:52:24 PST
(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.
Comment 7 Timothy Loh 2013-01-22 20:52:57 PST
Created attachment 184122 [details]
Patch
Comment 8 Eric Seidel (no email) 2013-01-22 22:11:49 PST
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.
Comment 9 Eric Seidel (no email) 2013-01-22 22:16:04 PST
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.
Comment 10 Timothy Loh 2013-01-22 22:41:37 PST
(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.
Comment 11 Timothy Loh 2013-01-22 22:51:01 PST
Created attachment 184147 [details]
Patch
Comment 12 Eric Seidel (no email) 2013-01-22 22:56:27 PST
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.
Comment 13 WebKit Review Bot 2013-01-22 23:23:59 PST
Comment on attachment 184147 [details]
Patch

Clearing flags on attachment: 184147

Committed r140511: <http://trac.webkit.org/changeset/140511>
Comment 14 WebKit Review Bot 2013-01-22 23:24:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Ryosuke Niwa 2013-02-01 11:41:13 PST
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.
Comment 16 Eric Seidel (no email) 2013-02-01 12:47:50 PST
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" :)
Comment 17 Ryosuke Niwa 2013-02-01 13:03:25 PST
(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.
Comment 18 Timothy Loh 2013-02-03 14:54:02 PST
(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.
Comment 19 Eric Seidel (no email) 2013-02-03 16:55:44 PST
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.
Comment 20 Ryosuke Niwa 2013-02-03 16:56:18 PST
(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.
Comment 21 Timothy Loh 2013-02-03 19:11:03 PST
(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).
Comment 22 Eric Seidel (no email) 2013-02-03 20:49:02 PST
(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.
Comment 23 Timothy Loh 2013-02-03 21:47:26 PST
(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.