Jason / Kamil, sorry for the delay on this one. Everything below is nitpicking; the patch can be applied as-is, except possibly for the last comment (re "exit 200") in the second patch.
Kamil Dworakowski <[email protected]> writes: > +## Copyright (C) YEAR Kamil Dworakowski Needs s/YEAR/2009/. > +darcs add --repo R R/foo > +darcs record --repo R -lam 'foo' Either remove the add line, or the l in -lam; they do the same thing. > +echo "test exit 1" >> R/_darcs/prefs/prefs Better to use "darcs setpref"? Also, we seem to prefer "false" to "exit 1". > +echo "this change should stay uncommitted" >> R/foo > +echo 'y' | not darcs amend --repo R -am 'change everything' R/foo The use of amend (instead of record) confuses me. Is its use here significant? FYI, in bash you can say "darcs <<<y" instead of "echo y | darcs". > +# if tentative state was not cleared, the previuos changes Typo: previous. > +# from failed transaction would piggy back on the next > +echo "xx" >> R/bar > +echo 'y' | darcs amend --repo R -am 'bar2' --no-test R/bar For symmetry, I would have done "setpref test true" instead of --no-test here, but this way is also fine. So this point here is the crux of the test. If the tentative state isn't cleared (the bug), then the change to R/foo would be included in the above amendment on R/bar. > +# should have uncommitted changes > +darcs wh --repo R | grep "this change should stay uncommitted" As a minor style point, the line above won't notice if its Darcs datasource exits unsuccessfully. This will detect failure in either the source or sink: darcs wh --repo R >log grep "this ..." log > ] move ./tests/failing-issue1406.sh ./tests/issue1406.sh > hunk ./tests/issue1406.sh 2 > #!/bin/sh > +set -ev It would be nice if you added the synopsis and license declaration here, as seen in EXAMPLE.sh. You can find infer the copyright holders by running "darcs changes" on the file. > hunk ./tests/issue1406.sh 15 > -echo "y" | darcs amend-record --all --patch=p1 > +echo "y" | not darcs amend-record --all --patch=p1 Not your bug, but I'm puzzled why "y" needs to be supplied when --all is used. Looks like the "not" you added fixes the old semantics. > +pwd Unnecessary? > +tar xf ../tests/repos/old-with-checkpoint.tgz I think "tar zxf" is needed for portability. > +echo 'y' | not darcs amend --repo old-with-checkpoint -m 'no longer a > checkpoint :P' -p "checkpoint here" > + > +# check that the patch has not been removed from > +# the checkpoints inventory > +grep "checkpoint here" old-with-checkpoint/_darcs/checkpoints/inventory > +#NOTE: checking the file is a rather white box way of testing it, but > +# I could not think of any other way at the time, please improve > + > +rm -rf old-with-checkpint AIUI checkpoints are only relevant to old-fashioned-inventory repos. Therefore use "exit 200" (skip test) when in hashed/darcs-2 format? As for the actual test, I don't know enough about checkpointing to know if it's doing the Right Thing. > *000000000000000000000000000000000000000000000000000000000000000000000000000000 > [repeats 162 times] Say Eric, can we optimize this kind of thing away in Darcs 3? Surely base64, at least, is our friend. > [resolve issue1406: amend-record unrecords on a test failure > Kamil Dworakowski <[email protected]>**20090902221053 > Ignore-this: f137023c7379af697dcda9f41ff6a61f > > A failing test on amend no longer unrecords the original patch. The > failure manifested itself only on old-fashioned repositories. Change > DarcsRepo.remove_from_tentative_inventory not to modify the pristine > nor the inventory. > > ] AIUI reviewing this part of the bundle is beyond my scope. _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
