Bug 182584 – webkit-patch suggest-reviewers dies with AttributeError: 'NoneType' object has no attribute 'revision'

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

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.

[Jonathan Bedard]

Created attachment 333340 [details]
Patch

[Jonathan Bedard]

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

[Daniel Bates]

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?

[Daniel Bates]

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?

[Daniel Bates]

(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

[Daniel Bates]

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.

[Jonathan Bedard]

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

[Jonathan Bedard]

Created attachment 333377 [details]
Patch

[Daniel Bates]

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.

[Daniel Bates]

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

[Daniel Bates]

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

[Jonathan Bedard]

Created attachment 333391 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 333391 [details]
Patch for landing

Clearing flags on attachment: 333391

Committed r228303: <https://trac.webkit.org/changeset/228303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37373232>