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

Authored by alexmv on Sep 20 2016, 10:05 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T11248: ArcanistLinter's addLintMessage incorrectly normalizes path on Windows

Commits

rARCe7906e47cba2: (stable) Promote 2016 Week 39
rARC483e985d08d2: Check both UNIX- and Windows-style paths from linter output

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 13795
Build 17826: Run Core Tests
Build 17825: arc lint + arc unit

Event Timeline

alexmv updated this revision to Diff 39904.Sep 20 2016, 10:05 PM

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)

alexmv added a task: T11248: ArcanistLinter's addLintMessage incorrectly normalizes path on Windows.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptSep 20 2016, 10:05 PM

Herald added a subscriber: epriestley. · View Herald Transcript

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 planned changes to this revision.Sep 21 2016, 7:19 PM

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:

alexmv updated this object.Sep 21 2016, 9:08 PM

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

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.

alexmv closed this revision.Sep 21 2016, 9:29 PM