Bug 115285 – [webkitpy] suggest-nominations doesn't count all qualified patches

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Glenn Adams]

The suggest-nominations command does not count reviewed patches for contributors after they become a contributor. In particular, the current code only counts those patches which contain a "Patch by" line, which doesn't hold for contributors after they become committers. As a result of this, the patch count for committers are undercounted for the purpose of nominating them as reviewer.

In addition, the command nominates reviewers that have not yet been confirmed as committers.

The proposed patch resolves these problems by:

(1) counting all qualified reviewed patches for nominations;
(2) nominating reviewers only if they are existing committers;

In addition, the proposed patch adds a number of useful options to obtain more information about analyzed commits:

--show-skipped-commits

show skipped commits except those that are explicitly marked as unreviewed

--show-skipped-unreviewed-commits

show skipped commits including those explicitly marked as unreviewed

--show-unreviewed-commits

show unmarked, unreviewed, non-gardening commits by committer/reviewer, including counts; when using this option, no nominations are output

--show-unreviewed-commits-for

used instead of --show-unreviewed-commits to show results for specific contributors

--show-unreviewed-commits-revisions

used with --show-unreviewed-commits or --show-unreviewed-commits-for in order to add list of revisions

--record-marked-unreviewed-commits

used with --show-unreviewed-commits or --show-unreviewed-commits-for in order to count marked, unreviewed, non-gardening commits which are normally not counted; a commit is marked as unreviewed by including 'Unreviewed' in the change log;

--record-gardening-unreviewed-commits

used with --record-marked-unreviewed-commits in order to additionally counted marked, unreviewed commits that include "Gardening" or "gardening";

Following is an example of using --show-unreviewed-commits (with post facto sorting of results):

webkit-patch suggest-nominations --show-unreviewed-commits
UNREVIEWED COMMITS: [R] (277) Ryosuke Niwa
UNREVIEWED COMMITS: [R] (100) Simon Fraser
UNREVIEWED COMMITS: [R] ( 95) Kent Tamura
UNREVIEWED COMMITS: [R] ( 64) Dimitri Glazkov
UNREVIEWED COMMITS: [R] ( 63) Ojan Vafai
UNREVIEWED COMMITS: [R] ( 58) Jessie Berlin
UNREVIEWED COMMITS: [R] ( 53) Geoffrey Garen
UNREVIEWED COMMITS: [R] ( 47) Anders Carlsson
UNREVIEWED COMMITS: [R] ( 45) Pavel Feldman
UNREVIEWED COMMITS: [R] ( 44) David Kilzer
UNREVIEWED COMMITS: [R] ( 43) Alexey Proskuryakov
UNREVIEWED COMMITS: [R] ( 40) Sam Weinig
UNREVIEWED COMMITS: [R] ( 36) Adam Barth
UNREVIEWED COMMITS: [R] ( 35) James Robinson
UNREVIEWED COMMITS: [R] ( 31) Oliver Hunt
UNREVIEWED COMMITS: [R] ( 25) Dan Bernstein
UNREVIEWED COMMITS: [R] ( 23) Csaba Osztrogonác
UNREVIEWED COMMITS: [R] ( 21) Martin Robinson
UNREVIEWED COMMITS: [R] ( 20) Mark Rowe
UNREVIEWED COMMITS: [R] ( 19) Beth Dakin
UNREVIEWED COMMITS: [R] ( 16) Dean Jackson
UNREVIEWED COMMITS: [R] ( 14) Andreas Kling
UNREVIEWED COMMITS: [R] ( 12) Andy Estes
UNREVIEWED COMMITS: [R] ( 10) Benjamin Poulain
UNREVIEWED COMMITS: [R] (  9) Levi Weintraub
UNREVIEWED COMMITS: [R] (  9) Daniel Bates
UNREVIEWED COMMITS: [R] (  9) Brady Eidson
UNREVIEWED COMMITS: [R] (  8) Filip Pizlo
UNREVIEWED COMMITS: [R] (  7) Simon Hausmann
UNREVIEWED COMMITS: [R] (  7) Julien Chaffraix
UNREVIEWED COMMITS: [R] (  6) Nate Chapin
UNREVIEWED COMMITS: [R] (  6) Gavin Barraclough
UNREVIEWED COMMITS: [R] (  6) Brent Fulgham
UNREVIEWED COMMITS: [R] (  5) Timothy Hatcher
UNREVIEWED COMMITS: [R] (  4) Michael Saboff
UNREVIEWED COMMITS: [R] (  4) Mark Hahnenberg
UNREVIEWED COMMITS: [R] (  4) Jer Noble
UNREVIEWED COMMITS: [R] (  4) Dirk Pranke
UNREVIEWED COMMITS: [R] (  4) Darin Adler
UNREVIEWED COMMITS: [R] (  3) Chris Fleizach
UNREVIEWED COMMITS: [R] (  2) Yury Semikhatsky
UNREVIEWED COMMITS: [R] (  2) Xan Lopez
UNREVIEWED COMMITS: [R] (  2) Tim Horton
UNREVIEWED COMMITS: [R] (  2) Kentaro Hara
UNREVIEWED COMMITS: [R] (  2) Dmitry Titov
UNREVIEWED COMMITS: [R] (  2) David Hyatt
UNREVIEWED COMMITS: [R] (  2) Antti Koivisto
UNREVIEWED COMMITS: [R] (  1) Zoltan Herczeg
UNREVIEWED COMMITS: [R] (  1) Philip Rogers
UNREVIEWED COMMITS: [R] (  1) Gustavo Noronha Silva
UNREVIEWED COMMITS: [R] (  1) Eric Seidel
UNREVIEWED COMMITS: [R] (  1) Brian Weinstein
UNREVIEWED COMMITS: [R] (  1) Antonio Gomes
UNREVIEWED COMMITS: [C] (102) Raphael Kubo da Costa
UNREVIEWED COMMITS: [C] ( 47) Patrick Gansterer
UNREVIEWED COMMITS: [C] ( 44) Roger Fong
UNREVIEWED COMMITS: [C] ( 35) Lucas Forschler
UNREVIEWED COMMITS: [C] ( 22) Jon Lee
UNREVIEWED COMMITS: [C] ( 15) Mark Lam
UNREVIEWED COMMITS: [C] ( 10) Hin-Chung Lam
UNREVIEWED COMMITS: [C] (  6) Yoshifumi Inoue
UNREVIEWED COMMITS: [C] (  6) Fady Samuel
UNREVIEWED COMMITS: [C] (  6) Adam Kallai
UNREVIEWED COMMITS: [C] (  5) Žan Doberšek
UNREVIEWED COMMITS: [C] (  5) Pavel Podivilov
UNREVIEWED COMMITS: [C] (  4) Balazs Kelemen
UNREVIEWED COMMITS: [C] (  3) Peter Kasting
UNREVIEWED COMMITS: [C] (  3) Ilya Tikhonovsky
UNREVIEWED COMMITS: [C] (  3) Chris Evans
UNREVIEWED COMMITS: [C] (  2) Takashi Sakamoto
UNREVIEWED COMMITS: [C] (  2) Pierre Rossi
UNREVIEWED COMMITS: [C] (  2) Kenichi Ishibashi
UNREVIEWED COMMITS: [C] (  2) Karen Grunberg
UNREVIEWED COMMITS: [C] (  2) Antoine Quint
UNREVIEWED COMMITS: [C] (  2) Andrey Adaykin
UNREVIEWED COMMITS: [C] (  2) Adam Klein
UNREVIEWED COMMITS: [C] (  1) Xiaohai Wei
UNREVIEWED COMMITS: [C] (  1) Xianzhu Wang
UNREVIEWED COMMITS: [C] (  1) W. James MacLean
UNREVIEWED COMMITS: [C] (  1) Vincent Scheib
UNREVIEWED COMMITS: [C] (  1) Terry Anderson
UNREVIEWED COMMITS: [C] (  1) Silvia Pfeiffer
UNREVIEWED COMMITS: [C] (  1) Sadrul Habib Chowdhury
UNREVIEWED COMMITS: [C] (  1) Raymond Toy
UNREVIEWED COMMITS: [C] (  1) Rafael Weinstein
UNREVIEWED COMMITS: [C] (  1) Mikhail Pozdnyakov
UNREVIEWED COMMITS: [C] (  1) Mike Lawther
UNREVIEWED COMMITS: [C] (  1) Michael Nordman
UNREVIEWED COMMITS: [C] (  1) Mark Pilgrim
UNREVIEWED COMMITS: [C] (  1) Luciano Wolf
UNREVIEWED COMMITS: [C] (  1) Leo Yang
UNREVIEWED COMMITS: [C] (  1) Lauro Neto
UNREVIEWED COMMITS: [C] (  1) Jesus Sanchez-Palencia
UNREVIEWED COMMITS: [C] (  1) Jeffrey Pfau
UNREVIEWED COMMITS: [C] (  1) Hayato Ito
UNREVIEWED COMMITS: [C] (  1) Dominic Mazzoni
UNREVIEWED COMMITS: [C] (  1) Dmitry Gorbik
UNREVIEWED COMMITS: [C] (  1) Dave Barton
UNREVIEWED COMMITS: [C] (  1) Dana Jansens
UNREVIEWED COMMITS: [C] (  1) Andras Becsi
UNREVIEWED COMMITS: Total 1660

[Glenn Adams]

Created attachment 199870 [details]
Patch

[Glenn Adams]

(In reply to comment #1)
> Created an attachment (id=199870) [details]
> Patch

I should also mention that this patch divides the prior functionality of SuggestNominations into AbstractSuggestCommand and SuggestNominations. This was done in order to prepare for the addition of a SuggestEmeriti class in order to add a suggest-emeriti command in a separate, upcoming patch, where both of these commands reuse the functionality in the base AbstractSuggestCommand.

[Glenn Adams]

Created attachment 199918 [details]
Patch

[Glenn Adams]

(In reply to comment #3)
> Created an attachment (id=199918) [details]
> Patch

Defer addition of support for additional commit analysis output, which is better targeted to a new command 'analyze-commits'. Once the present patch is landed (mutatis mutants) I will open a new bug with a separate patch to add such a command.

This allows the current patch to be smaller and focus on abstracting base functionality into the new AbstractCommitLogCommand class and make the bug fixes noted in this bug.

[Glenn Adams]

(In reply to comment #0)
> The suggest-nominations command does not count reviewed patches for contributors after they become a contributor.

s/after they become a contributor/after they become a committer/

[Glenn Adams]

Created attachment 199960 [details]
Remove duplication changelog entry.

[Ryosuke Niwa]

Comment on attachment 199960 [details]
Remove duplication changelog entry.

Too many changes at once. Please don't include the changes to add AbstractCommitLogCommand in this patch.

[Glenn Adams]

Created attachment 200036 [details]
Patch

[Glenn Adams]

(In reply to comment #8)
> Created an attachment (id=200036) [details]
> Patch

Removed AbstractCommitLogCommand and simplified changes as requested by rniwa.

[Ryosuke Niwa]

Comment on attachment 200036 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:48
> +    _match_leading_indent = re.compile(r"^[ ]{4}", re.MULTILINE)
> +    _match_reviewed_by = re.compile(r'^\s*((\w+\s+)+and\s+)?(Review|Rubber(\s*|-)stamp)(s|ed)?\s+([a-z]+\s+)*?by\s+(?P<reviewers>.*?)[\.,]?\s*$', re.MULTILINE)
> +    _match_patch_by = re.compile(r'^Patch by (?P<name>.+?)\s+<(?P<email>[^<>]+)> on (?P<date>\d{4}-\d{2}-\d{2})$', re.MULTILINE)
> +    _match_committer = re.compile(r'^Author: (?P<email>\S+)\s+<[^>]+>$', re.MULTILINE)
> +    _match_date = re.compile(r'^Date:   \S{3} (?P<month>\S{3}) (?P<day>\d{1,2}) (?P<time>\d{2}:\d{2}:\d{2}) (?P<year>\d{4}) [\+\-]\d{4}$', re.MULTILINE)
> +    _match_revision = re.compile(r'^git-svn-id: http://svn.webkit.org/repository/webkit/trunk@(?P<svnid>\d+) (?P<gitid>[0-9a-f\-]{36})$', re.MULTILINE)

Our naming convection is to postfix these names with _regex instead of prefixing them with match since they're not match objects.
Looks like there's a lot of code duplication with ChangeLogEntry at http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/changelog.py#L60

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:96
> +    def _recent_commits(self):

Looks like much of code in this function can be substituted by some combinations of method calls in ChangeLogEntry if we generalize the class well enough.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:175
> +        for commit in self._recent_commits():

Looks like it's completely unnecessary for _recent_commits to yield. This function can iterate over self._recent_commit_messages()
and call a function that analyzes the commit and returns None when it needs to be skipped.

[Glenn Adams]

Created attachment 200055 [details]
Patch

[Glenn Adams]

(In reply to comment #10)
> (From update of attachment 200036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200036&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:48
> > +    _match_leading_indent = re.compile(r"^[ ]{4}", re.MULTILINE)
> > +    _match_reviewed_by = re.compile(r'^\s*((\w+\s+)+and\s+)?(Review|Rubber(\s*|-)stamp)(s|ed)?\s+([a-z]+\s+)*?by\s+(?P<reviewers>.*?)[\.,]?\s*$', re.MULTILINE)
> > +    _match_patch_by = re.compile(r'^Patch by (?P<name>.+?)\s+<(?P<email>[^<>]+)> on (?P<date>\d{4}-\d{2}-\d{2})$', re.MULTILINE)
> > +    _match_committer = re.compile(r'^Author: (?P<email>\S+)\s+<[^>]+>$', re.MULTILINE)
> > +    _match_date = re.compile(r'^Date:   \S{3} (?P<month>\S{3}) (?P<day>\d{1,2}) (?P<time>\d{2}:\d{2}:\d{2}) (?P<year>\d{4}) [\+\-]\d{4}$', re.MULTILINE)
> > +    _match_revision = re.compile(r'^git-svn-id: http://svn.webkit.org/repository/webkit/trunk@(?P<svnid>\d+) (?P<gitid>[0-9a-f\-]{36})$', re.MULTILINE)
> 
> Our naming convection is to postfix these names with _regex instead of prefixing them with match since they're not match objects.

Change to _regexp suffix, such as used in changelogentry.py.

> Looks like there's a lot of code duplication with ChangeLogEntry at http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/changelog.py#L60

Turns out to be very little overlap. In particular, only reuse is ChangeLogEntry.reviewed_by_regexp, which I've reused. The reason there is little overlap is because:

(1) commit log doesn't contain change log date line, but uses git log's 'Date: ' line;
(2) determination of author uses different algorithm, i.e., uses 'Patch by' line (from change log body) first then uses 'Author: ' git log line second

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:96
> > +    def _recent_commits(self):
> 
> Looks like much of code in this function can be substituted by some combinations of method calls in ChangeLogEntry if we generalize the class well enough.

Not much. See above. Change log and commit log analysis use different algorithms and information to determine committer, author, and date.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:175
> > +        for commit in self._recent_commits():
> 
> Looks like it's completely unnecessary for _recent_commits to yield. This function can iterate over self._recent_commit_messages()
> and call a function that analyzes the commit and returns None when it needs to be skipped.

I swapped this out with iteration over messages invoking an analyzer callback that filters entries and extracts state as needed.

[Ryosuke Niwa]

Comment on attachment 200055 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:99
> +        # Determine committer if possible, otherwise skip commit.

We should share code with ChangeLogEntry as I mentioned earlier. r-.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:160
> +        on_analysis(analysis, committer, commit_date, revision, author_email, author_name, contributor, reviewers)

Why do we need a callback function?

[Glenn Adams]

(In reply to comment #13)
> (From update of attachment 200055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200055&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:99
> > +        # Determine committer if possible, otherwise skip commit.
> 
> We should share code with ChangeLogEntry as I mentioned earlier. r-.

I explained in comment #12 below why I have shared all that can be reasonably shared. Change log and commit log entries use different data sets, and different semantics for what constitutes author.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:160
> > +        on_analysis(analysis, committer, commit_date, revision, author_email, author_name, contributor, reviewers)
> 
> Why do we need a callback function?

How would your propose to write this logic then? It worked fine as a generator,  and you asked it to be change to "call a function that analyzes the commit".

[Ryosuke Niwa]

Comment on attachment 200055 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:166
> +    def _analyze_commits(self, on_analysis, analysis, analyze=None):
> +        if not analyze:
> +            analyze = self._analyze_commit
> +        for commit_message in self._recent_commit_messages():
> +            analyze(commit_message, on_analysis, analysis)

This should be merged into the caller of _analyze_commits. This function doesn't do anything useful and only adds undesirable indirections.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:213
> +        self._analyze_commits(_on_analysis, analysis)

This is the sole caller of _analyze_commits. We shouldn't need to make it take two callbacks.
We don't any more premature/over generalization in webkitpy. It's bad as is.

[Glenn Adams]

(In reply to comment #15)
> (From update of attachment 200055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200055&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:166
> > +    def _analyze_commits(self, on_analysis, analysis, analyze=None):
> > +        if not analyze:
> > +            analyze = self._analyze_commit
> > +        for commit_message in self._recent_commit_messages():
> > +            analyze(commit_message, on_analysis, analysis)
> 
> This should be merged into the caller of _analyze_commits. This function doesn't do anything useful and only adds undesirable indirections.
> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:213
> > +        self._analyze_commits(_on_analysis, analysis)
> 
> This is the sole caller of _analyze_commits. We shouldn't need to make it take two callbacks.
> We don't any more premature/over generalization in webkitpy. It's bad as is.

I disagree. There is a perfectly good reason to do this. At your request, I have already (temporarily) backed out the addition of an abstract commit log command base class, but there is good cause to begin to generalize the analysis process to support [1][2][3]:

[1] https://bugs.webkit.org/show_bug.cgi?id=115387
[2] https://bugs.webkit.org/show_bug.cgi?id=115388
[3] https://bugs.webkit.org/show_bug.cgi?id=115391

[Glenn Adams]

Created attachment 200065 [details]
Patch

[Benjamin Poulain]

Comment on attachment 200065 [details]
Patch

I agree with Ryosuke here, I don't get _analyze_commits. Especially the last, never used, argument.

If you need the function for future change, just add it with those changes.

I would also modify contributors.json separately from this patch.

> > This is the sole caller of _analyze_commits. We shouldn't need to make it take two callbacks.
> > We don't any more premature/over generalization in webkitpy. It's bad as is.
> 
> I disagree. There is a perfectly good reason to do this.

What is the good reason?
This is the kind of information best detailed in the ChangeLog.

[Glenn Adams]

Created attachment 200068 [details]
Patch

[Glenn Adams]

(In reply to comment #18)
> (From update of attachment 200065 [details])
> I agree with Ryosuke here, I don't get _analyze_commits. Especially the last, never used, argument.
> 
> If you need the function for future change, just add it with those changes.
> 
> I would also modify contributors.json separately from this patch.

Done.

> 
> > > This is the sole caller of _analyze_commits. We shouldn't need to make it take two callbacks.
> > > We don't any more premature/over generalization in webkitpy. It's bad as is.
> > 
> > I disagree. There is a perfectly good reason to do this.
> 
> What is the good reason?
> This is the kind of information best detailed in the ChangeLog.

Removed _analyze_commits, recoding inline.

[Benjamin Poulain]

Comment on attachment 200068 [details]
Patch

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

Comments bellow.

Personally I think it is okay to parse commit messages differently than ChangeLog. But realistically Ryosuke is the maintainer of webkitpy so better get his input too.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:91
> +    def _commit_date_from_match(self, match):
> +        year = match.group('year')
> +        month_abbr = match.group('month')
> +        month = 0
> +        for i in range(len(calendar.month_abbr)):
> +            if calendar.month_abbr[i] == month_abbr:
> +                month = i + 1
> +                break
> +        day = match.group('day')
> +        return '%04d-%02d-%02d' % (int(year), month, int(day))

Instead, I could just have git give the nicely formatted date.
Look for --pretty in the documentation.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:97
> +    def _analyze_commit(self, commit_message, on_analysis, analysis):

The code of this function is made extremely fragile by all the
   if not match
       return

Maybe it should instead raise an exception when it fails parsing?
The caller code can then catch the exceptions and notify the user of how many commits were skipped. It would not prevent the code from breaking, but at least the user of the command would know it is broken.

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

Empty line.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:107
> +        # Determine committer if possible, otherwise skip commit.
> +        committer_match = self._committer_regexp.search(commit_message)
> +        if not committer_match:
> +            return
> +
> +        # Determine committer email if possible, otherwise skip commit.
> +        committer_email = committer_match.group('email')
> +        if not committer_email:
> +            return

This looks like the common pattern. Maybe make a function out of it so that each of those 4 lines group become just one line.

The comments are not adding information here.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:109
> +        # Determine if committer is a known contributor, i.e., registered in committers.py, otherwise skip commit.

This sounds like a valid case for failing, no?

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:154
> +        # Determine reviewers if possible, otherwise optionally skip commit.

optionally?

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:209
> +        for commit_message in self._recent_commit_messages():
> +            self._analyze_commit(commit_message, _on_analysis, analysis)

Why does this with the callback _on_analysis?

What about renaming _analyze_commit to parse_commit. It returns a simple object that you pass to a _add_to_contribution_histogram or something?

[Glenn Adams]

Created attachment 200079 [details]
Patch

[Glenn Adams]

(In reply to comment #21)
> (From update of attachment 200068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200068&action=review
> 
> Comments bellow.
> 
> Personally I think it is okay to parse commit messages differently than ChangeLog. But realistically Ryosuke is the maintainer of webkitpy so better get his input too.

I already discussed with Ryosuke. If he has any specific suggested changes, I'd be happy to apply them. I'm not really changing anything in this patch with respect to how the commit log data is parsed. It was already different than the change log parser before this patch, and nothing is being changed here in that respect.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:91
> > +    def _commit_date_from_match(self, match):
> > +        year = match.group('year')
> > +        month_abbr = match.group('month')
> > +        month = 0
> > +        for i in range(len(calendar.month_abbr)):
> > +            if calendar.month_abbr[i] == month_abbr:
> > +                month = i + 1
> > +                break
> > +        day = match.group('day')
> > +        return '%04d-%02d-%02d' % (int(year), month, int(day))
> 
> Instead, I could just have git give the nicely formatted date.
> Look for --pretty in the documentation.

Fixed. Use --date=iso option on git log, replace regex.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:97
> > +    def _analyze_commit(self, commit_message, on_analysis, analysis):
> 
> The code of this function is made extremely fragile by all the
>    if not match
>        return

Well, no more fragile than the existing code, which used if not match continue. In any case, I've fixed by adding exceptions as you suggest below.

> 
> Maybe it should instead raise an exception when it fails parsing?
> The caller code can then catch the exceptions and notify the user of how many commits were skipped. It would not prevent the code from breaking, but at least the user of the command would know it is broken.
> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:98
> > +
> 
> Empty line.

Fixed.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:107
> > +        # Determine committer if possible, otherwise skip commit.
> > +        committer_match = self._committer_regexp.search(commit_message)
> > +        if not committer_match:
> > +            return
> > +
> > +        # Determine committer email if possible, otherwise skip commit.
> > +        committer_email = committer_match.group('email')
> > +        if not committer_email:
> > +            return
> 
> This looks like the common pattern. Maybe make a function out of it so that each of those 4 lines group become just one line.

Doing this makes the code harder to read by my estimation and doesn't really buy much. I'd prefer not to do this unless you insist.

> 
> The comments are not adding information here.
> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:109
> > +        # Determine if committer is a known contributor, i.e., registered in committers.py, otherwise skip commit.
> 
> This sounds like a valid case for failing, no?

Reordered the code a bit, but contributor == None isn't an error condition, it merely means the author (as previously determined) does not yet have an entry in the contributors list.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:154
> > +        # Determine reviewers if possible, otherwise optionally skip commit.
> 
> optionally?

Now throws CommitLogMissingReviewer.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:209
> > +        for commit_message in self._recent_commit_messages():
> > +            self._analyze_commit(commit_message, _on_analysis, analysis)
> 
> Why does this with the callback _on_analysis?
> 
> What about renaming _analyze_commit to parse_commit. It returns a simple object that you pass to a _add_to_contribution_histogram or something?

You guys are killing me. That's what I did to start with, via a generator and yield, but Ryosuke asked to change to a callback. Can we keep it as is? I'm going to be tweaking this very soon anyway in order to address [1].

[1] https://bugs.webkit.org/show_bug.cgi?id=115391

[Benjamin Poulain]

Comment on attachment 200079 [details]
Patch

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

I would really prefer the callback approach to be cleaned but I am too tired to fight.

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

Blank line.

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

Blank line.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:53
> -    help_text = "Suggest contributors for committer/reviewer nominations"
> +    help_text = "Suggest nominations for committer/reviewer role"

I think the previous text is better.

The patch count is only a lower limit. You can have thousands of patches and not be nominated.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:126
> +        if not author_match:
> +            raise CommitLogError

How could this happen?
We could not get here if committer_match is None.

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:132
> +        if not author_email:
> +            raise CommitLogError

ditto

[Glenn Adams]

Created attachment 200196 [details]
Patch for landing

[Glenn Adams]

(In reply to comment #24)
> (From update of attachment 200079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200079&action=review
> 
> I would really prefer the callback approach to be cleaned but I am too tired to fight.

I went ahead and recoded this as you suggested. See _parse_commit_message and _count_commit.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:40
> > +
> 
> Blank line.

Fixed.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:46
> > +
> 
> Blank line.

Fixed.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:53
> > -    help_text = "Suggest contributors for committer/reviewer nominations"
> > +    help_text = "Suggest nominations for committer/reviewer role"
> 
> I think the previous text is better.

Fixed.

> 
> The patch count is only a lower limit. You can have thousands of patches and not be nominated.
> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:126
> > +        if not author_match:
> > +            raise CommitLogError
> 
> How could this happen?
> We could not get here if committer_match is None.

Fixed.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:132
> > +        if not author_email:
> > +            raise CommitLogError
> 
> ditto

Fixed.

[WebKit Commit Bot]

Comment on attachment 200196 [details]
Patch for landing

Clearing flags on attachment: 200196

Committed r149419: <http://trac.webkit.org/changeset/149419>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Comment on attachment 200196 [details]
Patch for landing

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

> Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:81
> +        messages = re.split(r"^commit \w{40}$", git_log, flags=re.MULTILINE)[1:]  # Ignore the first message which will be empty.

Can't use this. Only Python 2.7 supports flags argument, and we still need to support Python 2.6 on Windows.
Fixed it in http://trac.webkit.org/changeset/150461.