Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Run scripted-diff in subshell #14864

Merged
merged 1 commit into from Dec 6, 2018

Conversation

Projects
None yet
7 participants
@dongcarl
Copy link
Contributor

commented Dec 4, 2018

scripted-diffs should be run in subshells so that their execution does not
affect the shell variables of commit-script-check. Shell variables are not
unset before evaluating the scripted-diff, so that they might be used in
the subshell. To this end, the variable previously named i is now more
descriptively named commit, this also allows scripted-diffs to use the
commonly used variable i without fear of losing a reference to the
commit.

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Fixes concerns described in: #13743 (review)

@fanquake fanquake added the Tests label Dec 4, 2018

@Empact

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

How about making this a scripted-diff?

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Concept ACK

Nice improvement to the scripted-diff integrity!

Would be nice and meta to have this change done as a scripted-diff :-)

scripted-diff: Run scripted-diff in subshell
-BEGIN VERIFY SCRIPT-
sed -i 's/\bi\b/commit/g' test/lint/commit-script-check.sh
sed -i '34s/eval "$SCRIPT"/(eval "$SCRIPT")/' test/lint/commit-script-check.sh
-END VERIFY SCRIPT-

@dongcarl dongcarl force-pushed the dongcarl:2018-12-unset-commit-script-check branch to 43f9099 Dec 4, 2018

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Made it M E T A

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Dec 4, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

utACK 43f9099

@ryanofsky
Copy link
Contributor

left a comment

utACK 43f9099. I'd probably prefer not to set -x by default, and just leave this up to individual scripts, but I wouldn't oppose it. Maybe something to do in a followup PR.

@Empact

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

utACK 43f9099

@laanwj laanwj merged commit 43f9099 into bitcoin:master Dec 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 6, 2018

Merge #14864: test: Run scripted-diff in subshell
43f9099 scripted-diff: Run scripted-diff in subshell (Carl Dong)

Pull request description:

  scripted-diffs should be run in subshells so that their execution does not
  affect the shell variables of commit-script-check. Shell variables are not
  unset before evaluating the scripted-diff, so that they might be used in
  the subshell. To this end, the variable previously named i is now more
  descriptively named commit, this also allows scripted-diffs to use the
  commonly used variable i without fear of losing a reference to the
  commit.

Tree-SHA512: 0d86c069c2a978ca07d71bcd2b1b273e9bfabfe7e31a50c7b1b860e04f178b81c65814c3a38fb01e50b41a5065b646f0dab5b05d9be71138e72d4baba607e37b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.