Bug 62166 – Add a suggest-nominations command to webkit-patch for computing potential committer/reviewer nominations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Add a suggest-nominations command to webkit-patch for computing potential committer/reviewer nominations

[Eric Seidel (no email)]

Created attachment 96155 [details]
Patch

[Eric Seidel (no email)]

I wrote this while I was waiting for my fastMalloc(0) patch to finally run through all the layout tests this morning.  Mostly because I had the surprising realization the other day that Noel Gordon was not yet a committer.

[Eric Seidel (no email)]

Created attachment 96156 [details]
sorted the includes

[Eric Seidel (no email)]

Here is some example output (ignoring all the Warnings it currently prints due to missing email addresses in committers.py):

% webkit-patch suggest-nominations
REVIEWER: Patrick Gansterer ([e-mail]) has 224 reviewed patches
REVIEWER: Pavel Podivilov ([e-mail]) has 181 reviewed patches
REVIEWER: Alexander Pavlov ([e-mail]) has 108 reviewed patches
REVIEWER: Chris Rogers ([e-mail]) has 107 reviewed patches
REVIEWER: Carlos Garcia Campos ([e-mail]) has 101 reviewed patches
REVIEWER: Jer Noble ([e-mail]) has 100 reviewed patches
REVIEWER: Ilya Tikhonovsky ([e-mail]) has 88 reviewed patches
REVIEWER: Philippe Normand ([e-mail]) has 86 reviewed patches
REVIEWER: Abhishek Arya ([e-mail]) has 82 reviewed patches
COMMITTER: Andrey Adaikin ([e-mail]) has 40 reviewed patches
COMMITTER: Ryuan Choi ([e-mail]) has 36 reviewed patches
COMMITTER: Vsevolod Vlasov ([e-mail]) has 36 reviewed patches
COMMITTER: Luke Macpherson ([e-mail]) has 35 reviewed patches
COMMITTER: Mark Pilgrim ([e-mail]) has 32 reviewed patches
COMMITTER: Ilya Sherman ([e-mail]) has 25 reviewed patches
COMMITTER: Siddharth Mathur ([e-mail]) has 24 reviewed patches
COMMITTER: Mike Reed ([e-mail]) has 23 reviewed patches
COMMITTER: Naoki Takano ([e-mail]) has 23 reviewed patches
COMMITTER: Chris Guillory ([e-mail]) has 21 reviewed patches
COMMITTER: Nat Duca ([e-mail]) has 21 reviewed patches
COMMITTER: Noel Gordon ([e-mail]) has 21 reviewed patches

[Adam Barth]

Comment on attachment 96156 [details]
sorted the includes

I can haz tests?

[Eric Seidel (no email)]

Sure, what would you like tested? :)

[Adam Barth]

Comment on attachment 96156 [details]
sorted the includes

I would just stub out git_log = self._tool.executive.run_command(['git', 'log', '--since="9 months ago"']) to produce interesting output that you can then see what your code prints.

[Eric Seidel (no email)]

This has some relation to bug 26533, but I didn't re-use any of that code (because it was never landed into webkitpy).

[Siddharth Mathur]

(In reply to comment #4)

<joke>Gee, it's great to see a tool flag my name for *appreciation*. Wish life had more such tools :) </joke>

[Eric Seidel (no email)]

Created attachment 107553 [details]
Patch

[Adam Barth]

Comment on attachment 107553 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:61
> +        git_log = self._tool.executive.run_command(['git', 'log', '--since="9 months ago"'])
> +        match_git_svn_id = re.compile(r"\n\n    git-svn-id:.*\n", re.MULTILINE)
> +        match_get_log_lines = re.compile(r"^\S.*\n", re.MULTILINE)
> +        match_leading_indent = re.compile(r"^[ ]{4}", re.MULTILINE)

This only works on git?  Should we mention that in the help?

[Adam Barth]

Sorry, don't have time to review completely right now.

[Eric Seidel (no email)]

I'm in no rush.  :)

[Tom Zakrajsek]

I'm not sure if it's a big deal, but this patch chokes if you redirect the output (at least on linux).  Specifically when it is printing unicode names.

$ webkit-patch suggest-nominations > temp.txt
Traceback (most recent call last):
  File "/home/tomz/WebKit/Tools/Scripts/webkit-patch", line 66, in <module> main()
  File "/home/tomz/WebKit/Tools/Scripts/webkit-patch", line 61, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/home/tomz/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 308, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/home/tomz/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 117, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/home/tomz/WebKit/Tools/Scripts/webkitpy/tool/commands/suggestnominations.py", line 145, in execute
    self._print_nominations(nominations)
  File "/home/tomz/WebKit/Tools/Scripts/webkitpy/tool/commands/suggestnominations.py", line 140, in _print_nominations
    print "%(roles_string)s: %(author_name)s (%(author_email)s) has %(patch_count)s reviewed patches" % nomination

UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 16: ordinal not in range(128)

When I don't redirect, I can see that the name it choked on was:
COMMITTER: Kristóf Kosztyó ([e-mail]) has 19 reviewed patches
                ^

[Tom Zakrajsek]

You could append ".encode('ascii', 'replace')" to author_name at suggestnominations.py@121. 

121:   'author_name': author_name.encode('ascii', 'replace'),

The downside is that those characters become '?' even when not using redirection.  Still seems like a better idea than a potential exception.

[Eric Seidel (no email)]

We have this general problem with stdin/stdout encoding on systems with LANG=ascii set.  This is the same root problem as bug 63452.

[Tom Zakrajsek]

There are some people that have contributed using different emails.  Right now, this script doesn't take the union of those.  One fix would be to count patches by Name.

[Tom Zakrajsek]

Created attachment 110769 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 110769 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:58
> +        options = [
> +            steps.Options.committer_minimum,
> +            steps.Options.reviewer_minimum,
> +            steps.Options.max_commit_age,
> +            steps.Options.show_commits,
> +        ]
> +
> +        AbstractDeclarativeCommand.__init__(self, options=options)

I'm confused why we need to do this dance?

[Tom Zakrajsek]

(In reply to comment #19)
> (From update of attachment 110769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110769&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:58
> > +        options = [
> > +            steps.Options.committer_minimum,
> > +            steps.Options.reviewer_minimum,
> > +            steps.Options.max_commit_age,
> > +            steps.Options.show_commits,
> > +        ]
> > +
> > +        AbstractDeclarativeCommand.__init__(self, options=options)
> 
> I'm confused why we need to do this dance?

Assuming you mean the __init__() call, this was in your first draft, and without it bind_to_tool fails with "AttributeError: 'SuggestNominations' object has no attribute '_tool'".  The original version was supporting an option "git_commit" which I didn't see a reason for though.

[Tom Zakrajsek]

Created attachment 110850 [details]
Patch

[Tom Zakrajsek]

Adam Barth thought the issue in #19 was about the options setup, and recommended pointing to another example of this pattern.  See Tools/Scripts/webkitpy/tool/commands/upload.py: PostCommits, MarkBugFixed, CreateBug, ....

[Eric Seidel (no email)]

Comment on attachment 110850 [details]
Patch

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

One more round please. :)

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:48
> +        self.committer_minimum = steps.Options.committer_minimum.default
> +        self.reviewer_minimum = steps.Options.reviewer_minimum.default
> +        self.max_commit_age = steps.Options.max_commit_age.default
> +        self.show_commits = steps.Options.show_commits.default

Why do you need to pull defaults here?

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:55
> +            steps.Options.committer_minimum,
> +            steps.Options.reviewer_minimum,
> +            steps.Options.max_commit_age,
> +            steps.Options.show_commits,

Yeah, since you're not using steps, you could just do the make_option here, isntead of using the shared steps options.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:60
> +        # FIXME: This should probably be on the tool somewhere.
> +        self._committer_list = CommitterList()

Isn't it already on the tool?  Maybe it isn't... but it certainly should be. :)  (Not that you have to fix that.)

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:162
> +        for author_email, curr_counter in counters_by_email.items():

curr_ is a horrible name.  just call it "counter" or "counter_for_author" or whatever it is.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:163
> +            if author_email != curr_counter['mru_email']:

what's mru?  most recently used?  maybe latest_email?  latest_name?

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:199
> +            curr_counter = self._counters_by_email[nomination['author_email']]

yeah, just call it counter. :)

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:209
> +            # split the tuples
> +            # the second element is the "counter" structure
> +            ia, a = a_tuple
> +            ib, b = b_tuple

You want somethign like:

_, a_counter = a_tuple
_, b_counter = b_tuple

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:225
> +            aka_list = []

I might have called this alais_list.  aka is OK, but might be more confusing to non-native english speakers.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:230
> +            if len(aka_list):

You can just do if aka_list here.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:248
> +

two lines.

> Tools/Scripts/webkitpy/tool/steps/options.py:41
> +    committer_minimum = make_option("--committer-minimum", action="store", dest="committer_minimum", type="int", default=10, help="Specify minimum patch count for Committer nominations.")

I wouldn't put these in steps/options unless they're used by a step.  Just inline them into your command.

[Tom Zakrajsek]

Awesome feedback.  Thanks.  I'm working on fixing.  I'm having trouble annotating your review comments though.  Specifically:

Why do you need to pull defaults here?
> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:55
> +            steps.Options.committer_minimum,

it's because one of the unit tests is bypassing the "exec" function.  It just creates the object and calls _recent_commit_messages to test.  I could restructure that test.  I just thought that initializing these here made sense in the spirit of don't create an object in an invalid state.  They are "overridden" if exec is passed new values.  I'll see if I can come up with a better way.

[Eric Seidel (no email)]

Split out the "setup this object from options" into a separate method and call it maually from both execute and your unittest.

[Tom Zakrajsek]

Created attachment 111021 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 111021 [details]
Patch

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

I'm willing to rubber-stamp this, so we can use it and evaluate the utility. Seems like a huge amount of code for this.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:78
> +            (message, _) = match_git_svn_id.subn("", message)
> +            (message, _) = match_get_log_lines.subn("", message)
> +            (message, _) = match_leading_indent.subn("", message)

Why subn and not sub?  Seems you don't want the number of subs anyway. :)

[WebKit Review Bot]

Comment on attachment 111021 [details]
Patch

Clearing flags on attachment: 111021

Committed r97891: <http://trac.webkit.org/changeset/97891>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.