The --suggest-reviewers feature of webkit-patch is broken on newer versions of git when any commit being considered has a single ChangeLog and that ChangeLog is the first file in the list of changed files for a commit. In Tools/Scripts/webkitpy/common/checkout/scm/git.py we have: def _changes_files_for_commit(self, git_commit): # --pretty="format:" makes git show not print the commit log header, changed_files = self._run_git(["show", "--pretty=format:", "--name-only", git_commit]).splitlines() # instead it just prints a blank line at the top, so we skip the blank line: return changed_files[1:] And let's say we're looking at 0b09603c479afc9ce9256ecbb36cb18e149fe81c (r174332): $ git show --pretty=format: --name-only 0b09603c479afc9ce9256ecbb36cb18e149fe81c Source/WebCore/ChangeLog Source/WebCore/bindings/gobject/WebKitDOMPrivate.cpp Source/WebCore/css/CSSStyleSheet.h Source/WebCore/css/StyleSheet.h Source/WebCore/dom/Document.cpp Source/WebCore/dom/DocumentStyleSheetCollection.cpp Source/WebCore/dom/ProcessingInstruction.cpp Source/WebCore/inspector/InspectorCSSAgent.cpp Source/WebCore/xml/XSLStyleSheet.h The existing code will skip 'Source/WebCore/ChangeLog' from the list of files, thus causing --suggest-reviewers to break: 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 "./Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "./Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "./Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 54, in execute self._sequence.run_and_handle_errors(tool, options, state) File "./Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "./Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "./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 "./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 "./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'
Created attachment 241378 [details] Patch v1
Comment on attachment 241378 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=241378&action=review > Tools/Scripts/webkitpy/common/checkout/scm/git.py:229 > + return [s for s in changed_files.splitlines() if s.strip()] This is OK as-is (*). Notice that str.splitlines() breaks a string into lines removing carriage return, new-line characters (by default) and str.strip() returns a copy of a string without leading and trailing whitespace, including carriage return and newline characters. Is it necessary to remove leading and trailing whitespace from each line? I mean, it seems sufficient to trim leading whitespace from the string changed_files before splitting it at line breaks and write: return changed_files.lstrip().splitlines() (*) For completeness, with the proposed change we will truncate filenames that have trailing whitespace (e.g. "../WebCore/A Filename With A Space At The End "). I doubt we need to support such file names.
Comment on attachment 241378 [details] Patch v1 The patch is fine to land as-is. I'm marking this patch cq- should you wish to consider my remark in comment #2. Feel free to cq+ the patch to land as-is.
Comment on attachment 241378 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=241378&action=review >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:229 >> + return [s for s in changed_files.splitlines() if s.strip()] > > This is OK as-is (*). Notice that str.splitlines() breaks a string into lines removing carriage return, new-line characters (by default) and str.strip() returns a copy of a string without leading and trailing whitespace, including carriage return and newline characters. Is it necessary to remove leading and trailing whitespace from each line? I mean, it seems sufficient to trim leading whitespace from the string changed_files before splitting it at line breaks and write: > > return changed_files.lstrip().splitlines() > > (*) For completeness, with the proposed change we will truncate filenames that have trailing whitespace (e.g. "../WebCore/A Filename With A Space At The End "). I doubt we need to support such file names. I realize this change doesn't precisely preserve the previous behavior, but my goal was to make the new behavior more explicit/well-defined to defend against a future change to git-show output. I'll land a follow-up fix once the CQ lands this to use your suggestion above.
Committed r175991: <http://trac.webkit.org/changeset/175991>
This happens again on mac using Yosemite. [changseok@MacBookProStation:WebKit]$ ./Tools/Scripts/webkit-patch suggest-reviewers 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 "/Users/changseok/Projects/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Users/changseok/Projects/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Users/changseok/Projects/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 54, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Users/changseok/Projects/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Users/changseok/Projects/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Users/changseok/Projects/WebKit/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 "/Users/changseok/Projects/WebKit/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 "/Users/changseok/Projects/WebKit/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'