[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

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 <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 mailing list