darcs

Issue 1829 inconsistent patch

Title inconsistent patch
Priority urgent Status needs-diagnosis/design
Milestone Resolved in
Superseder Nosy List Serware, darcs-devel, dmitry.kurochkin, ganesh, igloo, kowey
Assigned To
Topics Conflicts, Darcs2

Created on 2010-04-23.11:54:27 by kowey, last changed 2018-04-25.16:23:45 by bfrk.

Files
File name Uploaded Type Edit Remove
base-context kowey, 2010-04-23.11:54:24 application/octet-stream
camp2.sh igloo, 2010-08-06.12:51:05 text/x-sh
cutdown6.tar.bz2 ganesh, 2010-08-05.21:06:31 application/x-bzip
fiddle.sh igloo, 2010-08-11.03:00:46 text/x-sh
pull-context kowey, 2010-04-23.11:54:37 application/octet-stream
run2.sh igloo, 2010-08-06.12:15:35 text/x-sh
shrunk1.dpatch ganesh, 2010-08-06.04:38:32 application/octet-stream
shrunk2.dpatch ganesh, 2010-08-06.04:38:45 application/octet-stream
Messages
msg10795 (view) Author: kowey Date: 2010-04-23.11:54:24
I guess we should be happy to have something open and easily reproducible.  

Defaulting to urgent as it's keep down and scary looking (although it
may not actually be urgent in the sense of us being able to act on it
right now)

We get this trying to pull from the darcs-2.4.1 branch into darcs HEAD
context files attached.

Next action required is to boil this down into a minimal case, probably
via http://wiki.darcs.net/Forensics

Shall I pull this patch? (9/9)  [ynWvplxdaqjk], or ? for help: y
darcs: Inconsistent patch:
conflictor {{
|hunk ./src/Darcs/Commands/Remove.lhs 118
|-              (Just (SubTree _), Just (SubTree _)) ->
|-                  return . Sealed $ rmdir f_fp :>: rest
|+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
|+                  if not $ null (list unrecordedChildren)
|+                    then skipAndWarn "it is not empty"
|+                    else return . Sealed $ rmdir f_fp :>: rest
|:
hunk ./src/Darcs/Commands/Remove.lhs 121
-                    else return . Sealed $ rmdir f_fp :>: rest
+                    else return $ Just (rmdir f_fp :>: NilFL)
|:
hunk ./src/Darcs/Commands/Remove.lhs 115
-                       return $ Sealed rest
+                       return $ Nothing
|:
hunk ./src/Darcs/Commands/Remove.lhs 110
-            Sealed rest <- mrp ftf recorded' unrecorded' fs
-            let f_fp = anchorPath "" f
+                f_fp = anchorPath "" f
|hunk ./src/Darcs/Commands/Remove.lhs 110
|-            Sealed rest <- mrp ftf recorded' unrecorded' fs
|+            rest <- mrp ftf recorded' unrecorded' fs
|:
hunk ./src/Darcs/Commands/Remove.lhs 110
-            rest <- mrp ftf recorded' unrecorded' fs
|:
hunk ./src/Darcs/Commands/Remove.lhs 110
-            Sealed rest <- mrp ftf recorded' unrecorded' fs
+            rest <- mrp ftf recorded' unrecorded' fs
|:
hunk ./src/Darcs/Commands/Remove.lhs 115
-                       return $ Sealed rest
+                       return $ emptyGap NilFL
|hunk ./src/Darcs/Commands/Remove.lhs 115
|-                       return $ Sealed rest
|+                       return $ emptyGap NilFL
|:
hunk ./src/Darcs/Commands/Remove.lhs 115
-                       return $ emptyGap NilFL
+                       return $ Nothing
|:
hunk ./src/Darcs/Commands/Remove.lhs 119
-                  return . Sealed $ rmdir f_fp :>: rest
+                  return $ freeGap (rmdir f_fp :>: NilFL)
|:
hunk ./src/Darcs/Commands/Remove.lhs 118
-              (Just (SubTree _), Just (SubTree _)) ->
-                  return . Sealed $ rmdir f_fp :>: rest
+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
+                  if not $ null (list unrecordedChildren)
+                    then skipAndWarn "it is not empty"
+                    else return . Sealed $ rmdir f_fp :>: rest
|hunk ./src/Darcs/Commands/Remove.lhs 119
|-                  return . Sealed $ rmdir f_fp :>: rest
|+                  return $ freeGap (rmdir f_fp :>: NilFL)
|conflictor [
|hunk ./src/Darcs/Commands/Remove.lhs 119
|-                  return . Sealed $ rmdir f_fp :>: rest
|+                  return $ freeGap (rmdir f_fp :>: NilFL)
|]
||:
|hunk ./src/Darcs/Commands/Remove.lhs 118
|-              (Just (SubTree _), Just (SubTree _)) ->
|-                  return . Sealed $ rmdir f_fp :>: rest
|+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
|+                  if not $ null (list unrecordedChildren)
|+                    then skipAndWarn "it is not empty"
|+                    else return . Sealed $ rmdir f_fp :>: rest
|:
hunk ./src/Darcs/Commands/Remove.lhs 118
-              (Just (SubTree _), Just (SubTree _)) ->
-                  return . Sealed $ rmdir f_fp :>: rest
+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
+                  if not $ null (list unrecordedChildren)
+                    then skipAndWarn "it is not empty"
+                    else return $ freeGap (rmdir f_fp :>: NilFL)
|hunk ./src/Darcs/Commands/Remove.lhs 119
|-                  return . Sealed $ rmdir f_fp :>: rest
|+                  return $ freeGap (rmdir f_fp :>: NilFL)
|conflictor [
|hunk ./src/Darcs/Commands/Remove.lhs 119
|-                  return . Sealed $ rmdir f_fp :>: rest
|+                  return $ freeGap (rmdir f_fp :>: NilFL)
|]
||:
|hunk ./src/Darcs/Commands/Remove.lhs 118
|-              (Just (SubTree _), Just (SubTree _)) ->
|-                  return . Sealed $ rmdir f_fp :>: rest
|+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
|+                  if not $ null (list unrecordedChildren)
|+                    then skipAndWarn "it is not empty"
|+                    else return . Sealed $ rmdir f_fp :>: rest
|hunk ./src/Darcs/Commands/Remove.lhs 118
|-              (Just (SubTree _), Just (SubTree _)) ->
|-                  return . Sealed $ rmdir f_fp :>: rest
|+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
|+                  if not $ null (list unrecordedChildren)
|+                    then skipAndWarn "it is not empty"
|+                    else return $ freeGap (rmdir f_fp :>: NilFL)
|:
hunk ./src/Darcs/Commands/Remove.lhs 121
-                    else return $ freeGap (rmdir f_fp :>: NilFL)
+                    else return $ Just $ freeGap (rmdir f_fp :>: NilFL)
}} []
|hunk ./src/Darcs/Commands/Remove.lhs 118
|-              (Just (SubTree _), Just (SubTree _)) ->
|-                  return . Sealed $ rmdir f_fp :>: rest
|+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
|+                  if not $ null (list unrecordedChildren)
|+                    then skipAndWarn "it is not empty"
|+                    else return $ freeGap (rmdir f_fp :>: NilFL)
|hunk ./src/Darcs/Commands/Remove.lhs 121
|-                    else return $ freeGap (rmdir f_fp :>: NilFL)
|+                    else return $ Just $ freeGap (rmdir f_fp :>: NilFL)
|hunk ./src/Darcs/Commands/Remove.lhs 115
|-                       return $ Sealed rest
|+                       return $ emptyGap NilFL
|hunk ./src/Darcs/Commands/Remove.lhs 115
|-                       return $ emptyGap NilFL
|+                       return $ Nothing
|hunk ./src/Darcs/Commands/Remove.lhs 110
|-            Sealed rest <- mrp ftf recorded' unrecorded' fs
|+            rest <- mrp ftf recorded' unrecorded' fs
|hunk ./src/Darcs/Commands/Remove.lhs 110
|-            rest <- mrp ftf recorded' unrecorded' fs
|:
hunk ./src/Darcs/Commands/Remove.lhs 110
-            let f_fp = anchorPath "" f
-                skipAndWarn reason =
-                    do putWarning opts . text $ "Can't remove " ++ f_fp
-                                                ++ " (" ++ reason ++ ")"
-                       return $ Nothing
-
-            local <- case (find recorded f, find unrecorded f) of
-              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
-                  if not $ null (list unrecordedChildren)
-                    then skipAndWarn "it is not empty"
-                    else return $ Just $ freeGap (rmdir f_fp :>: NilFL)
-              (Just (File _), Just (File _)) ->
-                  Just `fmap` treeDiff ftf unrecorded unrecorded'
-              (Just (File _), _) ->
-                  return $ Just $ freeGap (addfile f_fp :>: rmfile f_fp
:>: NilFL)
-              (Just (SubTree _), _) ->
-                  return  $ Just $ freeGap (adddir f_fp :>: rmdir f_fp
:>: NilFL)
-              (_, _) -> skipAndWarn "it is not tracked by darcs"
-                            $
-              -- we can tell if the remove succeeded by looking if local is
-              -- empty. If the remove succeeded, we should pass on updated
-              -- recorded and unrecorded that reflect the removal
-            let nextRecorded | isJust local = recorded'
-                             | otherwise    = recorded
-                nextUnrecorded | isJust local = unrecorded'
-                               | otherwise    = unrecorded
-
-            rest <- mrp ftf nextRecorded nextUnrecorded fs
+            local <- makeRemoveGap opts ftf recorded unrecorded
unrecorded' f
+            -- we can tell if the remove succeeded by looking if local is
+            -- empty. If the remove succeeded, we should pass on updated
+            -- recorded and unrecorded that reflect the removal
dependencies not in conflict:
{{
|:
hunk ./src/Darcs/Commands/Remove.lhs 118
-              (Just (SubTree _), Just (SubTree _)) ->
-                  return . Sealed $ rmdir f_fp :>: rest
+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
+                  if not $ null (list unrecordedChildren)
+                    then skipAndWarn "it is not empty"
+                    else return $ freeGap (rmdir f_fp :>: NilFL)
|hunk ./src/Darcs/Commands/Remove.lhs 118
|-              (Just (SubTree _), Just (SubTree _)) ->
|-                  return . Sealed $ rmdir f_fp :>: rest
|+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
|+                  if not $ null (list unrecordedChildren)
|+                    then skipAndWarn "it is not empty"
|+                    else return $ freeGap (rmdir f_fp :>: NilFL)
|:
hunk ./src/Darcs/Commands/Remove.lhs 121
-                    else return $ freeGap (rmdir f_fp :>: NilFL)
+                    else return $ Just $ freeGap (rmdir f_fp :>: NilFL)
|:
hunk ./src/Darcs/Commands/Remove.lhs 115
-                       return $ Sealed rest
+                       return $ emptyGap NilFL
|hunk ./src/Darcs/Commands/Remove.lhs 115
|-                       return $ Sealed rest
|+                       return $ emptyGap NilFL
|:
hunk ./src/Darcs/Commands/Remove.lhs 115
-                       return $ emptyGap NilFL
+                       return $ Nothing
|:
hunk ./src/Darcs/Commands/Remove.lhs 110
-            Sealed rest <- mrp ftf recorded' unrecorded' fs
+            rest <- mrp ftf recorded' unrecorded' fs
|hunk ./src/Darcs/Commands/Remove.lhs 110
|-            Sealed rest <- mrp ftf recorded' unrecorded' fs
|+            rest <- mrp ftf recorded' unrecorded' fs
|:
hunk ./src/Darcs/Commands/Remove.lhs 110
-            rest <- mrp ftf recorded' unrecorded' fs
}}
compared with deps itself:
hunk ./src/Darcs/Commands/Remove.lhs 118
-              (Just (SubTree _), Just (SubTree _)) ->
-                  return . Sealed $ rmdir f_fp :>: rest
+              (Just (SubTree _), Just (SubTree unrecordedChildren)) -> do
+                  if not $ null (list unrecordedChildren)
+                    then skipAndWarn "it is not empty"
+                    else return $ freeGap (rmdir f_fp :>: NilFL)
hunk ./src/Darcs/Commands/Remove.lhs 121
-                    else return $ freeGap (rmdir f_fp :>: NilFL)
+                    else return $ Just $ freeGap (rmdir f_fp :>: NilFL)
hunk ./src/Darcs/Commands/Remove.lhs 115
-                       return $ Sealed rest
+                       return $ emptyGap NilFL
hunk ./src/Darcs/Commands/Remove.lhs 115
-                       return $ emptyGap NilFL
+                       return $ Nothing
hunk ./src/Darcs/Commands/Remove.lhs 110
-            Sealed rest <- mrp ftf recorded' unrecorded' fs
+            rest <- mrp ftf recorded' unrecorded' fs
hunk ./src/Darcs/Commands/Remove.lhs 110
-            rest <- mrp ftf recorded' unrecorded' fs
Attachments
msg11797 (view) Author: ganesh Date: 2010-07-18.19:14:59
Here's cut-down context files. Not sure that they really help the 
situation but I spent quite a lot of CPU time producing them so thought I 
might as well record them in case they are any use :-)
Attachments
msg11798 (view) Author: ganesh Date: 2010-07-18.20:24:11
Here's some useful cut down patches. Still haven't tried to cut down the 
real meat of the problem, but most/all of the noise in other files and 
past history is gone now.
msg11981 (view) Author: ganesh Date: 2010-08-05.20:53:06
The cut down version (*3.dpatch) is actually wrong because I left out a 
patch that is in both branches, but is actually part of a conflict in one 
branch and thus must be kept.
msg11982 (view) Author: ganesh Date: 2010-08-05.21:06:31
Here's a valid cutdown version (start a new repo, add 'file' to it, then 
try to apply both .dpatch files).

I've written a test case shrinker which is currently beavering away at 
making an even more cut down version. I'll post anything useful once I 
have it.
Attachments
msg11984 (view) Author: ganesh Date: 2010-08-06.04:38:32
Here's the result of my shrinker - each patch is down to a couple of 
lines. It may well have missed further improvements (e.g. merging 
successive patches) but it's pretty small now.
Attachments
msg11998 (view) Author: igloo Date: 2010-08-06.12:15:35
Script for reproduction attached.
Attachments
msg12002 (view) Author: igloo Date: 2010-08-06.12:51:05
Attached a version of the script which also works with camp.
Interestingly, this script also causes a panic in camp:

+ camp pull ../r2 -a
camp: Panic! Got an unhandled exception!
Exception details:
Can't happen: Commute Catch Catch 1

This means we think we are in this commute case:
    p [p^, {:p}, :q] <-> q [q^, {:q}, :p]
which in code looks like
    commute (Patch p `Then` Conflictor qEffect [qConflict] qIdentity)
     | name p == name1 qConflict
but qIdentity has non-empty context.

This needs someone to sit down and work through what's going on;
unlikely to be me this month.
Attachments
msg12022 (view) Author: igloo Date: 2010-08-07.14:09:45
Oh, camp has a comment
-- XXX Needs to handle the p p^ case
in the code that handles adding a patch to a context, and the catch that
causes the panic has a context of p p^, so once that's implemented I
expect this would actually work in camp. I'll try to get around to
implementing and trying it.
msg12106 (view) Author: igloo Date: 2010-08-11.03:00:46
OK, camp now accepts the testcase, so there is hope!

I've attached fiddle.sh, in which I tried to make the testcase a little
simpler. Re-laying-out the patch it complains about (a more readable
format by default would be very useful!):

conflictor {{
    |hunk ./file 2
    |+Line DDDD
    |:
    hunk ./file 2
    +Line CCC

    |:
    hunk ./file 2
    +Line TTTTTTT

    |:
    hunk ./file 2
    +Line DDDD

    |hunk ./file 2
    |+Line TTTTTTT
    |conflictor [
    |    hunk ./file 2
    |    +Line TTTTTTT
    |]
    ||:
    |hunk ./file 2
    |+Line DDDD
    |:
    hunk ./file 2
    +Line XXXXXXXXX
}} 
[
    hunk ./file 1
    +Line A
]
|hunk ./file 2
|+Line XXXXXXXXX
|:
hunk ./file 1
-Line BB

I believe it's complaining that it expects
    hunk ./file 2
    +Line XXXXXXXXX
(which the patch identity depends on) to be in the set of conflicts, but
    |hunk ./file 2
    |+Line TTTTTTT
    |conflictor [
    |    hunk ./file 2
    |    +Line TTTTTTT
    |]
    ||:
    |hunk ./file 2
    |+Line DDDD
    |:
    hunk ./file 2
    +Line XXXXXXXXX
is what is in the set of conflicts.

I don't know why this discrepancy happens; it probably needs someone to
work out exactly what operation is causing the problem, and then to step
through the code for that operation to see what goes wrong.
Attachments
msg12118 (view) Author: kowey Date: 2010-08-11.12:24:55
On Wed, Aug 11, 2010 at 03:00:47 +0000, Ian Lynagh wrote:
> I've attached fiddle.sh, in which I tried to make the testcase a little
> simpler. Re-laying-out the patch it complains about (a more readable
> format by default would be very useful!):

Thanks!  Would it be a lot of trouble for you to patch
tests/failing-issue1829-inconsistent-conflictor.sh and darcs send?

It helps us when we can decentralise as much of this sort of work as
possible.  Of course, I don't mind doing it if it's really too much
to ask :-)

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.
msg12119 (view) Author: igloo Date: 2010-08-11.12:44:40
On Wed, Aug 11, 2010 at 01:27:56PM +0100, Eric Kow wrote:
> On Wed, Aug 11, 2010 at 03:00:47 +0000, Ian Lynagh wrote:
> > I've attached fiddle.sh, in which I tried to make the testcase a little
> > simpler. Re-laying-out the patch it complains about (a more readable
> > format by default would be very useful!):
> 
> Thanks!  Would it be a lot of trouble for you to patch
> tests/failing-issue1829-inconsistent-conflictor.sh and darcs send?

I don't think the testcase needs to be changed.


Thanks
Ian
History
Date User Action Args
2010-04-23 11:54:27koweycreate
2010-04-23 11:54:39koweysetfiles: + pull-context
2010-07-18 15:22:23koweysetstatus: unknown -> needs-reproduction
2010-07-18 19:15:00ganeshsetfiles: + base2-context.bz2
nosy: + ganesh
messages: + msg11797
2010-07-18 19:15:13ganeshsetfiles: + pull2-context.bz2
2010-07-18 20:24:12ganeshsetmessages: + msg11798
2010-07-18 20:24:22ganeshsetfiles: + inter3.dpatch
2010-07-18 20:24:28ganeshsetfiles: + base3.dpatch
2010-07-18 20:24:33ganeshsetfiles: + pull3.dpatch
2010-08-05 20:53:07ganeshsetmessages: + msg11981
2010-08-05 21:04:08ganeshsetfiles: - base3.dpatch
2010-08-05 21:04:32ganeshsetfiles: - base2-context.bz2
2010-08-05 21:04:35ganeshsetfiles: - inter3.dpatch
2010-08-05 21:04:40ganeshsetfiles: - pull2-context.bz2
2010-08-05 21:04:43ganeshsetfiles: - pull3.dpatch
2010-08-05 21:06:31ganeshsetfiles: + cutdown6.tar.bz2
messages: + msg11982
2010-08-06 04:38:33ganeshsetfiles: + shrunk1.dpatch
messages: + msg11984
2010-08-06 04:38:45ganeshsetfiles: + shrunk2.dpatch
2010-08-06 04:39:32ganeshsetnosy: + igloo
2010-08-06 12:15:36igloosetfiles: + run2.sh
messages: + msg11998
2010-08-06 12:51:06igloosetfiles: + camp2.sh
messages: + msg12002
2010-08-07 14:09:46igloosetmessages: + msg12022
2010-08-11 03:00:46igloosetfiles: + fiddle.sh
messages: + msg12106
2010-08-11 12:24:56koweysetmessages: + msg12118
2010-08-11 12:44:40igloosetmessages: + msg12119
2018-04-25 16:23:45bfrksetstatus: needs-reproduction -> needs-diagnosis/design