Bug 182584 - webkit-patch suggest-reviewers dies with AttributeError: 'NoneType' object has no attribute 'revision'
Summary: webkit-patch suggest-reviewers dies with AttributeError: 'NoneType' object ha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-07 15:01 PST by Daniel Bates
Modified: 2018-02-08 17:29 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.16 KB, patch)
2018-02-07 17:09 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (3.11 KB, patch)
2018-02-08 08:36 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (3.46 KB, patch)
2018-02-08 11:01 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-02-07 15:01:26 PST
When I ran webkit-patch suggest-reviewers today it died with the following stack trace:

Traceback (most recent call last):
  File "Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/.../OpenSource/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/.../OpenSource/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/.../OpenSource/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/.../OpenSource/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/.../OpenSource/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/.../OpenSource/Tools/Scripts/webkitpy/tool/steps/suggestreviewers.py", line 45, in run
    reviewers = self._tool.checkout().suggested_reviewers(self._options.git_commit, self._changed_files(state))[:5]
  File "/Volumes/.../OpenSource/Tools/Scripts/webkitpy/common/checkout/checkout.py", line 144, in suggested_reviewers
    commit_infos = sorted(self.recent_commit_infos_for_files(changed_files), key=lambda info: info.revision(), reverse=True)
  File "/Volumes/.../OpenSource/Tools/Scripts/webkitpy/common/checkout/checkout.py", line 144, in <lambda>
    commit_infos = sorted(self.recent_commit_infos_for_files(changed_files), key=lambda info: info.revision(), reverse=True)
AttributeError: 'NoneType' object has no attribute 'revision'

I am using a Git checkout of WebKit at r228243 + attachment #333316 [details]

Steps to reproduce:

1. Apply attachment #333316 [details] to your WebKit working copy and commit it (i.e. run "git commit").
2. Run `Tools/Scripts/webkit-patch suggest-reviewers -g HEAD`

Then webkit-patch will die with the aforementioned stack trace.
Comment 1 Jonathan Bedard 2018-02-07 17:09:29 PST
Created attachment 333340 [details]
Patch
Comment 2 Jonathan Bedard 2018-02-07 17:11:21 PST
One of the revisions does not have a ChangeLog associated with it. Seems like a pretty straight-forward fix, given that commit_info_for_revision can return 'None'.
Comment 3 Daniel Bates 2018-02-07 20:31:09 PST
Comment on attachment 333340 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/checkout.py:141
> +        if None in result:

Yuck. Is there no better way?
Comment 4 Daniel Bates 2018-02-07 20:34:13 PST
Comment on attachment 333340 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:431
> +                return [-1]  # Should return None for revison information

Should? When does it not return None?

revision => revision

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:436
> +        checkout._scm.changed_files = lambda git_commit: ["file1", "file2", "relative/path/ChangeLog", 'bad_file']

non-existent-file?
Comment 5 Daniel Bates 2018-02-07 20:35:38 PST
(In reply to Daniel Bates from comment #4)
> Comment on attachment 333340 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333340&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:431
> > +                return [-1]  # Should return None for revison information
> 
> Should? When does it not return None?
> 
> revision => revision
> 

revison => revision
Comment 6 Daniel Bates 2018-02-07 20:36:52 PST
Comment on attachment 333340 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/checkout/checkout.py:141
>> +        if None in result:
> 
> Yuck. Is there no better way?

Maybe we need a comment here as well.
Comment 7 Jonathan Bedard 2018-02-08 08:32:28 PST
Comment on attachment 333340 [details]
Patch

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

>>> Tools/Scripts/webkitpy/common/checkout/checkout.py:141
>>> +        if None in result:
>> 
>> Yuck. Is there no better way?
> 
> Maybe we need a comment here as well.

We could subtract set([None]), that looks a bit cleaner.

>> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:436
>> +        checkout._scm.changed_files = lambda git_commit: ["file1", "file2", "relative/path/ChangeLog", 'bad_file']
> 
> non-existent-file?

The file does exist, it's recent revision just does not have changelog
Comment 8 Jonathan Bedard 2018-02-08 08:36:29 PST
Created attachment 333377 [details]
Patch
Comment 9 Daniel Bates 2018-02-08 10:23:21 PST
Comment on attachment 333377 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/checkout.py:140
> +        return set(map(self.commit_info_for_revision, revisions)) - set([None])

Please add a comment above this line that explains why a None can be in the set. Maybe something like:

# Remove a None entry from the set. This can happen if a revision does have an associated ChangeLog entry (e.g. r185745).

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:431
> +                return [-1]  # Will return None for revision information.

This code is disingenuous and the comment is not precise. {Git, SVN}.revisions_changing_file() always returns a list of valid revisions. For the purposes of testings the number that we pick for the revision serves as a unique identifier so that when mock_changelog_entries_for_revision() is called it knows what mock value to return. It is always best to make the unit test and mock code as representable as possible to the real environment to ensure that we are exercising real-world code paths and not inadvertently testing some quirky, unit-test-specific code path that may make the unit test pass, but is not representative of the real-world code path that we intended to test.

We should return a list with some unique revision number here, say 27, and then have mock_changelog_entries_for_revision() return the empty list for revision 27. I do not think a comment is necessary. If you are adamant about having a comment then the comment should reference "mock_changelog_entries_for_revision()".

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:436
> +        checkout._scm.changed_files = lambda git_commit: ["file1", "file2", "relative/path/ChangeLog", 'file_with_empty_changelog']

We should take the opportunity to fix up the style of this line since we are modifying it. Please change all double quotes to single quotes.
Comment 10 Daniel Bates 2018-02-08 10:25:41 PST
Comment on attachment 333377 [details]
Patch

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

> Tools/ChangeLog:7
> +

Please add an explanation of the issue the fix here. Per-line function comments are insufficient. In general, you should always write a description in the ChangeLog entry. The only exception is when the bug title describes the fix (e.g. Rename A to B).
Comment 11 Daniel Bates 2018-02-08 10:26:26 PST
(In reply to Daniel Bates from comment #10)
> Comment on attachment 333377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333377&action=review
> 
> > Tools/ChangeLog:7
> > +
> 
> Please add an explanation of the issue the fix here. 

Please add an explanation of the issue and the fix here.
Comment 12 Jonathan Bedard 2018-02-08 11:01:00 PST
Created attachment 333391 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2018-02-08 17:24:40 PST
Comment on attachment 333391 [details]
Patch for landing

Clearing flags on attachment: 333391

Committed r228303: <https://trac.webkit.org/changeset/228303>
Comment 14 WebKit Commit Bot 2018-02-08 17:24:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2018-02-08 17:29:32 PST
<rdar://problem/37373232>