Bug 67935 – webkit-patch should add reviewer if "Reviewed by NOBODY (OOPS!)." is missing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

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.

[Balazs Ankes]

Created attachment 141425 [details]
proposed fix

This modification do the same just send a boolean value if the reviewer was set and handle it.

[Rafael Brandao]

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.

[Adam Barth]

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() ?

[Adam Barth]

Likely Eric should review this change.

[Eric Seidel (no email)]

Comment on attachment 141425 [details]
proposed fix

The change does not appear to do what it claims to in the bug title. :)

[Balazs Ankes]

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

[Adam Barth]

> 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?

[Balazs Ankes]

Created attachment 142238 [details]
proposed fix

Now it does what the bug title says.

[Eric Seidel (no email)]

Comment on attachment 142238 [details]
proposed fix

This needs tests. :(

[Balazs Ankes]

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.

[Balazs Ankes]

Created attachment 147564 [details]
proposed fix

I updated the patch. Could somebody check it, please?

[Ryosuke Niwa]

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.

[Peter Gal]

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)

[Balazs Ankes]

Created attachment 147839 [details]
proposed fix

Improved patch based on comments.

[Peter Gal]

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.

[Balazs Ankes]

Created attachment 148090 [details]
proposed fix

Update based on comment.

[Peter Gal]

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...

[Balazs Ankes]

Created attachment 148102 [details]
proposed fix

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Balazs Ankes]

Created attachment 148323 [details]
proposed fix

Added the test case.

[WebKit Review Bot]

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

[Csaba Osztrogonác]

Comment on attachment 148323 [details]
proposed fix

Landed manually - http://trac.webkit.org/changeset/120821

[WebKit Review Bot]

Re-opened since this is blocked by 89605

[Csaba Osztrogonác]

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".

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Rolled out by http://trac.webkit.org/changeset/120867 .

[Adam Barth]

"webkit-patch land" is supposed to work even on fully filed out ChangeLogs.

[Ryosuke Niwa]

It appears that we shouldn't run this code in webkit-patch.

[Adam Barth]

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

[Ryosuke Niwa]

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

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 149498 [details]
Patch

Fixed patch. (Balázs isn't online now, but he asked me to upload his updated patch.

[Csaba Osztrogonác]

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.

[Peter Gal]

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?

[Balazs Ankes]

Created attachment 150814 [details]
proposed fix

I hope it will be finally good :)

[Ryosuke Niwa]

Comment on attachment 150814 [details]
proposed fix

Alright, let's give a shot.

[WebKit Review Bot]

Comment on attachment 150814 [details]
proposed fix

Clearing flags on attachment: 150814

Committed r121873: <http://trac.webkit.org/changeset/121873>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.