http://trac.webkit.org/changeset/94955 It was an r+ -ed patch, but "Reviewed by NOBODY (OOPS!)." line was deleted from the patch accidentally and webkit-patch didn't add the name of the reviewer to the changelog and commit log. It would be great if webkit-patch can handle this situation.
Created attachment 141425 [details] proposed fix This modification do the same just send a boolean value if the reviewer was set and handle it.
Comment on attachment 141425 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=141425&action=review Just an informal review. > Tools/Scripts/webkitpy/common/checkout/changelog.py:345 > + founded_reviewer = re.search(".*NOBODY\s*\(OOPS!\).*", latest_entry, re.MULTILINE) English typo, it should be "found" not "founded". Also, you look for the pattern "NOBODY (OOPS!)" and if you find it, you add the reviewer name. How is this going to fix what has been proposed on bug title? > Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py:72 > + has_been_set = ChangeLog(changelog_path).set_reviewer(reviewer) Name is weird. What has been set? Also, it doesn't look natural that a function called "set_reviewer" also returns something.
Comment on attachment 141425 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=141425&action=review Changes to webkitpy require unit tests. Both changelog.py and updatechangelogswithreviewer.py have unit tests that should be easy to extend to test this change. > Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py:75 > + if not self._tool.user.confirm("Do you want to continue?", default="n"): Do we need to check the --non-interactive option before prompting the user? > Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py:76 > + self._exit(1) Is this the proper way to exist, or should we call error() ?
Likely Eric should review this change.
Comment on attachment 141425 [details] proposed fix The change does not appear to do what it claims to in the bug title. :)
(In reply to comment #5) > (From update of attachment 141425 [details]) > The change does not appear to do what it claims to in the bug title. :) You're right, Ossy and I discussed it and we concluded that the script should only inform the user about the issue because it's not clear where should put the "Reviewed by NOBODY (OOPS!)." line from code.
> You're right, Ossy and I discussed it and we concluded that the script should only inform the user about the issue because it's not clear where should put the "Reviewed by NOBODY (OOPS!)." line from code. Why not just put it after the first blank line after the bug URL?
Created attachment 142238 [details] proposed fix Now it does what the bug title says.
Comment on attachment 142238 [details] proposed fix This needs tests. :(
Created attachment 142465 [details] proposed fix Here is the test. I had to make 2 type from _new_entry_boilerplate because the set_reviewer function looks for bug url, but I had to keep the original _new_entry_boilerplate because the preparechangelog_unittest.py uses it.
Created attachment 147564 [details] proposed fix I updated the patch. Could somebody check it, please?
Comment on attachment 147564 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=147564&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:365 > + if found_bug_url and latest_entry: > + print "\n Reviewed by " + reviewer.encode("utf-8") + "." > + latest_entry = False People sometimes list multiple bug URLs so you'll probably need to account for that. To detect the second entry, you can use date_line_regexp instead.
View in context: https://bugs.webkit.org/attachment.cgi?id=147564&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:348 > + latest_entry = self.latest_entry()._contents Didn't you wanted to use the .contents() method instead of the _contents? It is marked as non-public, we should honor the conventions. > Tools/Scripts/webkitpy/common/checkout/changelog.py:349 > + found_reviewer = re.search(".*NOBODY\s*\(OOPS!\).*", latest_entry, re.MULTILINE) hmm.. do we really need the '.*' parts? It should work without those. > Tools/Scripts/webkitpy/common/checkout/changelog.py:357 > + latest_entry = True Do you really wanted to name this the same as above? Maybe this should be renamed to describe its purpose better. > Tools/Scripts/webkitpy/common/checkout/changelog.py:364 > + print "\n Reviewed by " + reviewer.encode("utf-8") + "." I think it'll be better to use the % operator to format strings (or .format)
Created attachment 147839 [details] proposed fix Improved patch based on comments.
View in context: https://bugs.webkit.org/attachment.cgi?id=147839&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:350 > + bug_url_regexp = 'https://bugs.webkit.org/show_bug.cgi\?id=' This url is already present in the config_urls which is imported at the top. Also what if someone uses a short url for the bug? > Tools/Scripts/webkitpy/common/checkout/changelog.py:366 > + bug_url_number -= 1 > + elif found_bug_url: > + bug_url_number -= 1 If you always decrement the counter, then there is no need to put it in both cases. Simply put it after the 'if' block.
Created attachment 148090 [details] proposed fix Update based on comment.
View in context: https://bugs.webkit.org/attachment.cgi?id=148090&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:358 > + bug_url_number = len(re.findall(config_urls.bug_url_long, latest_entry, re.MULTILINE)) > + bug_url_number += len(re.findall(config_urls.bug_url_short, latest_entry, re.MULTILINE)) Could we use a more descriptive name? > Tools/Scripts/webkitpy/common/checkout/changelog.py:367 > + if found_bug_url and bug_url_number == 1: > + print "\n Reviewed by %s." % (reviewer.encode("utf-8")) > + if found_bug_url: > + bug_url_number -= 1 Ok, now I see what you wanted. So use something like this: if found...: if bug_num.. == 1: print ... bug_num...
Created attachment 148102 [details] proposed fix
Comment on attachment 148102 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=148102&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:358 > + bug_url_number_of_items = len(re.findall(config_urls.bug_url_long, latest_entry, re.MULTILINE)) > + bug_url_number_of_items += len(re.findall(config_urls.bug_url_short, latest_entry, re.MULTILINE)) Please add a test case when we have multiple bug URLs before you land.
(In reply to comment #19) > (From update of attachment 148102 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148102&action=review > > > Tools/Scripts/webkitpy/common/checkout/changelog.py:358 > > + bug_url_number_of_items = len(re.findall(config_urls.bug_url_long, latest_entry, re.MULTILINE)) > > + bug_url_number_of_items += len(re.findall(config_urls.bug_url_short, latest_entry, re.MULTILINE)) > > Please add a test case when we have multiple bug URLs before you land. Ugh... I meant to say, please add a test case for having multiple bug URLs.
Created attachment 148323 [details] proposed fix Added the test case.
Comment on attachment 148323 [details] proposed fix Rejecting attachment 148323 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/13006065
Comment on attachment 148323 [details] proposed fix Landed manually - http://trac.webkit.org/changeset/120821
Re-opened since this is blocked by 89605
It made some duplicated reviwer entries: - http://trac.webkit.org/changeset/120859 - http://trac.webkit.org/changeset/120863 - http://trac.webkit.org/changeset/120864 - http://trac.webkit.org/changeset/120865 We should guarantee somehow that the replace run only once. It seems it replaced the "reviewed by nobody" string first, and on the second run it inserted reviewer again, because it can't find "reviewed by nobody".
Julien told me that the problem is with webkit-patch apply-attachment + webkit-patch land combo. It is absolutely believable. apply-attachment replace "the reviewed by nobody" line and land insert one more, because it can't find. I think the new line should be added if it can't find "the reviewed by nobody" line _and_ reviewed by isn't added already.
Rolled out by http://trac.webkit.org/changeset/120867 .
"webkit-patch land" is supposed to work even on fully filed out ChangeLogs.
It appears that we shouldn't run this code in webkit-patch.
(In reply to comment #29) > It appears that we shouldn't run this code in webkit-patch. Can't we just check whether there's already a reviewer listed?
(In reply to comment #30) > (In reply to comment #29) > > It appears that we shouldn't run this code in webkit-patch. > > Can't we just check whether there's already a reviewer listed? Maybe but it's quite tricky to find out whether there's "reviewed by" line or not because people can get creative. e.g. Rubber-stamped by, etc...
The problem is the following: webkit-patch apply-attachment and apply-from-bug commands uses Tools/Script/svn-apply perl script to apply the patch and add reviewer name with its --reviewer option. And then webkit-patch land uses common/checkout/changelog.py to add reviewer's name again. It doesn't find "reviewed by nobody, oops", so it adds the reviewer line again.
Created attachment 149498 [details] Patch Fixed patch. (Balázs isn't online now, but he asked me to upload his updated patch.
Comment on attachment 149498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149498&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:352 > + (reviewer_text, reviewer_list) = ChangeLogEntry._parse_reviewer_text(latest_entry) > + > + if not found_nobody and not reviewer_text: Here is one more check to avoid adding one more "Reviewed by" line.
View in context: https://bugs.webkit.org/attachment.cgi?id=149498&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:350 > + (reviewer_text, reviewer_list) = ChangeLogEntry._parse_reviewer_text(latest_entry) It's not really nice to call a method which is marked az private. Also, the latest_entry() returns the last ChangeLogEntry and then you pass it back to the ChangeLogEntry to parse it?
Created attachment 150814 [details] proposed fix I hope it will be finally good :)
Comment on attachment 150814 [details] proposed fix Alright, let's give a shot.
Comment on attachment 150814 [details] proposed fix Clearing flags on attachment: 150814 Committed r121873: <http://trac.webkit.org/changeset/121873>
All reviewed patches have been landed. Closing bug.