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

I was recording some changes, before noticing at last regrets that I
don't want to include a particular sub-hunk that I've already said yes to.

I have already recorded a file containing:

    -- a comment

    data D = C Int
           deriving (Eq, Show)

which I then change in working to:

    -- another comment

    -- a comment

    data D = C !Int
           deriving (Eq)

Then, in record, I don't record the initial comment-addition, I do
record the final hunk.
I get to last regrets before realising I don't want to remove D's Show
instance,
so I put it back using the hunk editor. I accept the remaining changes,
but darcs fails:

    WARNING: Doing a one-time conversion of pristine format.
    This may take a while. The new format is backwards-compatible.
    Pristine conversion done...

    darcs failed:  ### Error applying:
    hunk ./file 5
    -data D = C Int
    -       deriving (Eq, Show)
    +data D = C !Int
    +       deriving (Eq)
    ### to file ./file:
    -- a comment

    data D = C Int
           deriving (Eq, Show)

darcs shouldn't fail, and should record a patch that simply adds a ! to
the Int parameter of C.

And the obvious thing to point out is that the pristine conversion
message is spurious, and any fix of this bug should look into why it's
occurring!

To my great surprise I can still reproduce this bug.

Indeed we have a failing test case for it.

The reason the apply fails here is the wrong line number "5" in the 
hunk. It should be "3" instead. It looks as if the context of the hunk 
we want to apply (the one edited in the hunk editor) has de-selected 
comment hunk as context. So it seems that if we go back after the last 
regrets prompt we fail to commute the de-selected hunk past.

Indeed, when I repeat the record manually and display ('l') the 
changes to be recorded at the second last regrets question I get 
this:

Do you want to Record these changes? [Yglqk...], or ? for more 
options: l
---- selected changes ----
hunk ./file 5
-data D = C Int
+data D = C !Int
hunk ./file 6
-       deriving (Eq, Show)
+       deriving (Eq)
---- end of selected changes ----

(The two hunks shown above will be coalesced before recording them.)

The basic underlying problem is that our bookkeeping for interactive 
selection is duplicated: one the one hand we have an FZipper of labelled 
patches (lps), used to determine the order in which we offer patches. On 
the other hand we have a PatchChoice to keep track of the selection status 
of each patch (choices). What happens in 'splitCurrent' is that we take 
the current patch from 'lps', let the user split it, and then splice the 
resulting sequence into 'choices'. This uses 
Darcs.Patch.Choices.substitute which in turn uses the unsafe 
'compareLabels' to find the corresponding patch in 'choices'. But the 
patches inside 'choices' can be reordered, relative to the original 
sequence 'lps'. So 'substitue' is unsound, in general.

The reason this error occurs only when we go back from the last regrets is 
that commutation inside 'choices' is postponed as long as possible. I 
haven't found out precisely why, but it is clear that 'lastQuestion' 
somehow forces this commutation. I am pretty sure we could construct other 
scenarios that exhibit this broken behavior. For instance, listing all 
selected changes also forces some of the commutations.

Fixing this is not as simple as I first thought. What we have to do is to 
make the splitter operate on the version of the patch as found in 
'choices'. This eliminates the unsoundness. But we also need to splice a  
new sequence into 'lps'. This is difficult because we now have to commute 
the new sequence so that it has the same context as the original patch in 
'lps' before splicing it into 'lps'; this is not trivial.

It may not even be possible to commute the split patches into the 
same place as in the other data structure, because splitting does 
not preserve commutation behavior. We may thus be forced to undo 
some of the decisions we already made. This would be a very 
complicated procedure with behavior that is hard to specify and 
implement.

I think the only viable solution is to do away with the double 
bookkeeping, use a plain Zipper of (just) the Labels for tracking 
where we are (relative to the original sequence), and always 
retrieving the actual patch under consideration from the 
PatchChoices for display and editing.