Page MenuHomePhabricator

Check both UNIX- and Windows-style paths from linter output
ClosedPublic

Authored by alexmv on Sep 20 2016, 10:05 PM.
Tags
None
Referenced Files
F14169320: D16579.id39904.diff
Sat, Dec 7, 4:03 PM
F14169248: D16579.diff
Sat, Dec 7, 3:46 PM
Unknown Object (File)
Wed, Dec 4, 3:33 AM
Unknown Object (File)
Sun, Nov 24, 4:30 AM
Unknown Object (File)
Sun, Nov 17, 3:06 PM
Unknown Object (File)
Nov 5 2024, 11:34 PM
Unknown Object (File)
Nov 5 2024, 11:34 PM
Unknown Object (File)
Nov 5 2024, 11:34 PM
Subscribers

Details

Summary

Paths are passed into linters using UNIX-style slashes (/), as returned from the version control system; however,
Filesystem::readablePath swaps them to Windows-style (\) on
Windows when storing the names of the files with lint messages. This causes no lint message's path to match the set of
changed files, and thus no lint warnings are ever produced.

If a lint message's file is not found using the provided filename, also try looking up the UNIX-style filename, on Windows when determining if a lint mesage is "relevant."

Fixes T11248.

Test Plan

Ran arc lint on Windows.

Diff Detail

Repository
rARC Arcanist
Branch
up-windows-normalization
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 13790
Build 17816: Run Core Tests
Build 17815: arc lint + arc unit

Unit TestsFailed

TimeTest
103 msArcanistClosureLinterTestCase::Unknown Unit Message ("")
In 'gjslint.lint-test', expected lint to raise error on line 2 at char 0, but no error was raised. Actually raised: No messages.
20 msArcanistCppcheckLinterTestCase::Unknown Unit Message ("")
In 'zblair.lint-test', lint raised error on line 15 at char 0, but nothing was expected: Mismatching allocation and deallocation: buf
36 msArcanistCSSLintLinterTestCase::Unknown Unit Message ("")
1 assertion passed.
325 msArcanistCSSLintLinterTestCase::Unknown Unit Message ("")
9 assertions passed.
8 msArcanistChmodLinterTestCase::Unknown Unit Message ("")
5 assertions passed.
View Full Test Results (2 Failed · 40 Passed · 12 Skipped)

Event Timeline

alexmv retitled this revision from to Normalize paths stored in linters to UNIX-style.
alexmv updated this object.
alexmv edited the test plan for this revision. (Show Details)

This feels like it's headed in the wrong direction -- I imagine we'll be better off in the long run normalizing paths at the last possible second.

Fixing this properly is probably entangled with a lot of other stuff, but I'll accept a change to isRelevantMessage() instead which accounts for this normalization and treats \x\y as matching /x/y or whatever, if that's reasonable? That is, just make \x\y a "relevant message" if /x/y was changed.

In the long run, when running on Windows I think we should try to keep paths in Windows format and, e.g., show the user Windows paths, since this is likely what they expect and are most familiar with. Things maybe become more ambiguous in some contexts (e.g., there may be some value in normalizing paths headed to the server, so that changes look identical whether they came from a Windows machine or not).

Is doing this correction in isRelevantMessage() instead reasonable?

That seems reasonable. I'll push an updated diff later today, which tries both forms of slashes in isRelemvantMessage() if running on Windows.

alexmv edited edge metadata.

Do normalization on checking in isRelevantMessage, not during
storage in paths.

The previous version of this diff thus reported paths to the user in
UNIX-style:

>>> Lint path/to/file.php:

With this update, they are reported to the user using the Windows
delimeter:

>>> Lint got path\to\file.php:

Somehow another change snuck in, though I don't see it locally?

epriestley added a reviewer: epriestley.

Seems reasonable, thanks!

This revision is now accepted and ready to land.Sep 21 2016, 9:25 PM
alexmv retitled this revision from Normalize paths stored in linters to UNIX-style to Check both UNIX- and Windows-style paths from linter output.Sep 21 2016, 9:28 PM
alexmv edited edge metadata.