rmlint icon indicating copy to clipboard operation
rmlint copied to clipboard

Add set -e to the bash script?

Open ghost opened this issue 4 years ago • 3 comments
trafficstars

In general, set -e is not such a great thing, as it requires extra care when working with pipes. (http://mywiki.wooledge.org/BashFAQ/105)

However, rmlint.sh does not seem to be using a lot of pipes, and generally is using a very fragile technique of bash script generation, so I expect set -e to bring more good than harm, in case some strange file names are escaped incorrectly, for example.

ghost avatar Jan 28 '21 02:01 ghost

I like the link. Especially the conclusions at the end.

Will test it and see if it breaks any nosetests, and then maybe come back and discuss some more.

SeeSpotRun avatar Jan 28 '21 05:01 SeeSpotRun

Testing result: didn't break any of the existing nosetests.

@sahib: any philosophical or technical objections?

SeeSpotRun avatar Mar 27 '21 07:03 SeeSpotRun

I'm a fan of the proposal. There's no good reason set -e isn't already there, mostly because I didn't know about it many years ago. You might also think about adding -u (abort if expanding undefined variables) and maybe also -o pipefail. I can think of some drawbacks though, which need to be evaluated first:

  • set -e does not necessarily mean that an error is printed. Quiet often it leads to scripts that silently abort because some program silently returned a non-zero exit code. One should double check that there's no program in rmlint.sh that might return silent or neglectable errors. If unsure, suffixing with || true might be a solution.
  • This changes the behavior. Some users/scripts might rely on the behavior of just continuing in case of slight errors. Maybe a rmlint.sh option like --ignore-errors (which does set +e) would be good here.

sahib avatar Mar 27 '21 17:03 sahib