darcs

Issue 2212 pending incorrectly being updated with extra changes upon darcs add

Title pending incorrectly being updated with extra changes upon darcs add
Priority bug Status needs-implementation
Milestone Resolved in
Superseder Nosy List galbolle, owst
Assigned To
Topics ThePendingPatch

Created on 2012-07-18.20:51:08 by owst, last changed 2012-11-08.08:39:10 by galbolle.

Files
File name Uploaded Type Edit Remove
generalize-gencommutewhatwecanrl_-allow-heterogenous-commute_-add-gen___fl.dpatch galbolle, 2012-11-05.19:59:23 application/octet-stream
generalize-gencommutewhatwecanrl_-allow-heterogenous-commute_-add-gen___fl.dpatch owst, 2012-11-06.14:37:06 application/octet-stream
wip_-issue2212.dpatch kowey, 2012-08-12.17:20:52 application/octet-stream
Messages
msg15886 (view) Author: owst Date: 2012-07-18.20:51:07
1. Summarise the issue (what were doing, what went wrong?)

These steps:

$ darcs init --repo R
$ cd R
 
$ touch a b
$ darcs add a
$ darcs rec -am 'Add a'
 
$ rm a
$ darcs add b
$ cat _darcs/patches/pending
{
rmfile ./a
addfile ./b
}

Why has a been removed, in pending? 

2. What behaviour were you expecting instead?

Darcs shouldn't be changing pending for files if I didn't tell it to. I
would
expect the addfile ./b, but not the rmfile ./a

3. What darcs version are you using? (Try: darcs --exact-version)

2.9.1 (+ 533 patches)

4. What operating system are you running?

Arch x86_64
msg15918 (view) Author: owst Date: 2012-07-23.16:20:09
This is caused by `addToPending p` doing more than just adding patch p
to pending: it reads the unrecorded state of the repo (presumably, with
some filtering out of hunks - I don't remember), and adds p to that
state. So, we end up with the change to a being included in pending. 

I attempted to only look at the unrecorded state for filepaths mentioned
in p, but that caused a bunch of test failures when the index is updated.

I'll need to do more investigating.
msg15939 (view) Author: owst Date: 2012-08-03.09:32:53
Another script to show this issue:
#!/bin/bash

rm -rf R

darcs init --repo R

cd R

mkdir foo

for f in {1..20}
do
echo -e 'line1\nline2\nline3' > "foo/file$f"
done

darcs rec -alm 'add files'

mv foo bar

touch newfile

darcs add newfile

# Hmm, we've forced all those changes into pending!
[[ $(cat _darcs/patches/pending | grep -c line) -eq 60 ]]
msg15957 (view) Author: kowey Date: 2012-08-09.10:05:52
Hey Owen, what exactly was that change you tried to make?  Was it 
changing unrecordedState itself, or just passing it Just some-subpaths, 
or something else?
msg15958 (view) Author: kowey Date: 2012-08-09.14:25:14
I have an inkling that this may be a regression introduced by the 
issue1825 fix
msg15971 (view) Author: kowey Date: 2012-08-12.17:20:52
I think I have a rough idea how to fix this, but I don't know how to 
write it yet:

17:39:18 - kowey: the issue (as I understand it) is basically for the 
repo-local operations, we recompute pending pretty much from scratch
17:39:35 - kowey: by doing (sift (pending +>+ working +>+ newp))
17:39:50 - kowey: so what I'm doing now is trying to push newp as far 
back working as we can
17:39:53 - kowey: and dump the excess

http://irclog.perlgeek.de/darcs/2012-08-12#i_5893336

Where I'm stuck is that I don't know how to express the idea: given list 
xs +>+ ys, push ys as far back as possible and dump the stuff that comes 
after it.  Am I going to have to write something based on 
commuteWhatWeCanRL?

Giving up for now.  (Feel free to take over; what I've started is 
attached)
Attachments
msg16291 (view) Author: galbolle Date: 2012-11-05.19:59:23
This bundle compiles and fixes issue 2212, but it fails
tests/issue1332_add_r_boring.sh. Feel free to look at that if you know
what it's about.
Attachments
msg16300 (view) Author: owst Date: 2012-11-06.12:27:04
issue1332 was related to whether or not darcs should add boring files
recursively. The old test passed since it didn't properly invalidate the
index (to observe: add a call to whatsnew after the pending patch is
removed) If `darcs rev -a` is called instead of `rm
_darcs/patches/pending` then the test fails with & without this patch.

That said, there's something fishy with this patch's effects, if the
pending patch is nuked:

> darcs init --repo R
> cd R
> touch f

> darcs add f
> darcs wh

addfile ./f

> rm _darcs/patches/pending

> darcs add f
> darcs wh

addfile ./f
addfile ./f

Is this something to do with the index? The output is not duplicated if
I also `touch _darcs/index_invalid`
msg16302 (view) Author: galbolle Date: 2012-11-06.13:00:13
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Le mar. 06 nov. 2012 13:27:05 CET, Owen Stephens a écrit :
>
> Owen Stephens <darcs@owenstephens.co.uk> added the comment:
>
> issue1332 was related to whether or not darcs should add boring files
> recursively. The old test passed since it didn't properly invalidate the
> index (to observe: add a call to whatsnew after the pending patch is
> removed) If `darcs rev -a` is called instead of `rm
> _darcs/patches/pending` then the test fails with & without this patch.
>
> That said, there's something fishy with this patch's effects, if the
> pending patch is nuked:
>
>> darcs init --repo R
>> cd R
>> touch f
>
>> darcs add f
>> darcs wh
>
> addfile ./f
>
>> rm _darcs/patches/pending
>
>> darcs add f
>> darcs wh
>
> addfile ./f
> addfile ./f
>
> Is this something to do with the index? The output is not duplicated if
> I also `touch _darcs/index_invalid`
>
Yes, I didn't do anything about the index, that's probably wrong. Can 
you write a test for that?

Thanks,

- --
Florent
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlCZCGUACgkQTCPcDztjGo49AACeIZ2yeaumFSuHOS0jNOY1N9dO
SYoAnjD09xAN3gWvuoUsp8IcDwg+Fhn7
=Mqad
-----END PGP SIGNATURE-----
msg16303 (view) Author: owst Date: 2012-11-06.14:37:06
Attaching an updated bundle containing a test for the index issue.
Attachments
msg16312 (view) Author: galbolle Date: 2012-11-08.08:39:09
It seems that removing _darcs/patches/pending is now an unsafe way to
undo an add if you've run one of the index-updating commands, for
instance whatsnew. whatsnew will see the index is invalid after add, and
rebuild it. Then, removing pending will make it invalid again, but it's
not notified. I'm tempted to file that under "don't do that, then".
History
Date User Action Args
2012-07-18 20:51:08owstcreate
2012-07-23 16:20:10owstsetmessages: + msg15918
2012-08-02 09:28:18koweysettopic: + ThePendingPatch
2012-08-02 15:58:04koweysetpriority: bug
status: unknown -> needs-diagnosis/design
2012-08-03 09:32:55owstsetmessages: + msg15939
2012-08-09 10:05:53koweysetmessages: + msg15957
2012-08-09 14:25:15koweysetmessages: + msg15958
2012-08-12 17:20:54koweysetstatus: needs-diagnosis/design -> needs-implementation
files: + wip_-issue2212.dpatch
messages: + msg15971
2012-11-05 19:59:25galbollesetfiles: + generalize-gencommutewhatwecanrl_-allow-heterogenous-commute_-add-gen___fl.dpatch
messages: + msg16291
2012-11-05 19:59:48galbollesetnosy: + galbolle
2012-11-06 12:27:05owstsetmessages: + msg16300
2012-11-06 13:00:14galbollesetmessages: + msg16302
2012-11-06 14:37:08owstsetfiles: + generalize-gencommutewhatwecanrl_-allow-heterogenous-commute_-add-gen___fl.dpatch
messages: + msg16303
2012-11-08 08:39:10galbollesetmessages: + msg16312