1. Summarise the issue (what were doing, what went wrong?)

1. I had unrecorded changes.
2. I did a "darcs rebase suspend", which worked fine.
3. I did a "darcs rebase unsuspend", which failed because there were 
unrecored changes. 
4. I had to workaround the issue, by recording a temporary patch, running 
"rebase unsuspend" again, and unrecording my temporary patch.

2. What behaviour were you expecting instead?

If the case can't be handled automatically, "darcs rebase unsuspend could 
offer semi-automation":

 "Unrecorded changes were found in your repo which aren't allowed during 
unsuspend. Would you like me to store them in temporary patch and then 
unrecord them when unsuspend is finished?"

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

2.9.6+22 patches

From IRC:

<owst> markstos: yeah, that sounds like something I'd like, too.
<Heffalump> The current behaviour is conservative and I think others have 
mentioned it too

Would you be ok with it only refusing to unsuspend if there would be a 
conflict with an unrecorded change? I think that's implementable and 
would be symmetric with suspend.

Note to myself: I think obliterate also has this restriction, should 
relax both in the same way.

So, possibilities are:

1. Unrecorded changes can be preserved as-is. Darcs handles this 
automatically.
2. Unrecorded changes conflict with changes to be unsuspended. Darcs 
alerts the user to this and user must fix the conflict before 
proceeding.

That's OK, but I hope there would be way to find out exactly what the 
conflict was, so it could be efficient resolved. Perhaps there should be 
options here:

2a. Refuse to unsuspend, mention --allow-conflicts as a "force" option. 
(default?)
2b. darcs rebase unsuspend --allow-conflicts which proceeds anyway, 
leaving conflict markers.

I've had another look at this and actually I don't see any fundamental 
reason why it can't just behave like pull/apply - i.e. leave conflicts 
in working if necessary (and with options to avoid that if you want).

The only issue is that the code will be a bit complex/fiddly - I think 
the main reason for the current restriction is just a result of me 
trying to keep things simple when I was first implementing unsuspend.

Further notes to self or any other future implementer of this:

 - I was talking nonsense when I said above that obliterate has the same 
restriction. The correspondence is between obliterate and suspend, and 
both refuse to suspend when that would cause a conflict with working. I 
did recently add some diagnostics to suspend to tell you _what_ 
conflicts then and those would make sense to put in obliterate too.

 - Essentially the unsuspend code is the same as 
Darcs.Repository.Merge.tentativelyMergePatches, except that it doesn't 
handle working and it does handle putting the rebase patch back. Need to 
unify the two or less ideally copy and paste more code from 
tentativelyMergePatches.

Thanks for looking at this, Ganesh. I could contribute a shell-test if 
that would be helpful.  (It would check that unsuspending with unrecorded 
changes present succeeds, but with a possible conflict).

A shell-test would be very helpful if you have time, thanks!

I've now created a patch with a failing test for this and sent it to 
patches@darcs.net, as well publishing it in my hub repo:

http://hub.darcs.net/markstos/darcs-screened/patch/20130217024323-8a0ef

If a complete fix which tries to merge whatever the new unrecorded
changes are is not obvious, perhaps a simple fix is possible which at
least would make possible the going-backwards with "unsuspend" in the
case when it is "symmetric" to "suspend":

(I have even looked a bit into the code, but since I'm not familiar with
the code of darcs, I'm not ready yet to modify it.)

the "suspend" was fine if the unrecorded changes didn't intersect with
the suspended patch -- Darcs/UI/Commands/Rebase.hs (line 262):

    doSuspend opts repository qs rOld psToSuspend = do
	pend <- unrecordedChanges (diffingOpts opts) repository Nothing
	FlippedSeal psAfterPending <-
	    let effectPsToSuspend = effect psToSuspend in
	    case commute (effectPsToSuspend :> pend) of
		Just (_ :> res) -> return (FlippedSeal res)
		Nothing -> do
		    ...
		    fail $ "Can't suspend selected patches without reverting some
unrecorded change. Use --verbose to see the details."
    ....
    _ <- applyToWorking repository'' (verbosity opts) (invert
psAfterPending)

Quite clear.

And unsuspend does the much more stupid check at the very beginning
(which is not nice; perhaps, we should just warn, then let the user
select the patches to unsuspend, and then refuse to proceed if the
unrecorded changes conflict with their "effect", like above) --
Darcs/UI/Commands/Rebase.hs (line 413):

    pend <- unrecordedChanges (diffingOpts opts) repository Nothing
    let checkChanges :: FL (PrimOf p) wA wB -> IO (EqCheck wA wB)
        checkChanges NilFL = return IsEq
        checkChanges _ = error "can't unsuspend when there are
unrecorded changes"
    IsEq <- checkChanges pend :: IO (EqCheck wR wU)

and then doesn't check the effects againt `pend` (line 472):

    let effect_to_apply = concatFL (mapFL_FL effect ps_to_unsuspend) +>+
resolved_p
    ...
    _ <- applyToWorking repository''' (verbosity opts) effect_to_apply
`catch` \(e :: IOException) ->

This looks like a place to insert the commutability check between
`effect_to_apply` and `pend` -- like the one in "suspend".

Do you have any comments: can this easily be implemented, can't it?

let effect_to_apply = concatFL (mapFL_FL effect ps_to_unsuspend) +>+
resolved_p

But here I don't yet understand the significance of mapFL_FL and (+>+)
which are not used in the suspend code.

As a first and very simple thing to try to do with the code, I've
factored out the piece of code that deals with unrecorded changes from
"suspend". The patch is below. (It's very simple, just taking out the
piece of code; but the diff got a bit complicated to read.)

BTW, whose darcs repo is best to fork from at hub.darcs.net? I'd like to
save my changes there (to have a backup of my working copy and to
publish them)?

patch 25a4f08b946aea68d646cbb5f40fc6eb6a545943
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date:   Wed Dec 23 14:38:23 MSK 2015
  * Rebase.hs: factored out afterPending from doSuspend
  
  
  I'm preparing a simple fix for issue2272. A similar check is to be
  used for unsuspend, too (instead of simply refusing to proceed if
  there are unrecorded changes).
  
diff -rN -u -d old-darcs/src/Darcs/UI/Commands/Rebase.hs
new-darcs/src/Darcs/UI/Commands/Rebase.hs
--- old-darcs/src/Darcs/UI/Commands/Rebase.hs	2015-12-23
15:37:20.519694154 +0300
+++ new-darcs/src/Darcs/UI/Commands/Rebase.hs	2015-12-23
15:37:20.519694154 +0300
@@ -260,9 +260,22 @@
     -> FL (PatchInfoAnd (Rebasing p)) wX wT
     -> IO (Repository (Rebasing p) wR wU wX)
 doSuspend opts repository qs rOld psToSuspend = do
+    FlippedSeal psAfterPending <- afterPending opts repository psToSuspend
+    rNew <- mkSuspended (mapFL_FL (ToEdit . fmapNamed unNormal .
hopefully) psToSuspend +>+ qs)
+    invalidateIndex repository
+    repository' <- tentativelyRemovePatches repository (compression
opts) YesUpdateWorking (psToSuspend +>+ (rOld :>: NilFL))
+    tentativelyAddToPending repository' YesUpdateWorking $ invert $
effect psToSuspend
+    repository'' <- tentativelyAddPatch repository' (compression opts)
(verbosity opts) YesUpdateWorking (n2pia rNew)
+    _ <- applyToWorking repository'' (verbosity opts) (invert
psAfterPending)
+            `catch` \(e :: IOException) -> fail ("Couldn't undo patch
in working dir.\n" ++ show e)
+    return repository''
+  where unNormal :: Rebasing p wA wB -> p wA wB
+        unNormal (Normal q) = q
+        unNormal (Suspended _) = error "Can't suspend a rebase patch"
+
+afterPending opts repository psToSuspend = do
     pend <- unrecordedChanges (diffingOpts opts) repository Nothing
-    FlippedSeal psAfterPending <-
-        let effectPsToSuspend = effect psToSuspend in
+    let effectPsToSuspend = effect psToSuspend in
         case commute (effectPsToSuspend :> pend) of
             Just (_ :> res) -> return (FlippedSeal res)
             Nothing -> do
@@ -277,20 +290,7 @@
                             showNicely suspendedConflicts $$
                             text "conflict with these local changes:" $$
                             showNicely pendConflicts
-                fail $ "Can't suspend selected patches without
reverting some unrecorded change. Use --verbose to see the details."
-
-
-    rNew <- mkSuspended (mapFL_FL (ToEdit . fmapNamed unNormal .
hopefully) psToSuspend +>+ qs)
-    invalidateIndex repository
-    repository' <- tentativelyRemovePatches repository (compression
opts) YesUpdateWorking (psToSuspend +>+ (rOld :>: NilFL))
-    tentativelyAddToPending repository' YesUpdateWorking $ invert $
effect psToSuspend
-    repository'' <- tentativelyAddPatch repository' (compression opts)
(verbosity opts) YesUpdateWorking (n2pia rNew)
-    _ <- applyToWorking repository'' (verbosity opts) (invert
psAfterPending)
-            `catch` \(e :: IOException) -> fail ("Couldn't undo patch
in working dir.\n" ++ show e)
-    return repository''
-  where unNormal :: Rebasing p wA wB -> p wA wB
-        unNormal (Normal q) = q
-        unNormal (Suspended _) = error "Can't suspend a rebase patch"
+                fail $ "Can't (un)suspend selected patches without
reverting some unrecorded change. Use --verbose to see the details."
 
 unsuspendBasicOpts :: DarcsOption a
                       (Maybe O.AllowConflicts

(Attaching the same patch here for clarity.)

As for my question about the upstream mirror repo for forking off at
hub.darcs.net, I have discovered darcs-reviewed and darcs-screened:

http://hub.darcs.net/darcs/darcs-reviewed/patches
http://hub.darcs.net/darcs/darcs-screened/patches

I'm a bit in doubt which one to fork off (basing on the reviewed one
seems a bit more cleaner, but as I see most people fork off the screened
branch, so I shouldn't break that tradition.)

I've saved my initial patch as
http://hub.darcs.net/imz/darcs_symmetric-unsuspend-over-unrecorded/patch/25a4f08b946aea68d646cbb5f40fc6eb6a545943

Attachments

• darcs-suspend-factorout-unrecorded.diff

On 23/12/2015 09:58, Ivan Zakharyaschev wrote:
> 
> Ivan Zakharyaschev <imz@altlinux.org> added the comment:
> 
> let effect_to_apply = concatFL (mapFL_FL effect ps_to_unsuspend) +>+
> resolved_p
> 
> But here I don't yet understand the significance of mapFL_FL and (+>+)
> which are not used in the suspend code.

FYI these are operations on the "FL" type, which is like lists but for
sequences of patches. I've just started fleshing out the guide to the
"type witnesses" that underpin these:

http://darcs.net/Internals/Witnesses

concatFL/mapFL_FL/(+>+) are equivalent to concat/map/(++).

Cheers,

Ganesh

Hi Ivan,

Thanks a lot for taking a look at this! This should indeed be a
relatively straightforward thing to fix - the main reason it hasn't
happened yet is that I didn't get round to it.

I also had concerns about adding the code inline, so like you, I was
thinking that abstracting things out would be a good idea. Unfortunately
the full story is a bit more complex - see below:

On 23/12/2015 12:33, Ivan Zakharyaschev wrote:
> 
> Ivan Zakharyaschev <imz@altlinux.org> added the comment:
> 
> As a first and very simple thing to try to do with the code, I've
> factored out the piece of code that deals with unrecorded changes from
> "suspend". The patch is below. (It's very simple, just taking out the
> piece of code; but the diff got a bit complicated to read.)

Sadly this isn't actually the right piece of code to reuse for dealing
with unrecorded changes in "unsuspend", and I think that's probably the
root cause of the type errors you ended up with when trying to use it.

The reason is that they are actually two subtly different operations.
When suspending, we are removing patches from the recorded state, and
the risk is that the unrecorded changes may *depend* on the patches we
are removing.

In this case, the changes look like this:

    --> [patch being suspended] --> [unrecorded changes] -->

What the code you pulled out into 'afterPending' does is try to commute
the two. If it succeeds, then the changes can be viewed as

       /-- [patch being suspended]
    --<
       \-- [unrecorded changes']

and it's fine to go ahead and suspend the patch, leaving the commuted
unrecorded changes in the repository.

This is similar to what we need to do when we obliterate/unpull patches,
so the code that does this in Rebase really ought to be shared with the
similar code in Darcs.UI.Commands.Unrecord.genericObliterateCmd

When unsuspending, we are *adding* patches to the recorded state, and so
the risk is that the unrecorded changes may *conflict*. We are starting with

       /-- [patch being unsuspended]
    --<
       \-- [unrecorded changes]

and what we need to do is to *merge* the two to get:

    --> [patch being unsuspended] --> [unrecorded changes'] -->

This is similar to what happens during a pull/apply, and the relevant
code is in Darcs.Repository.Merge.tentativelyMergePatches.

So I think the two options here are to reuse tentativelyMergePatches or
abstract out just the code for merging with unrecorded from that, or to
reimplement the necessary bits directly.

Cheers,

Ganesh

For the limitation in suspend and obliterate I have a partial 
solution (not yet published): first commute what we can, and if some 
unrecorded changes remain (depend on the stuff we want to 
obliterate/suspend), display them and ask the user if they want to 
revert them or cancel the operation.

The limitation for unsuspend was partly lifted some time ago. It now 
allows unrecorded changes to exist, but fails if they conflict with 
the effect of the patches to unsuspend (including possible conflict 
markup due to the unsuspend alone). The remaining limitation seems 
prudent, as long as the conflict markup introduced by unsuspend 
cannot be re-created once it is lost.