test: Run scripted-diff in subshell #14864

1. 
   dongcarl commented at 4:38 AM on December 4, 2018: member

   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.

2. 
   dongcarl commented at 4:39 AM on December 4, 2018: member

   Fixes concerns described in: #13743#pullrequestreview-166625569

3. fanquake added the label Tests on Dec 4, 2018
4. 
   Empact commented at 10:24 AM on December 4, 2018: member

   How about making this a scripted-diff?

5. 
   practicalswift commented at 10:42 AM on December 4, 2018: contributor

   Concept ACK

   Nice improvement to the scripted-diff integrity!

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

6. in test/lint/commit-script-check.sh:34 in 53977cf12f outdated

     37 | -            echo "Running script for: $i"
     38 | +            echo "Running script for: $commit"
     39 |              echo "$SCRIPT"
     40 | -            eval "$SCRIPT"
     41 | -            git --no-pager diff --exit-code $i && echo "OK" || (echo "Failed"; false) || RET=1
     42 | +            (eval "$SCRIPT")
   

   ---

   ---

   

   practicalswift commented at 10:50 AM on December 4, 2018:

   Would it make sense to use (set -e; eval "$SCRIPT") || RET=1 && echo "scripted-diff execution failed."?

   That would allow us to catch execution failures within the scripted-diff run. Currently such execution failure are silent (as long as the diff produces the expected result).

   It probably shouldn't be done as part of this PR as that would change behaviour.

   ---

   

   dongcarl commented at 4:08 PM on December 4, 2018:

   Not sure about set -e, as I've found that people are not used to its behaviour and there are legitimate uses of not failing immediately upon the failure of one command. I think that as long as the diff produces the expected result, we should be good.

   What I would like to have is set -x, which doesn't change behaviour but allows us to see line by line execution fully expanded (in case it's a complex script they'd like to debug).

   ---

   

   practicalswift commented at 4:13 PM on December 4, 2018:

   Yes, you have a good point. set -x would be really nice :-)

7. 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-

   43f9099901
8. dongcarl force-pushed on Dec 4, 2018
9. 
   dongcarl commented at 4:03 PM on December 4, 2018: member

   Made it M E T A

10. MarcoFalke added this to the milestone 0.18.0 on Dec 4, 2018
11. 
    practicalswift commented at 4:27 PM on December 4, 2018: contributor

    utACK 43f909990190b3ff7883f0b2c117daa876d8fd99

12. ryanofsky approved
13. 
    ryanofsky commented at 4:53 PM on December 4, 2018: member

    utACK 43f909990190b3ff7883f0b2c117daa876d8fd99. 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.

14. 
    Empact commented at 9:35 PM on December 4, 2018: member

    utACK 43f9099

15. laanwj merged this on Dec 6, 2018
16. laanwj closed this on Dec 6, 2018

    ---

17. laanwj referenced this in commit 1858e6f2f2 on Dec 6, 2018
18. zkbot referenced this in commit 311a079dd5 on Oct 27, 2020
19. Munkybooty referenced this in commit 361567c1e5 on Aug 2, 2021
20. Munkybooty referenced this in commit dc5e329328 on Aug 3, 2021
21. Munkybooty referenced this in commit f6c48f05ed on Aug 5, 2021
22. Munkybooty referenced this in commit 8175bd9e5b on Aug 5, 2021
23. Munkybooty referenced this in commit f10d013d78 on Aug 8, 2021
24. Munkybooty referenced this in commit a0caeb56b4 on Aug 11, 2021
25. Munkybooty referenced this in commit 96149e2723 on Aug 11, 2021
26. Munkybooty referenced this in commit 31924dbd30 on Aug 15, 2021
27. MarcoFalke locked this on Sep 8, 2021

Contributors

dongcarlEmpactpracticalswiftryanofsky

Milestone
0.18.0