[darcs-users] darcs patch: simplify cleanup in overriding-defaults ... (and 5 more)

Trent W. Buck trentbuck at gmail.com
Thu Mar 19 07:11:55 UTC 2009


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 <kowey at darcs.net> 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 <droundy at darcs.net>**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 <droundy at darcs.net>**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.



More information about the darcs-users mailing list