This is a great idea I plan to heavily use.
utACK 4759716 besides the script itself which I still have some doubts about. Probably instead of learning the git-fu involved now I could simply go ahead and test it by writing something based on this.

@jtimon You're right to doubt the script, it's quite kludgy. it works by rewinding to the previous commit, running the supplied transform script, and comparing the result to the actual commit.

Because the script needs to work on real files, I'm not sure how to avoid working on the current repo, other than maybe setting up a dummy index/workdir.

ACK 6ca7f42 conditional to 74214a2 failing on travis but the whole PR squashed as a single commit passing. See #10193

ACK 6ca7f42 Travis fails if not all the changes performed by the script are included in the commit as expected https://travis-ci.org/bitcoin/bitcoin/jobs/221211199 I will now squash #10193 so that it passes.

As discussed on IRC, making the script require some prefix in the commit title (ie first line) would help make sure people see it.

Added enforcement of the prefix: "scripted-diff:" in the first line.

Relying on Travis to do review is a bad thing, and as-implemented, it's not very safe for reviewers to run manually.

But more annoyingly, can the prefix be on a not-first-line where it pollutes the commit summary? :/

@luke-jr it's enforced in the subject line so that reviewers can't miss it. That was the resounding request during the meeting.

It can be fixed up to (optionally) require approval before running. Would that make you comfortable enough to run it locally?

rebased at @jtimon's request

I wouldn't be comfortable with anything less than reading the script and manually running it explicitly...

@luke-jr You're free to c/p the script and run it, then. I understand your hesitance, but this doesn't take away anything, only adds a check which can be ignored.

It takes away from readability of single-line summaries. And gives people a false sense of security if Travis is trusted.

Concept ACK

@luke-jr You're free to c/p the script and run it, then. I understand your hesitance, but this doesn't take away anything, only adds a check which can be ignored.

I agree. We already use Travis for many checks, adding one more isn't problematic. It should not take away from people manually reviewing or running tests locally.

Needs rebase, re-utACK except for the script itself, which I don't fully understand but I have tested using travis.
I don't plan to ever run it locally. The idea of using shell scripts for certain refactor changes and put the commands used in the commit message for easier review is awesome and I'm going to use it whether this check is done by travis or not. Even if reviewers don't run the scripts locally to make sure the changes are complete, they can point it out if the commit is doing something else than what would running the script could possibly do.

It takes away from readability of single-line summaries.

No, the single line summary can still be very clear, and the script in the detailed message just says the same thing in a more formal way.

And gives people a false sense of security if Travis is trusted.

It doesn't give me any false sense of security and from what you say it doesn't give it to you either.
Perhaps we should make clearer somewhere that travis doesn't replace review and testing and that commits prefixed with "scripted-diff" need review like any other commit.

If we're going to keep discussing it, maybe the PR should be separated in the script itself and the changes to net_processing, since the scripted-diff commits can be merged before the script itself is merged anyway (and the script itself will much more unlikely require rebase).

Rebased. If this ends up needing another rebase later due to another net change, I'll split it up.

I'd rather hold off on making the interactive change until this gets a little real-world testing. I'm sure some other things worth addressing will crop up.

utACK 644cb250e708b5724a0879f26449baa5544d35cf (needs squash, of course)

As mentioned on IRC, I drafted some documentation for this, not sure where to put it, maybe contributing.md : https://0bin.net/paste/mTtIoaSoedZ3al-y#dWgD7aBKElhKofVS5nHJBvuvlDLRFmgRy8a03KroOFM

utACK e50c33e