138627 2014-11-11 13:29:25 -0800 webkit-patch --suggest-reviewers is broken with newer versions of git 2015-06-12 23:11:01 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ddkilzer ddkilzer ap buildbot changseok commit-queue dbates glenn jake.nielsen.webkit rniwa oldest_to_newest 1047960 0 ddkilzer 2014-11-11 13:29:25 -0800 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' 1047961 1 241378 ddkilzer 2014-11-11 13:31:52 -0800 Created attachment 241378 Patch v1 1047991 2 241378 dbates 2014-11-11 15:15:51 -0800 Comment on attachment 241378 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. 1047994 3 241378 dbates 2014-11-11 15:21:25 -0800 Comment on attachment 241378 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. 1047997 4 241378 ddkilzer 2014-11-11 15:27:19 -0800 Comment on attachment 241378 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. 1048028 5 ddkilzer 2014-11-11 16:47:19 -0800 Committed r175991: <http://trac.webkit.org/changeset/175991> 1101796 6 changseok 2015-06-12 23:11:01 -0700 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' 241378 2014-11-11 13:31:52 -0800 2014-11-11 15:21:25 -0800 Patch v1 bug-138627-20141111133155.patch text/plain 2075 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMTc1OTU2CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggNzcwMzM0YjE5NDQzNGQxNjM2ZGFjNTI1M2YyYjhjYWIy YTAzNThiZi4uY2UzM2FjYzkzN2VmNjNjZjYwOWY1MDJmOWU3YTgzYjU1NjNjZTM3MiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1 IEBACisyMDE0LTExLTExICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CisKKyAg ICAgICAgd2Via2l0LXBhdGNoIC0tc3VnZ2VzdC1yZXZpZXdlcnMgaXMgYnJva2VuIHdpdGggbmV3 ZXIgdmVyc2lvbnMgb2YgZ2l0CisgICAgICAgIDxodHRwOi8vd2Via2l0Lm9yZy9iLzEzODYyNz4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIFNjcmlw dHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHk6CisgICAgICAgIChHaXQuX2No YW5nZXNfZmlsZXNfZm9yX2NvbW1pdCk6IEZpbHRlciBvdXQgYmxhbmsgbGluZXMgaW5zdGVhZAor ICAgICAgICBvZiBhc3N1bWluZyB0aGVyZSBpcyBhbHdheXMgYSBibGFuayBsaW5lIGF0IHRoZSBi ZWdpbm5pbmcgb2YgdGhlCisgICAgICAgIGxpc3QuCisKIDIwMTQtMTAtMDcgIFNlcmdpbyBWaWxs YXIgU2VuaW4gIDxzdmlsbGFyQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0NTUyBHcmlkIExheW91 dF0gTGltaXQgdGhlIHNpemUgb2YgZXhwbGljaXQvaW1wbGljaXQgZ3JpZApkaWZmIC0tZ2l0IGEv VG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvc2NtL2dpdC5weSBiL1Rvb2xz L1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHkKaW5kZXggZGVkMTZm OTMyMmQ4MGFiMzExNmE2ZGI5ZmI0NTEzZWZiNzU1ZWYzNS4uZTdhN2FlNjE1MDJlOWE1OWM1ZWJk Y2RkYmI2NzRlOTFiN2Y4NmY2YiAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9j b21tb24vY2hlY2tvdXQvc2NtL2dpdC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2Nv bW1vbi9jaGVja291dC9zY20vZ2l0LnB5CkBAIC0yMjMsMTAgKzIyMywxMCBAQCBjbGFzcyBHaXQo U0NNLCBTVk5SZXBvc2l0b3J5KToKICAgICAgICAgcmV0dXJuIHNlbGYucnVuX3N0YXR1c19hbmRf ZXh0cmFjdF9maWxlbmFtZXMoc3RhdHVzX2NvbW1hbmQsIHNlbGYuX3N0YXR1c19yZWdleHAoIkFE TSIpKQogCiAgICAgZGVmIF9jaGFuZ2VzX2ZpbGVzX2Zvcl9jb21taXQoc2VsZiwgZ2l0X2NvbW1p dCk6Ci0gICAgICAgICMgLS1wcmV0dHk9ImZvcm1hdDoiIG1ha2VzIGdpdCBzaG93IG5vdCBwcmlu dCB0aGUgY29tbWl0IGxvZyBoZWFkZXIsCi0gICAgICAgIGNoYW5nZWRfZmlsZXMgPSBzZWxmLl9y dW5fZ2l0KFsic2hvdyIsICItLXByZXR0eT1mb3JtYXQ6IiwgIi0tbmFtZS1vbmx5IiwgZ2l0X2Nv bW1pdF0pLnNwbGl0bGluZXMoKQotICAgICAgICAjIGluc3RlYWQgaXQganVzdCBwcmludHMgYSBi bGFuayBsaW5lIGF0IHRoZSB0b3AsIHNvIHdlIHNraXAgdGhlIGJsYW5rIGxpbmU6Ci0gICAgICAg IHJldHVybiBjaGFuZ2VkX2ZpbGVzWzE6XQorICAgICAgICAjIC0tcHJldHR5PSJmb3JtYXQ6IiBt YWtlcyBnaXQgc2hvdyBub3QgcHJpbnQgdGhlIGNvbW1pdCBsb2cgaGVhZGVyLgorICAgICAgICBj aGFuZ2VkX2ZpbGVzID0gc2VsZi5fcnVuX2dpdChbInNob3ciLCAiLS1wcmV0dHk9Zm9ybWF0OiIs ICItLW5hbWUtb25seSIsIGdpdF9jb21taXRdKQorICAgICAgICAjIEZpbHRlciBvdXQgYmxhbmsg bGluZXMgd2hpY2ggY291bGQgYXBwZWFyIGF0IHRoZSB0b3Agb24gb2xkZXIgdmVyc2lvbnMgb2Yg Z2l0LgorICAgICAgICByZXR1cm4gW3MgZm9yIHMgaW4gY2hhbmdlZF9maWxlcy5zcGxpdGxpbmVz KCkgaWYgcy5zdHJpcCgpXQogCiAgICAgZGVmIGNoYW5nZWRfZmlsZXNfZm9yX3JldmlzaW9uKHNl bGYsIHJldmlzaW9uKToKICAgICAgICAgY29tbWl0X2lkID0gc2VsZi5naXRfY29tbWl0X2Zyb21f c3ZuX3JldmlzaW9uKHJldmlzaW9uKQo=