mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
6.99k stars 333 forks source link

interp: make trap consistent with errexit logic #954

Closed reubeno closed 1 year ago

reubeno commented 1 year ago

interp already excludes conditional contexts for exiting on error when errexit is set (see case above); this makes the logic consistently apply the same filtering for reporting errors to a trap routine.

To test this behavior, I ran the below script under bash, under gosh (without this change), and under gosh (with this change). bash reports that the handler was invoked once. Without this change gosh reports that it was invoked twice, and with this change, only once.

#!/bin/bash

count=0

my_handler()
{
  echo "Handler invoked"
  count=$((count + 1))
}

trap my_handler ERR

echo "Trying unconditional"
non_existent_cmd

echo "Trying conditional"
non_existent_cmd || true

echo "Handler called: $count time(s)"
mvdan commented 1 year ago

Thanks for the patch! I guess you're using the interpreter for a project, would be curious to hear what it is :)

The change sounds good to me - it just needs a regression test. See recent changes in the interpreter for some examples, or see the existing trap tests. The script you shared would work, but it's quite lengthy. The test cases tend to be much shorter for the sake of being able to maintain lots of tests more easily.

reubeno commented 1 year ago

@mvdan Thanks for the feedback! And you're right, that test case was far lengthier than it needed to be. I've updated my commit with an additional test case that fails without this change and passes with it. Anything else you'd recommend me do?

As for my interest, I experimented with custom open/exec/readdir/stat/call handlers with interp and ran some off-the-shelf scripts. (I ran into two issues, this being one of them, the other being a script that expected a bash-provided printf to implement %b.) I'm curious to see whether this would be useful for getting more insight into the access patterns of a script during execution. Mostly just exploring for now :).