Page MenuHomePhabricator

Run the linters as part of arc land
ClosedPublic

Authored by Fabien on Feb 7 2020, 12:51.

Details

Summary

The default arc land workflow does not run the linters before landing.
This can cause issues when the user issues a rebase before landing, such
as D5192.

This diff runs arc lint --severity autofix --amend-autofix before
arc land to try to catch these issues.

A --bypass-linters options is added to arc land to avoid deadlocks.

Test Plan

Add a trailing whitespace to an arcanist PHP file, then:

arc land --preview

Check the linter error is returned.

arc land --preview --bypass-linters

Check no linter error is returned. Arc land will fail due to dirty tree.

Create a local commit with a clang-format violation (such as bad header
ordering).

arc land --preview

See the auto amend message. arc land should succeed.

arc help land

Check the --bypass-linters is added to the list.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markblundeberg added inline comments.
arcanist/.phutil_module_cache
1 ↗(On Diff #16110)

possible to mark this as autogenerated?

1 ↗(On Diff #16110)

or should this not be tracked? (instead generated locally)

arcanist/.phutil_module_cache
1 ↗(On Diff #16110)

It should be marked as generated.

I'm curious, supposing I'm on an old commit and the landing process needs to rebase me onto master, which state does it run lint on: the preland commit or the rebased commit?

This is strictly equivalent to running arc lint then arc land.
It applies to the current comment before the rebase. If there is no rebase conflict the risk that a new linter issue appear is very low.

This is strictly equivalent to running arc lint then arc land.
It applies to the current comment before the rebase. If there is no rebase conflict the risk that a new linter issue appear is very low.

👍

Hmm I guess one thing that could happen is if someone lands and the linting rules have since been restricted, then the rebase commit might have errors.

deadalnix requested changes to this revision.Feb 7 2020, 16:24
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
arcanist/configuration/ArcanistBitcoinABCConfiguration.php
8 ↗(On Diff #16110)

Do not run the linters. But in fact even this is a bit misleading, because it is assumed that the default behavior of the linter is known, which is likely not the case for someone reaching for the help.

Something like --bypass-linters convey the point way better for instance.

This revision now requires changes to proceed.Feb 7 2020, 16:24
arcanist/configuration/ArcanistBitcoinABCConfiguration.php
8 ↗(On Diff #16110)

I will update.
Note that this is a direct copy of the arc diff option (including the help message).

arcanist/configuration/ArcanistBitcoinABCConfiguration.php
29 ↗(On Diff #16110)

I'm not totally sure we want autofix by the way, as it means something can get committed to master that's just wrong. (or are the autofixers super duper reliable?)

Rebase and address feedback.

arcanist/configuration/ArcanistBitcoinABCConfiguration.php
29 ↗(On Diff #16110)

The autofix have proven being reliable so far, but this can't be 100% guaranteed.

There are a lot of ways to commit something wrong to master, in particular since there can be any mistake occurring from a rebase, that won't be catched by linters and can cause a build failure.

That said, the risk is pretty low that a bad linter introduces an issue as it requires several conditions to occur, so I think the UX improvement is worth the added risk.

This revision is now accepted and ready to land.Feb 7 2020, 17:17
Fabien edited the test plan for this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.