Page MenuHomePhabricator
Create Task
Maniphest T9749

`arc lint --only-new` doesn't work as expected
Closed, ResolvedPublic
Actions

Description

We have a CI job which roughly does this:

arc lint --rev=${PREVIOUS_COMMIT_HASH} --never-apply-patches --only-changed --severity=advice

This is an optimization so that we don't have to lint the entire repository (which takes several hours) on every push. The idea is that we only lint the changes since the last build. The problem here is that --only-changed doesn't raise linter messages for cases in which a change to one line causes a linter violation on another line. For example:

>>> Lint for REDACTED:


   Auto-Fix  (TXT8) Leading Whitespace at BOF
    This file contains leading whitespace at the beginning of the file. This
    is unnecessary and should be avoided when possible.

    >>> -      1 
        -      2 // @import "variables.less";
        +        
               3 @import (reference) "../../theme/variables.less";
               4 @import (reference) "../../libs/mixins.less";
               5

From the output of arc lint --help, it seems that arc lint --only-new is what we should be using (rather than arc lint --only-changed). The problem is that arc lint --only-new relies on the diffusion.getlintmessages Conduit method, which I'm not sure works at the moment.

Revisions and Commits

rARC Arcanist
D18642rARC074dd8f3a6ec Remove "arc lint --only-new"

Related Objects

Event Timeline

joshuaspence created this task.Nov 9 2015, 9:33 PM
joshuaspence raised the priority of this task from to Needs Triage.
joshuaspence updated the task description. (Show Details)
joshuaspence added projects: Arcanist, Lint.
joshuaspence added a subscriber: joshuaspence.

I was thinking of changing arc lint --only-new to do the following:

  1. Work out which files need to be linted.
  2. git checkout $REVISION
  3. Run linters.
  4. git checkout $PREVIOUS_HASH
  5. Run linters.
  6. Compare results.

I'm not sure if this is desirable. The alternative is to wait for diffusion.getlintmessages to mature.

epriestley added a subscriber: epriestley.Nov 9 2015, 10:31 PM

I conceptually like the approach you outline for --only-new.

I think --only-changed has no effect with --rev. It's more for this case:

$ arc lint file.c

...which normally shows all lint for a file. The flags --only-new and --only-changed imply they're closely related but they aren't really particularly closely related.

Maybe better would be removing --only-changed and doing this instead:

$ arc lint --only-changed file.c # Remove
$ arc lint --rev HEAD^ file.c    # New Command

This may already work. One minor advantage is that --only-changed is slightly easier to type if you have a complex "base" rule. We could also make arc lint <path> consider changes by default and disable this behavior with --lintall.

Both of these flags came from Facebook and probably see little-to-no use in the wild so I'm broadly comfortable with major changes.

eadler added a project: Restricted Project.Jan 11 2016, 9:40 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jan 11 2016, 9:42 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:10 PM
Herald added a subscriber: eadler. · View Herald TranscriptJul 4 2016, 9:10 PM
epriestley added a revision: D18642: Remove "arc lint --only-new".Sep 25 2017, 6:17 PM
epriestley mentioned this in D18644: Remove "arc lint --only-changed".Sep 25 2017, 6:39 PM
epriestley closed this task as Resolved by committing rARC074dd8f3a6ec: Remove "arc lint --only-new".Sep 27 2017, 3:21 PM
epriestley added a commit: rARC074dd8f3a6ec: Remove "arc lint --only-new".
epriestley mentioned this in rARCa053fcc4d51b: Remove "arc lint --only-changed".
Phabricator · Built by Phacility