Bug 186810 – [style] Fix --git-index option for check-webkit-style command

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

[Basuke Suzuki]

It doesn't work well if master is out of sync with origin/master.

[Basuke Suzuki]

Created attachment 343082 [details]
FIX

[Jonathan Bedard]

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.

[Basuke Suzuki]

Oh, yea. I'll try.

[Basuke Suzuki]

Created attachment 343273 [details]
Add unittest

[Basuke Suzuki]

$ python Tools/Scripts/test-webkitpy webkitpy.common.checkout.scm.scm_unittest.GitTest

[Jonathan Bedard]

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.

[Basuke Suzuki]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Daniel Bates]

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.

[Basuke Suzuki]

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

[Basuke Suzuki]

Created attachment 343360 [details]
fix

[WebKit Commit Bot]

Comment on attachment 343360 [details]
fix

Clearing flags on attachment: 343360

Committed r233097: <https://trac.webkit.org/changeset/233097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41380569>

[Michael Catanzaro]

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

[Daniel Bates]

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