Page MenuHomePhabricator
Differential D2735

Add scripted-diffs verification into arcanist
AbandonedPublic
Actions

Authored by Fabien on Mar 25 2019, 21:39.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

This diff adds a new command to arcanist arc scripted-diff to verify
that the current commit is compliant with the application of the shell
script contained in the commit message.
This command is also run as part of arc diff.

The shell script should be contained between the following two markers
-BEGIN/END VERIFY SCRIPT-.

It is inspired by core PR10189.

Workflow:

  • Extract the script from the commit message.
  • Checkout the base commit.
  • Run the script
  • Compare the working tree output with the working tree from the

scripted-diff commit (with and without linting it).

  • Reset the working tree and checkout back to the scipted-diff commit.
Test Plan

Create an empty scripted-diff commit (use the --allow-empty option)
with echo scripted-diff; as the shell script, then run:

arc scripted-diff

Should return:

Scripted-diff check is successful

Modify any file then run:

arc scripted-diff

Should return:

Scripted-diff check cannot run due to uncommited changes. Please commit
or reset your changes.

Reset the file change then amend the commit message to set the shell
script section to:

cat filenotfound.txt

Then run:

arc scripted-diff

Should return:

Scripted-diff: the script returned an error (1): cat: filenotfound.txt:
No such file or directory

Amend the commit message to set the shell script section to:

echo "/**/" | tee -a arcanist/workflow/ArcanistScriptedDiffWorkflow.php

Should return:

Scripted-diff: scripted output does not match the commit content.

Change the commit message to set the shell script section to:

echo "/* This comment is really too long, clang-format will cut it to a 80 char length. */" | tee -a src/init.cpp

Edit the init.cpp and add the following comment on the last line:

/* This comment is really too long, clang-format will cut it to a 80 char length. */

Amend the commit, run arc scripted-diff, it should return
Scripted-diff check is successful.
Run arc lint and amend the commit with the proposed change, the
comment should now be formatted on 2 lines.
Run arc scripted-diff again and check the output is the same:

Scripted-diff check is successful

Comment the line "src/init.cpp:.*atoi" in the file
lint-locale-dependence.sh, and amend to commit.
Now add an empty commit on top of this one, with a scripted-diff message
for the script echo lint error.
Run

arc scripted-diff

Should return

Error while linting the base commit. Fixes the issue then run `arc
scripted-diff` again.

Change the commit message to remove the scripted-diff content and run:

arc scripted-diff

It should return nothing.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
scripted_diff
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5464
Build 8990: Bitcoin ABC Buildbot (legacy)
Build 8989: arc lint + arc unit

Event Timeline

Fabien created this revision.Mar 25 2019, 21:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 25 2019, 21:39
Herald added a reviewer: deadalnix. · View Herald Transcript
Herald added a subscriber: schancel. · View Herald Transcript
teamcity added a comment.Mar 25 2019, 21:41

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Mar 25 2019, 21:56

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B5294: Diff 7823!Mar 25 2019, 21:56
jasonbcox requested changes to this revision.Mar 27 2019, 19:43
jasonbcox added a subscriber: jasonbcox.

What's the benefit of scripted diffs? If it's for testing, it seems like we should have proper tests.

btw: the build fails with this diff.

This revision now requires changes to proceed.Mar 27 2019, 19:43
Fabien added a comment.Mar 28 2019, 07:04

Scripted-diffs are not aimed for testing, this is a method to submit diffs that can be created from a few script lines, for example for mass renaming.

This diff provide a submitter level verification of whether the commit content is compliant with the script in the commit message. It runs the script on the previous revision and compare the output to the working tree.
It allows for early detection of missed element or file. This is likely to happen for example when backporting a scripted diff from core; if there is a target for the script in our codebase that doesn't exist in core it can be easily missed.

This diff alone is not sufficient, another check should be performed on server side (likely by the CI), because at the moment there is no way for the reviewers to be sure that the diff submitted actually conforms to the script without running arc patch then arc scripted-diff which is inconvenient.

I'm currently working on backporting PR11651, and I expect the scripted-diff feature to be helpful in this regard. It seems easier to review a 3 lines script rather than a 320+ file change diff (even if splitted this is a large boring change, prone to errors).

Fabien requested review of this revision.Mar 28 2019, 07:05
Fabien updated this revision to Diff 7858.Mar 28 2019, 12:22

Rebase

teamcity added a comment.Mar 28 2019, 12:23

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Mar 28 2019, 12:38

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5323: Diff 7858.Mar 28 2019, 12:38
deadalnix added a comment.Mar 31 2019, 17:45

Why isn't it a linter?

deadalnix requested changes to this revision.Mar 31 2019, 17:46
deadalnix added inline comments.
arcanist/workflow/ArcanistScriptedDiffWorkflow.php
99 ↗(On Diff #7858)

No.

107 ↗(On Diff #7858)

You should check that the repo is clean before doing anything of the kind.

This revision now requires changes to proceed.Mar 31 2019, 17:46
Fabien added a comment.Mar 31 2019, 18:52

A linter runs once per file, which is not the expected workflow here as the check covers the entire working tree.

Fabien updated this revision to Diff 7887.Apr 1 2019, 09:44

Run the script directly instead of writing first to a file.
Check that the working tree is clean before calling git checkout.
Improve the error message if the script returns an error.

Fabien edited the test plan for this revision. (Show Details)Apr 1 2019, 09:44
teamcity added a comment.Apr 1 2019, 09:45

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Apr 1 2019, 10:00

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5341: Diff 7887.Apr 1 2019, 10:00
deadalnix requested changes to this revision.EditedApr 4 2019, 19:57

This will fail if the code if subsequently formatted, which should be fairly common. The test plan should definitively address this. I'm also not convinced this won't fuck everything up when run in parallel with other linters or test runners. If nothing else, it will dirty all builds.

arcanist/workflow/ArcanistScriptedDiffWorkflow.php
127 ↗(On Diff #7887)

There is most likely a way to execute something from the right directory.

This revision now requires changes to proceed.Apr 4 2019, 19:57
Fabien added a comment.Apr 5 2019, 08:41

It will not conflict with other linters because it is run before the linters.
It will conflict with other programs accessing the tree at the same time the same way that git checkout would do.

Fabien updated this revision to Diff 7992.Apr 8 2019, 13:10

Use arcanist facilities to run the script from the root directory.
Make the check insensible to the linter formatting.

teamcity added a comment.Apr 8 2019, 13:11

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fabien edited the summary of this revision. (Show Details)Apr 8 2019, 13:13
Fabien edited the test plan for this revision. (Show Details)
teamcity added a comment.Apr 8 2019, 13:26

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5413: Diff 7992.Apr 8 2019, 13:26
Fabien updated this revision to Diff 8071.Apr 16 2019, 12:17

Rebase, and lint

teamcity added a comment.Apr 16 2019, 12:18

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Apr 16 2019, 12:33

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5464: Diff 8071.Apr 16 2019, 12:33
markblundeberg added a subscriber: markblundeberg.May 5 2019, 18:22

Cool! I've been seeing these scripted-diffs in backports and was wondering how much pain it would be to get them running for us ...

deadalnix requested changes to this revision.May 21 2019, 23:52

This is not a workflow we can run on diff. The whole approach is unsound. See previous comments as to why. Right now, arc diff supports unclean tree and this breaks that. I don't really see how this can be integrated in an automated workflow, but regardless, this is not it.

This revision now requires changes to proceed.May 21 2019, 23:52
Fabien abandoned this revision.May 22 2019, 09:59

Abandon due to concept NACK.

Revision Contents
Changeset List

Diff 8071

View Options

.arcconfig

Loading...
View Options

arcanist/.phutil_module_cache

Loading...
View Options

arcanist/__phutil_library_map__.php

Loading...
View Options

arcanist/configuration/ArcanistBitcoinABCConfiguration.php

Loading...
View Options

arcanist/workflow/ArcanistScriptedDiffWorkflow.php

Loading...