On 16/02/12 02:26, Dave Reisner wrote:
On Wed, Feb 15, 2012 at 7:05 AM, Allan McRae <allan@archlinux.org> wrote:
On 15/02/12 04:57, Dave Reisner wrote:
It's expected that this will lead to unwanted behavior, and needs widespread testing. It's desirable to commit this for a few reasons:
- there's no reason we can't do our own error checking for code that we write. - it avoids the need for ||true hacks scattered about in the code. - it makes us immune to upstream changes in exit codes (FS#28248)
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Allan, just making sure we're on the same page -- this _will_ cause breakage, and the next patch in this series addresses one specific case. I figure getting this patch in now gives us "ample time" to uncover and fix all these before the next release.
OK... I like this idea somewhat as I have seen the issues this can cause. But I have also seen it validly stop the scripts execution due to errors that would have been non-obvious.
Here goes a few concerns you might help me alleviate:
1) does it really make us immune to upstream changes in exit codes? In the particular example of mercurial, would we not be checking the exit code of the "hg pull" part anyway?
Yeah, this is probably accurate. I feel like there's always going to be commands which can exit non-zero for which we still expect that they did what we told them to, I just picked a bad example.
2) how much extra error checking are we going to need to do? e.g. if I look at create_srcpackage() would we only need to check the mkdir and ln lines?
So, the big stuff is: file/directory creation, directory changes, and executing arbitrary code (e.g. sourcing files). There's no reason we couldn't create a "lighter weight" ERR trap with errtrace if we expect to go through an entire function that needs to succeed, rather than checking everything. create_srcpackage() looks like a good candidate for this sort of thing. We can do something like this:
somefunc() { true; false; true; echo 'shouldnt be here' } set -E trap -- 'return 1 2>/dev/null' ERR somefunc set +E trap -- ERR echo 'success'
The stderr redirect is due to what I suspect is a bug in bash (exists in bash3 as well) -- it errors out saying you can only return from a function, but it gives us the behavior we want (evidenced by the debug echo's). I'm going to followup with bug-bash@gnu to see if this is really the case, though.
You are not going to convince anyone with that code! :D Would it be useful to have wrapper functions around cd, ln, mkdir that would take most of the additional testing burden away? Anyway, I am happy for this change to go ahead. But I would hold off committing it master until a few more of the obvious checks that will now be needed have been added. Allan