Page MenuHomePhabricator

Run the linters as part of arc land
ClosedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABC025472d9a046: Run the linters as part of arc land
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Fri, Feb 7, 12:51
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Feb 7, 12:51
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)

Fabien added inline comments.Fri, Feb 7, 12:59
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?

markblundeberg edited the summary of this revision. (Show Details)Fri, Feb 7, 14:46
Fabien added a comment.Fri, Feb 7, 15:03

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.Fri, Feb 7, 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.Fri, Feb 7, 16:24
Fabien added inline comments.Fri, Feb 7, 16:31
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).

markblundeberg added inline comments.Fri, Feb 7, 16:32
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?)

Fabien updated this revision to Diff 16130.Fri, Feb 7, 16:41

Rebase and address feedback.

Fabien added inline comments.Fri, Feb 7, 16:51
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.

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