BTW, I would prefer that David start patch comments with a capital letter, particularly as the final period strongly implies that they are sentences (and not sentence fragments (clauses), as some other contributors seem to prefer).
Eric Kow <[email protected]> writes: > simplify cleanup in overriding-defaults test. > --------------------------------------------- It'd be nice if people editing the tests also added comments explaining what those tests are testing (similar to how we want haddock docstrings for functions). It looks like this test was doing things to $HOME/.darcs and then using traps to clean up after itself, and the new patch changes thing to simply redefine $HOME (which is a big improvement). +1 for this. > [clean up example.sh. > David Roundy <[email protected]>**20081115202545 > Ignore-this: 5342d73ff9d647e0b99f20d6116c579d > ] conflictor [ > hunk ./tests/example.sh 3 > -not () { "$@" && exit 1 || :; } > +. lib > hunk ./tests/example.sh 4 > -alias pwd=hspwd > ] > : > hunk ./tests/example.sh 4 > -alias pwd=hspwd > hunk ./tests/example.sh 6 > not () { "$@" && exit 1 || :; } > alias pwd=hspwd > > -rm -rf temp1 temp2 > mkdir temp1 > cd temp1 > darcs init > hunk ./tests/example.sh 10 > cd .. > - > -rm -rf temp1 temp2 I don't understand why it is "clean" to leave temp directories lying around, and not to remove them if another script left them lying around. See next comment. Also, this conflicts with another patch, which AIUI is Bad Juju. > [get rid of some needless creation and deletion of directories in tests. > David Roundy <[email protected]>**20081201171800 > Ignore-this: d8b3f003397f05ed5a69a16a0b7298d3 > ] hunk ./tests/partial.sh 7 > > set -ev > > -rm -rf temp > mkdir temp > cd temp This is wrong. These rm -rf's are very important, because if the previous test failed or was interrupted, then temp/ will already exist, and the current test will spuriously fail (because "mkdir temp" will abort) -- or worse, they will give very confusing errors if "mkdir -p temp" is used. Instead of this patch, the initial rm -rf's should be amended with a comment "# cleanup after another naughty test" or similar. For a proper solution, see http://bugs.darcs.net/issue1283, "run tests in separate subdirs". > fix typo in test and clean it up a bit. > --------------------------------------- > > chmod 0000 no_perms.txt > # surely there is an easier way to write this 'not' > # i'm think of the 'finally' in try/catch/finally > -dacrs add no_perms.txt 2> log && (chmod 0755 no_perms.txt ; exit 1) || chmod > 0755 no_perms.txt > +darcs add no_perms.txt 2> log && (chmod 0755 no_perms.txt ; exit 1) > +chmod 0755 no_perms.txt > +cat log > grep -i 'permission denied' log A more meaningful patch description would have helped me understand this. What happens is that the "chmod 0000" creates a file that nobody can touch. The later "chmod 0755" is necessary regardless of whether the script fails, because otherwise "make clean" will not be able to delete that file. The sh-ly way of handling this is to trap exit and error signals, such that no matter how the script exits, chmod is run. > cut hardly-used type synonym. > ----------------------------- I can't comment on this one. _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
