Bug 138627 – webkit-patch --suggest-reviewers is broken with newer versions of git

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

[David Kilzer (:ddkilzer)]

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'

[David Kilzer (:ddkilzer)]

Created attachment 241378 [details]
Patch v1

[Daniel Bates]

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.

[Daniel Bates]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Committed r175991: <http://trac.webkit.org/changeset/175991>

[ChangSeok Oh]

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'