Page MenuHomePhabricator

Add scripted-diffs verification into arcanist
Needs ReviewPublic

Authored by Fabien on Mon, Mar 25, 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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 5464
Build 8990: Bitcoin ABC Teamcity Staging
Build 8989: arc lint + arc unit

Event Timeline

Fabien created this revision.Mon, Mar 25, 21:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Mar 25, 21:39
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox requested changes to this revision.Wed, Mar 27, 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.Wed, Mar 27, 19:43
Fabien added a comment.Thu, Mar 28, 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.Thu, Mar 28, 07:05
Fabien updated this revision to Diff 7858.Thu, Mar 28, 12:22

Rebase

Why isn't it a linter?

deadalnix requested changes to this revision.Sun, Mar 31, 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.Sun, Mar 31, 17:46
Fabien added a comment.Sun, Mar 31, 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.Mon, Apr 1, 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)Mon, Apr 1, 09:44
deadalnix requested changes to this revision.EditedThu, Apr 4, 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.Thu, Apr 4, 19:57
Fabien added a comment.Fri, Apr 5, 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.Mon, Apr 8, 13:10

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

Fabien edited the summary of this revision. (Show Details)Mon, Apr 8, 13:13
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 8071.Tue, Apr 16, 12:17

Rebase, and lint