Paths

Table of Contentst

Differential D5196

Run the linters as part of arc land
ClosedPublic
Actions

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

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING025472d9a046: Run the linters as part of arc land
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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Feb 7 2020, 12:51

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 7 2020, 12:51

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg added a subscriber: markblundeberg.Feb 7 2020, 12:56

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.Feb 7 2020, 12:59

arcanist/.phutil_module_cache
1 ↗	(On Diff #16110)	It should be marked as generated.

Harbormaster completed remote builds in B9324: Diff 16110.Feb 7 2020, 13:10

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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)Feb 7 2020, 14:46

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.

In D5196#125526, @Fabien wrote:

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

Fabien added inline comments.Feb 7 2020, 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.Feb 7 2020, 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?)

Rebase and address feedback.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fabien added inline comments.Feb 7 2020, 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.