[darcs-users] darcs patch: test that tentative leftover is cleared (and 2 more)
Trent W. Buck
twb at cybersource.com.au
Fri Sep 11 07:01:50 UTC 2009
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 <kamil at dworakowski.name> writes:
> +## Copyright (C) YEAR Kamil Dworakowski
> +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
FYI, in bash you can say "darcs <<<y" instead of "echo y | darcs".
> +# if tentative state was not cleared, the previuos changes
> +# 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
> +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
Looks like the "not" you added fixes the old semantics.
> +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.
> [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 <kamil at dworakowski.name>**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.
More information about the darcs-users