[darcs-devel] [issue2272] darcs rebase unsuspend should automate or semi-automate handling unrecorded changes

[Ganesh Sittampalam]

Ganesh Sittampalam <ganesh at earth.li> added the comment:

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 at 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

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/issue2272>
__________________________________