darcs

Issue 1825 Invalid pending on revert

Title Invalid pending on revert
Priority urgent Status resolved
Milestone 2.5.0 Resolved in 2.5.0
Superseder Nosy List darcs-devel, dmitry.kurochkin, kowey, mornfall, tux_rocker, wferi
Assigned To mornfall
Topics ThePendingPatch

Created on 2010-04-20.10:18:36 by wferi, last changed 2012-08-12.13:33:50 by kowey.

Files
File name Uploaded Type Edit Remove
pending wferi, 2010-04-27.17:56:45 application/octet-stream
pending_bug wferi, 2010-04-29.22:05:55 application/octet-stream
pending_buggy wferi, 2010-04-20.10:18:35 application/octet-stream
revert_test1 wferi, 2010-04-27.18:21:25 application/octet-stream
Messages
msg10775 (view) Author: wferi Date: 2010-04-20.10:18:35
wferi@n0:~/nfsroot$ darcs --version
2.4.1 (release)
wferi@n0:~/nfsroot$ darcs revert -a ./root/etc/rcS.d ./root/etc/init.d/clgr
Reverting changes in "root/etc/init.d/clgr" "root/etc/rcS.d"..

darcs: bug at src/Darcs/Repository/Internal.hs:339 compiled Apr 14 2010
12:05:41
There was an attempt to write an invalid pending!
If possible, please send the contents of _darcs/patches/pending_buggy
along with a bug report.
See http://wiki.darcs.net/index.html/BugTrackerHowto for help on bug
reporting.
Attachments
msg10835 (view) Author: kowey Date: 2010-04-26.19:26:33
Pending patch bugs should be taken *very* seriously (they can create
havoc further down the line in the event that darcs fails to notice them
on record time).

Ferenc, do you have any way of reproducing the error?

A good move would be for us to study the pending patch and figure out
what is wrong with it (which may give us an idea how it got created). 
The revert is a good clue.  The adddirs and moves may be worth noting.

Ferenc: if you need a work around, I think you can (after making a
backup for forensics) delete the pending patch and your index file.
msg10861 (view) Author: wferi Date: 2010-04-27.17:13:47
Eric Kow <bugs@darcs.net> writes:

> Eric Kow <kowey@darcs.net> added the comment:
>
> Pending patch bugs should be taken *very* seriously (they can create
> havoc further down the line in the event that darcs fails to notice them
> on record time).
>
> Ferenc, do you have any way of reproducing the error?

I copied the repository, it reproduces the error message at the given
command.  I didn't try to do it from darcs init, though.

> A good move would be for us to study the pending patch and figure out
> what is wrong with it (which may give us an idea how it got created). 
> The revert is a good clue.  The adddirs and moves may be worth noting.
>
> Ferenc: if you need a work around, I think you can (after making a
> backup for forensics) delete the pending patch and your index file.

I didn't get stuck, did some records instead and the error disappeared.
Your starting paragraph gives me strange feelings, though...
-- 
Regards,
Feri.
msg10862 (view) Author: kowey Date: 2010-04-27.17:17:22
On Tue, Apr 27, 2010 at 19:13:08 +0200, Ferenc Wagner wrote:
> I didn't get stuck, did some records instead and the error disappeared.
> Your starting paragraph gives me strange feelings, though...

I'd do a darcs check to be sure.

I suspect Darcs didn't try to use the buggy pending from revert.
Since you have a copy of the repository, you can look at the
pending files and compare.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
msg10864 (view) Author: wferi Date: 2010-04-27.17:56:45
_darcs/patches/pending is not touched by the revert command.
Attachments
msg10865 (view) Author: wferi Date: 2010-04-27.18:00:40
darcs revert -a ./root/etc/rcS.d succeeds, but after that darcs revert
-a ./root/etc/init.d/clgr gives the same error as the original command.
msg10866 (view) Author: wferi Date: 2010-04-27.18:21:25
I lost control here. Why does darcs think I don't want to revert after
all?  Output:

$ ./revert_test1
/tmp/tmp.WSJJvaMHnA
Recording changes in "a/b/c":

Finished recording patch 'levels'
Reverting changes in "a/b/c"..

If you don't want to revert after all, that's fine with me!
Finished reverting.

On the other hand, darcs revert -a a/b works and reverts a/b/c as well.
Somebody is confused by the nesting here...
Attachments
msg10868 (view) Author: kowey Date: 2010-04-27.19:13:28
On Tue, Apr 27, 2010 at 18:21:26 +0000, Ferenc Wágner wrote:
> I lost control here. Why does darcs think I don't want to revert after
> all?  Output:

It could be darcs revert being confused by a/b/c not being in the
working directory.  Maybe darcs remove without actually removing
the file will have a different effect

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
msg10880 (view) Author: wferi Date: 2010-04-29.22:05:55
I attach a script (pending_bug), which reproduces the error under darcs
2.4.1.
Attachments
msg10916 (view) Author: wferi Date: 2010-05-03.15:49:06
I can't really do more about this, I'm afraid.
msg10941 (view) Author: kowey Date: 2010-05-05.14:54:31
Test case submitted (patch233).  Beautiful and minimal.  

We now need somebody to stare at the Darcs code and figure out what the
heck is happening.  I recommend we do this by the 2010-10 hacking sprint
at the latest.
msg11609 (view) Author: tux_rocker Date: 2010-06-27.19:22:38
I have trouble debugging this because there are no Show or Show2
instances for the datatypes used in the SelectChanges code, and the
witnesses stuff makes it hard to make them.
msg11635 (view) Author: tux_rocker Date: 2010-06-28.20:23:57
mornfall already fixed it before I had even finished telling him about
the issue on IRC. So assigning to him.
msg11681 (view) Author: mornfall Date: 2010-07-05.21:19:40
The following patch updated issue issue1825 with status=resolved;resolvedin=2.6.0 HEAD

* Resolve issue1825: do not omit important prims in unrecordedChanges w/ files. 
Ignore-this: 6948f29fe09882ec719269b21fc50ae7

This is a rather heavy-handed fix. We stop filtering the pending patch in
unrecordedChanges -- overall, this is an optimisation, since we can now avoid
reading the pending patch twice, and in most cases we also avoid filtering that
part of the patch twice. However, in exchange, whatsnew now has to filter the
patch (this was not required previously). This may be slightly less efficient
for diffs with very many changed files: however, it is unlikely to be a
bottleneck, since computing and even formatting the patches for display is much
more expensive.
msg15969 (view) Author: kowey Date: 2012-08-12.13:17:31
Historical note (I'm trawling through pending bugs trying to figure out 
how stuff works): Petr's patch292 was reviewed by Ganesh (thanks for 
that!).  Part of the reason I'm doing this is because I suspect that 
this change has something to do with issue2212

The basic story as I understand it is that we would sometimes get 
invalid pending (can't be applied to pristine) when passing a set of 
files to restrict operations like revert.  The reason seems to be that 
we do a diff between (pristine + pending) vs working and return the 
resulting (pending +>+ diff).

If pending is restricted to only the patches that touch a subset of 
files, but the diff is not; the patch can be invalid.  The diff would be 
in the wrong context as it may contain says hunks for files we weren't 
looking at.

I think since the time of this patch, we've started to use the 
unrecordedChanges function in more places like darcs add (or was it 
always there?).  Now our struggle is that the pending patch contains 
“too much” stuff.  See issue2212
msg15970 (view) Author: kowey Date: 2012-08-12.13:33:44
Feh, I forgot to the important bit, which is that the fix is essentially 
to move the filtering of patches (to see if they touch the file in 
question) out of `unrecordedChanges`.  The `revert` command already does 
it own touch filtering; however other commands which use this function 
would then need to do filtering of their own, as a post-processing step.  
Only whatsnew was updated accordingly.
History
Date User Action Args
2010-04-20 10:18:36wfericreate
2010-04-26 19:26:35koweysetstatus: unknown -> waiting-for
priority: bug -> urgent
nosy: + kowey
messages: + msg10835
topic: + ThePendingPatch
assignedto: wferi
2010-04-27 17:13:47wferisetmessages: + msg10861
2010-04-27 17:17:23koweysetmessages: + msg10862
2010-04-27 17:56:45wferisetfiles: + pending
messages: + msg10864
2010-04-27 18:00:41wferisetmessages: + msg10865
2010-04-27 18:21:26wferisetfiles: + revert_test1
messages: + msg10866
2010-04-27 19:13:29koweysetmessages: + msg10868
2010-04-29 22:05:56wferisetfiles: + pending_bug
messages: + msg10880
2010-05-03 15:49:06wferisetassignedto: wferi -> (no value)
messages: + msg10916
2010-05-05 14:54:33koweysetstatus: waiting-for -> needs-reproduction
topic: + Target-2.5
messages: + msg10941
2010-06-15 20:52:13adminsetmilestone: 2.5.0
2010-06-15 20:59:56adminsettopic: - Target-2.5
2010-06-21 19:17:42tux_rockersetassignedto: tux_rocker
nosy: + tux_rocker
2010-06-27 19:22:39tux_rockersetmessages: + msg11609
2010-06-28 20:23:58tux_rockersetassignedto: tux_rocker -> mornfall
messages: + msg11635
nosy: + mornfall
2010-07-04 18:38:54ganeshlinkpatch292 issues
2010-07-05 21:19:41mornfallsetstatus: needs-reproduction -> resolved
messages: + msg11681
resolvedin: 2.8.0
2010-07-23 19:17:46tux_rockersetresolvedin: 2.8.0 -> 2.5.0
2012-08-12 13:17:34koweysetstatus: resolved -> unknown
messages: + msg15969
2012-08-12 13:18:19koweysetstatus: unknown -> resolved
2012-08-12 13:33:45koweysetstatus: resolved -> unknown
messages: + msg15970
2012-08-12 13:33:50koweysetstatus: unknown -> resolved