On 16/02/12 02:26, Dave Reisner wrote: > On Wed, Feb 15, 2012 at 7:05 AM, Allan McRae <[email protected]> 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 <[email protected]> >>> --- >>> 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
