| Summary: | [style] Fix --git-index option for check-webkit-style command | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||
| Component: | Tools / Tests | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | aakash_jain, ap, Basuke.Suzuki, bugs-noreply, calvaris, commit-queue, darin, dbates, ddkilzer, ews-bot+gtk-3, ews-watchlist, glenn, jbedard, lforschler, mcatanzaro, rniwa, webkit-bug-importer, youennf | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Basuke Suzuki
2018-06-19 11:53:58 PDT
Created attachment 343082 [details]
FIX
Is it possible to write unit tests for this? We have scm unit tests in webkitpy/common/checkout/scm/scm_unittest.py. These tests actually instantiate repositories locally, they're slow, but this seems like the kind of weird edge case that warrants tests. Oh, yea. I'll try. Created attachment 343273 [details]
Add unittest
$ python Tools/Scripts/test-webkitpy webkitpy.common.checkout.scm.scm_unittest.GitTest Confirmed these tests locally, the last question: should this be the behavior? If I'm understanding this correctly, this change means that if I have a local commit and have added, but not committed, additional changes, both the local commit and uncommitted files would be included. I use svn, so I'm probably not the right person to make this call. Git is different from svn that git has an indexed state, which is intermediate state between uncommitted and committed. It exists only one for a repository. So if the command line argument says "please compare with index", that means scan staged files only.
Here is the document of this option.
> --git-index Scan staged files only
It says stated files only. (Staged file means the files in index state).
And this option to create_patch() method only passed from check-webkit-style script.
Comment on attachment 343273 [details] Add unittest Attachment 343273 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8282659 New failing tests: http/tests/security/canvas-remote-read-remote-video-localhost.html Created attachment 343287 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 343273 [details] Add unittest Attachment 343273 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8283656 New failing tests: imported/w3c/web-platform-tests/workers/semantics/structured-clone/dedicated.html Created attachment 343300 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 343273 [details] Add unittest View in context: https://bugs.webkit.org/attachment.cgi?id=343273&action=review > Tools/Scripts/webkitpy/common/checkout/scm/git.py:341 > + command = [self.executable_name, 'diff', '--binary', '--no-color', "--no-ext-diff", "--full-index", "--no-renames", order] We should take this opportunity to fix the style in this lien and make all literals single quoted. (In reply to Daniel Bates from comment #12) > Comment on attachment 343273 [details] > Add unittest > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343273&action=review > > > Tools/Scripts/webkitpy/common/checkout/scm/git.py:341 > > + command = [self.executable_name, 'diff', '--binary', '--no-color', "--no-ext-diff", "--full-index", "--no-renames", order] > > We should take this opportunity to fix the style in this lien and make all > literals single quoted. Sure I do. For the reference, where can I find those Python coding style guideline? Thanks for r+. Created attachment 343360 [details]
fix
Comment on attachment 343360 [details] fix Clearing flags on attachment: 343360 Committed r233097: <https://trac.webkit.org/changeset/233097> All reviewed patches have been landed. Closing bug. (In reply to Basuke Suzuki from comment #13) > Sure I do. For the reference, where can I find those Python coding style > guideline? I'm not sure if it's written down anywhere, but it is PEP 8 minus the line length limit. (In reply to Michael Catanzaro from comment #18) > (In reply to Basuke Suzuki from comment #13) > > Sure I do. For the reference, where can I find those Python coding style > > guideline? > > I'm not sure if it's written down anywhere, but it is PEP 8 minus the line > length limit. The PEP 8 requirement is written down at <https://webkit.org/code-style-guidelines/#python>. We should amend the guidelines to add a remark that we do not respect the line length limit. See the discussion on bug #179387. |