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
F14172243: D16579.id39910.diff
Sun, Dec 8, 3:59 AM
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
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
Tests Passed
Build Status
Buildable 13796
Build 17828: Run Core Tests
Build 17827: arc lint + arc unit

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.