This bundle solves issue2548, issue2592, and issue1316, as well as an
issue with no number (tests/look_for_replaces1.sh).

7 patches for repository http://darcs.net/screened:

patch 940da5fdbc54e64875e3ef9a962e582cef9b0423
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jun 13 19:50:34 CEST 2018
  * resolve issue2548: inconsistent pending
  
  In Darcs.Repository.Diff.treeDiff we did not correctly handle the case
where
  a file is changed to a subTree: we only emptied the file but failed to add
  an rmfile patch before adding the dir.
  
  With a pending "addfile ./f" and a working tree where f has been
changed to
  a directory, we now get only the "adddir ./f" when recording with -l.
  
  However, this leaves us with a new pending "rmdir ./f; addfile ./f". This
  pending change can be reverted but does not show up with whatsnew -l and
  cannot be recorded with record -l.

patch 85c22f36716b5abd2a398a7c3a6cd81d7b6f3070
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 18 21:05:30 CEST 2018
  * accept issue2592: unclean pending with look-for options

patch ffa25a85a94ebb4ec8ac856fdb33135387cb34e4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 15:40:08 CEST 2018
  * fix handling of pending patch in amend command
  
  In case of look-for-moves/replaces, the code called
  tentativelyRemoveFromPending on the old patch. This was done after
  tentativelyRemovePatch and tentativelyAddPatch had already adapted the
  pending patch (via YesUpdatePending). This is clearly wrong and worked
only
  because tentativelyRemovePatches called prepend, which pre-filtered
the new
  prim patches with crudeSift. This is no longer done, and so we now get
  failures when we try to apply pending.
  
  The clean solution is to pass NoUpdatePending to both
tentativelyRemovePatch
  and tentativelyAddPatch and then adapt pending in one go with the
difference
  between old and new patch. The function tentativelyRemoveFromPending now
  takes an FL of prim patches, has more correct type witnesses, uses less
  coercion, and has its UpdatePending parameter removed.
  
  This restores the regression of issue2209-look_for_replaces after the fix
  for issue2548-inconsistent-pending. It also makes the previously failing
  test look_for_replaces1 succeed.

patch 6bbdbe7d6c50ae89a86aa2461c82b42df5658fe5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 16:51:01 CEST 2018
  * make D.R.Pending.prepend more type safe (by removing it)
  
  This function coerced witnesses and had corresponding warnings
attached. The
  need for coercing was that it reads the pending patch and also writes it
  back with some changes that were removed from the repo prepended. If we
  split this action up and read pending before adding the patches to the
repo
  and afterwards write the pending, then the witnesses all match up just
fine.
  This requires removeFromTentativeInventory to return a repo with
  appropriately coerced witnesses.
  The cost of this operation is that we must export
read/writeTentativePending.

patch a3e7b0254ad94acc5c30b056f27a5a032f0eba17
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 16:51:12 CEST 2018
  * simplified setTentativePending and fix its type witnesses

patch 90c1861cb3d104833964c1328075febc49dd9e98
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 18:38:03 CEST 2018
  * resolve issue2592: update pending with coalesced look-for changes

patch b072c4eba14e88f51a5d1b8b6ac0076c9cd40a0d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 13 15:44:49 CEST 2018
  * tests for issue1316 no longer fail
  
  This is most probably due to the recent fix for how the amend command
  handles the pending patch.

Oops, forgot to attach the bundle.

Attachments

• resolve-issue2548_-inconsistent-pending.dpatch

>  * resolve issue2548: inconsistent pending

> hunk ./src/Darcs/Repository/Diff.hs 134
> -        do rmFileP <- diff p (Changed a' (File emptyBlob))
> +        do emptyFileP <- diff p (Changed a' (File emptyBlob))
> +           rmFileP <- diff p (Removed a')
> hunk ./src/Darcs/Repository/Diff.hs 137
> -           return $ joinGap (+>+) rmFileP addDirP
> +           return $ joinGap (+>+) (joinGap (+>+) emptyFileP rmFileP) addDirP

OK - should the analogous case just below for going the other way
also be changed (to get an addfile patch)?

>  * accept issue2592: unclean pending with look-for options

OK

>  * fix handling of pending patch in amend command

OK

>  * make D.R.Pending.prepend more type safe (by removing it)

OK

>  * simplified setTentativePending and fix its type witnesses

OK (you actually simplified tentativelyAddToPending as well)

>  * resolve issue2592: update pending with coalesced look-for changes

> --- | @tentativelyRemoveFromPending p@ is used by Darcs whenever it
> ---   adds a patch to the repository (eg. with apply or record).
> ---   Think of it as one part of transferring patches from pending to
> ---   somewhere else.

I actually find this comment you've removed quite helpful for building
intuition, so it would be good if it could be restored in some form,
and if tentativelyRemoveFromPW could also have a doc comment.

If I understand correctly, tentativelyRemoveFromPending is now only used
via tentativelyAddPatch with 'YesUpdatePending', which in turn may
only be used from Rebase and Tag - the former is probably for no good
reason, and I doubt it really matters with Tag.

> +-- We aim for the following property: the result should be the same as if patch
> +-- selection had been done with the un-coalesced @pending+>+working@ (with the
> +-- original patches selected in place of the coalesced ones), and if, of that
> +-- sequence, the elements that were originally in @pending@ are now removed
> +-- from it.

Thanks for writing a specification for this :-) What exactly do you mean by
"patch selection" here? Just that it's like looking for the relevant patches
in the patch selection UI? I also can't understand "if, of that sequence".

It's also not immediately obvious to me why working now is a parameter to this
stack of operations - if you could explain that a bit in the comment it'd be
great.


> -                if nullFL chs && null deps
> -                  then putStrLn "Ok, if you don't want to record anything, that's fine!"
> -                  else do setEnvDarcsFiles chs
> -                          (name, my_log, logf) <- getLog (patchname cfg) (pipe cfg) (logfile cfg) 
(askLongComment cfg) Nothing chs
> -                          debugMessage ("Patch name as received from getLog: " ++ show (map ord name))
> -                          doActualRecord repository cfg name date my_author my_log logf deps chs
> +                when (nullFL chs && null deps) $
> +                  putStrLn "Ok, if you don't want to record anything, that's fine!"
> +                setEnvDarcsFiles chs
> +                (name, my_log, logf) <- getLog (patchname cfg) (pipe cfg) (logfile cfg) (askLongComment cfg) 
Nothing chs
> +                debugMessage ("Patch name as received from getLog: " ++ show (map ord name))
> +                doActualRecord repository cfg name date my_author my_log logf deps chs pw

This looks like an unintended change in behaviour: as the 'when' clause won't cause
an early exit from the rest of the do block, we will now try to record an empty
patch.

>  * tests for issue1316 no longer fail

Fine

Following up on a your review.

2 patches for repository http://darcs.net/screened:

patch 3d2b7705d1c59fcb986f9815b845e057c0678495
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 25 13:16:44 CEST 2018
  * re-add docs to explain what tentativelyRemoveFromPending is used for

patch 1176cba1e69591f3686e5b81bd0250ac9345d614
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 25 13:18:21 CEST 2018
  * add docs for tentativelyRemoveFromPW

Attachments

• re_add-docs-to-explain-what-tentativelyremovefrompending-is-used-for.dpatch

> If I understand correctly, tentativelyRemoveFromPending is now only used
> via tentativelyAddPatch with 'YesUpdatePending', which in turn may
> only be used from Rebase and Tag - the former is probably for no good
> reason, and I doubt it really matters with Tag.

tentativelyAddPatch_ is indirectly involved in many more commands e.g.
via tentativelyMergePatches. It is usually called with YesUpdatePending,
except by amend, record, and convert which modify/create patches with
changes from pending and working. So these commands update pending
explicitly and they use tentativelyRemoveFromPW for that.

>> hunk ./src/Darcs/Repository/Diff.hs 134
>> -        do rmFileP <- diff p (Changed a' (File emptyBlob))
>> +        do emptyFileP <- diff p (Changed a' (File emptyBlob))
>> +           rmFileP <- diff p (Removed a')
>> hunk ./src/Darcs/Repository/Diff.hs 137
>> -           return $ joinGap (+>+) rmFileP addDirP
>> +           return $ joinGap (+>+) (joinGap (+>+) emptyFileP rmFileP)
addDirP
> 
> OK - should the analogous case just below for going the other way
> also be changed (to get an addfile patch)?

Hm. This case is not handled fully symmetrically. The file->dir case
previously emptied the file without removing it and my patch adds the
missing remove.

OTOH the dir->file case does not make the directory empty but /does/
remove it. I just added the reverse case to the test for issue2548 (add
directory f, remove it, then touch file f) and found that now darcs
record says there are no changes, even though the pending patch still
contains the "adddir ./f". I need to investigate this further to find
out why it behaves differently here.

>> +-- We aim for the following property: the result should be the same
as if patch
>> +-- selection had been done with the un-coalesced @pending+>+working@
(with the
>> +-- original patches selected in place of the coalesced ones), and
if, of that
>> +-- sequence, the elements that were originally in @pending@ are now
removed
>> +-- from it.
> 
> Thanks for writing a specification for this :-) What exactly do you
mean by
> "patch selection" here? Just that it's like looking for the relevant
patches
> in the patch selection UI? I also can't understand "if, of that sequence".

I am talking about the selection of primitive changes from pending and
working when we amend or record a patch.

How about this re-worded paragraph:

-- We aim for the following property: the result should be the same as if
-- selection of the primitive changes had been done with the un-coalesced
-- @pending+>+working@ (with the original changes selected in place of the
-- coalesced ones). Of this (hypothetical) selection of primitive
changes, we
-- want to identify those that originated from pending and then remove them
-- from pending.

> It's also not immediately obvious to me why working now is a parameter
to this
> stack of operations - if you could explain that a bit in the comment
it'd be
> great.

I thought the part below that explains the algorithm makes it pretty
clear why we need the (old) working:

-- The algorithm is as follows. For each @x@ in @changes@ we first try to
-- remove it from @pending@ as is. If this fails, we try to commute it past
-- @pending@, pushing any (reverse) dependencies with it, and check if
we can
-- remove the result from @working@. If that fails too, we try to
"decoalesce"
-- the @x@ with every element of @pending@ (using 'headPermutationsFL'),
again
-- checking that the result can be commuted past @pending and removed from
-- @working@.

>> -                if nullFL chs && null deps
>> ...
> 
> This looks like an unintended change in behaviour: as the 'when'
clause won't cause
> an early exit from the rest of the do block, we will now try to record
an empty
> patch.

Right, sorry about that. Rollback is attached (modulo addition of new pw
parameter to doActualRecord).

Attachments

• rolled-back-an-accidental-semantic-change-in-record.dpatch

I have found that the fix in Darcs.Repository.Diff.treeDiff can be
simplified. This still leaves the question why do we not make
directories empty before we remove them as we do for files.

1 patch for repository http://darcs.net/screened:

patch 0c411632a2068c2004d41162bd790863e39011b2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Aug 26 11:18:15 CEST 2018
  * remove extraneous emptying of file object in treeDiff
  
  The fix for issue2548 correctly added an rmFileP patch in the case
where we
  detect a file that is changed into a directory. However, the rmFileP
already
  contains a patch that makes the file empty, so we don't have to do it
again.

Ganesh, sorry for being dense, I didn't see the obvious: the patch named
addFileP doesn't actually create an addfile patch but merely sets the
content, exactly as the other case did (before the fix). Well spotted!
Attached is the fix, together with the extended test script.

1 patch for repository http://darcs.net/screened:

patch 1595eb2fc367e9e530a834f904f7a499b9d32111
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Aug 26 12:34:13 CEST 2018
  * resolve the symmetric case for issue2548, too, and extended test script

Attachments

• resolve-the-symmetric-case-for-issue2548_-too_-and-extended-test-script.dpatch

Here is an amended patch that superseeds the previous one. See long
comment for details.

patch a6ca1a898e7362c83d7f6dd6d2a40a310f60eb54
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Aug 26 12:34:13 CEST 2018
  * resolve the symmetric case for issue2548, tightened and extended
test script
  
  The test script previously specified only that the unrecorded changes
that a
  'record -l' sees are consistent i.e. addfile or adddir but not both.
We now
  require that the state of the working tree wins over whatever is in
pending.
  This is justified because when the user issues 'darcs add' she does not
  specify whether to add a file or a directory, so whether pending has an
  addfile or an adddir is incidental.

Attachments

• resolve-the-symmetric-case-for-issue2548_-tightened-and-extended-test-script.dpatch

Extended the test script by adding a file to the directory before the 'darcs
add'. This is to check that even if pending has 'adddir ./f; addfile ./f/g'
(but the working state has just a file f), darcs record -l (and also darcs
whatsnew -l) will "fix" pending to be consistent with the working tree.

I was surprised at first that this works, since treeDiff does not seem to
create a patch that recursively removes a directory's content when it
removes a directory (in contrast to files, where it makes the file empty).
However, a closer look reveals that this work has already been done on
previous recursive calls, since tree nodes are visited in depth first order.
So I added a small comment to explain the apparent asymmetry.

4 patches for repository http://darcs.net/screened:

patch 15f00771cd40c88a0f14f94d1bed02fa1e4d7d61
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Aug 26 19:25:51 CEST 2018
  * add comment to explain how treeDiff handles file vs. subtree removals

patch 9225a96386fcbbb4b8079b5f47ddf810459b9e0b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 28 11:12:42 CEST 2018
  * treeDiff: abbreviate 'anchorPath ""' with a local function

patch d88f122acf89227e9c12f1ea605310c81469b58c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 28 11:14:53 CEST 2018
  * tests: add a missing shebang for bash interpreter

patch 8a21799800f581d5615f19a44a9cef4c6cfd20a3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 28 11:31:52 CEST 2018
  * extend and polish test script for issue2548

Attachments

• add-comment-to-explain-how-treediff-handles-file-vs_-subtree-removals.dpatch
• patch-preview.txt
• unnamed

I think this untracked patch in screened belongs in this ticket

1 patch for repository /home/ganesh/darcs/screened-temp:

patch 0c411632a2068c2004d41162bd790863e39011b2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Aug 26 10:18:15 BST 2018
  * remove extraneous emptying of file object in treeDiff
  
  The fix for issue2548 correctly added an rmFileP patch in the case where we
  detect a file that is changed into a directory. However, the rmFileP already
  contains a patch that makes the file empty, so we don't have to do it again.

Attachments

• patch-preview.txt
• remove-extraneous-emptying-of-file-object-in-treediff.dpatch
• unnamed

Again, it seems I forgot to attach the bundle (see msg20265). Thanks for
catching this. Hopefully, with the email based tracker interface now
working again, these kind of mistakes are easier to avoid!

I am screening the patches from the follow-up bundle (see msg20277).

Here is a patch that tries to improve the documentation for 'updatePending'
according to what I proposed in msg20254. I am not screening this one yet.
Please criticize, I am willing to spend more effort to make the
specification more understandable but I need feedback to do so.

1 patch for repository http://darcs.net/screened:

patch 5eea86e475c7e8cf5857f35c30eb4547cf66d3ac
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Aug 26 19:26:20 CEST 2018
  * improved wording of docs for updatePending

Attachments

• improved-wording-of-docs-for-updatepending.dpatch
• patch-preview.txt
• unnamed

This bundle superseeds the one which tried to improve on the docs of
updatePending. The new description should be easier to understand.

2 patches for repository http://darcs.net/screened:

patch 078ef4dfc7f0fa7b03d9ddead5153a359e75e691
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Sep 22 11:02:28 CEST 2018
  * simplify decoalescing and add all missing cases
  
  Decoalescing can /almost/ be defined as decoalesce x y = coalesce y^ x.
  There are only a few cases that need to be handled specially, see comments
  in the code for details. The simplified implementation can now handle hunks
  and token replaces, too.

patch 676bf23cd07f7dc961587a1148b64dd63a5d0542
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Aug 26 19:26:20 CEST 2018
  * fix updatePending and rewrite docs
  
  The previous specification and implementation was invalid, because it did
  not take hunk splitting into account. This resulted in crashes when we use
  interactive hunk editing with a non-empty but unrelated pending.

Attachments

• patch-preview.txt
• simplify-decoalescing-and-add-all-missing-cases.dpatch
• unnamed

I am screening the latest follow-up bundle because it fixes a bug in 
patches that have already been screened.

Working through this review again, the new docs look good - thanks,
and sorry for the long gaps between your work and my reviews.

Marking these patches as reviewed:

* resolve issue2592: update pending with coalesced look-for changes
* re-add docs to explain what tentativelyRemoveFromPending is used
  for
* add docs for tentativelyRemoveFromPW
* rolled back an accidental semantic change in record

and also the new docs for updatePending in
* fix updatePending and rewrite docs
(I still need to review the updated decoalesce logic etc).

This one is fine too:

  * remove extraneous emptying of file object in treeDiff

>  * treeDiff: abbreviate 'anchorPath ""' with a local function

OK

Re-reviewing this whole collection together as that's simpler to do, all 
look fine:

>  * resolve issue2548: inconsistent pending
>  * remove extraneous emptying of file object in treeDiff
>  * resolve the symmetric case for issue2548, tightened
      and extended test script
>  * extend and polish test script for issue2548
>  * add comment to explain how treeDiff handles file vs.
     subtree removals

Looking at the latest decoalescing code, these cases look dubious
to me:

> decoalescePair (FP b AddFile) (FP a AddFile) = Just (Move a b)
> decoalescePair (DP b AddDir) (DP a AddDir) = Just (Move a b)

Aren't they essentially inventing a Move out of nowhere?
I can see an argument that the only way to get here would be
if AddFile a wasn't already in pending+working, in which case
it must have been the result of a coalesce, but even then how
do we know that AddFile b is the right one to decoalesce with?
If there are two AddFiles in pending+working, either could be
"correct".

Also, put together with these cases:

> -- These two cases must fail because the token replace patches 
that coalesce
> -- has eliminated are irretrievably lost.
> decoalesceFilePrim _ (AddFile) (RmFile) = Nothing
> decoalesceFilePrim _ (RmFile) (TokReplace{}) = Nothing

I am unsure that this specification of updatePending is correct,
referring to decoalescing the recorded change from pending+working:

> This final step must not fail. If it does, then we have a bug 
> because it means we recorded a change that cannot be removed from 
> the net effect of @pending@ and @working@.

If information is lost during coalescing, leading to cases where
decoalesce can't undo a coalesce, then isn't it also possible for
updatePending to be unable to remove a change that did originate 
from pending+working?

Hi Ganesh

you raise a number of issues that all seem valid to me. Decoalescing
really is a black art since we have almost no useful algebraic
properties here. This makes it quite difficult to reason about it in
general terms.

I have difficulty in giving this the concentration it requires, right
now,  because I am deep into a major refactoring. Could you try and
create concrete test cases that demonstrate the failures you expect?
E.g. one containing two pending addfiles where we may chose the wrong
one when we decoalesce, and ideally also one where algorithm fails and
crashes darcs? If you are right then this shouldn't be too hard. It
would make it easier for me to think the example through and perhaps
come up with a solution.

Or perhaps you have a better idea how to approach the whole mess?

One problem is that we should not change the behavior of the existing
coalese because at least the V1 commute/merge code uses it. If that
weren't the case I would vote for scrapping the cases where coalesce
irrecoverably looses a change, i.e. the
replace-before-remove/replace-after-add cases. I'd rather want a safe
and reliable system even if that costs a bit of convenience. The "which
addfile" ambiguity could perhaps be solved by trying all possible
decoalescings and gracefully failing if there are multiple ones and
there is no other way to disambiguate.

Here's a test that demonstrates a problem with two
adds not being properly distinguishable. It works
fine without the last two patches:

  * simplify decoalescing and add all missing cases
  * simplify decoalescing and add all missing cases

But with them we end up with a funny pending in
the second case:

{
move ./c ./a
addfile ./b
}


I'm not entirely sure how we get that exact broken state,
but I think it's roughly as I surmised with decoalescing
with the wrong add.

I'm also not sure if directly grepping pending for
'move' is quite the right thing for the test, an
alternative would be to check for exactly what we
expect, which should be an add of the file that wasn't
moved/recorded.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 0eb0f482ca79539ba0bc484e691575e19daa44f5
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Oct 16 18:39:31 BST 2018
  * add test that decoalescing a move works properly

Attachments

• add-test-that-decoalescing-a-move-works-properly.dpatch
• patch-preview.txt
• unnamed

On 16/10/2018 21:14, Ganesh Sittampalam wrote:

>   * simplify decoalescing and add all missing cases
>   * simplify decoalescing and add all missing cases

Sorry, that second patch should be:

  * fix updatePending and rewrite docs

Thanks Ganesh. The two decoalesce cases are complete rubbish.

As the comment indicated (but perhaps not clearly enough) the intention was
to add the cases that correspond to the two commented-out cases that that I
added for coalesce and which are obviously nonsense when coalescing.

That is, we do /not/ want to coalesce

  remove a :> add b ---> move a b

but we /do/ want to decoalesce

  remove a <--- move a b :\/: add b

I just got confused and ended up adding the /same/ nonsense cases to
decoalesce, instead of the /corresponding/ ones. Indeed, if you take the
nonsense I accidentally added to decoalesce:

  move a b <--- add b :\/: add a

and turn it around, you get this valid case for coalesce:

  move a b :> add a ---> add b

So this is nicely symmetric.

Attached is a patch that fixes the two cases and slightly extends the
comment to make the correspondence clearer. Your test (and all others)
succeed now.

1 patch for repository http://darcs.net/screened:

patch 3391054ba84fe9adcaa5e6057d0aac6a08eea59a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Oct 17 09:51:32 CEST 2018
  * fix nonsense cases for decoalesce

Attachments

• fix-nonsense-cases-for-decoalesce.dpatch
• patch-preview.txt
• unnamed

Amended your test patch to address the "grep pending" issue you mentioned.

1 patch for repository http://darcs.net/screened:

patch a56ffcf03fae25d66014544e1aeb6cd8e9077e02
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Oct 16 19:39:31 CEST 2018
  * add test that decoalescing a move works properly

Attachments

• add-test-that-decoalescing-a-move-works-properly.dpatch
• patch-preview.txt
• unnamed

Coming back to this question I asked:

> If information is lost during coalescing, leading to cases where
> decoalesce can't undo a coalesce, then isn't it also possible for
> updatePending to be unable to remove a change that did originate 
> from pending+working?

I've spent a while looking at the code/trying to break darcs,
without success. I think that may be partly because not all possible
can be generated in working as the --look-for options only find certain
kinds of things.

Anyway, I don't think it's worth any more effort; even if it's not
perfect, the new code is an improvement.

Hi Ganesh, thanks for trying to break darcs ;-)) I am still glad you 
weren't successful, though.

I have since been working a lot on understanding (and refactoring) the 
V1 commutation code and I no longer think that changing the semantics of 
sortCoalesceFL will break it. I think I got this impression initially 
because the code in its current form doesn't clearly separate what is 
needed for commutation/merge with what is needed for calculation of the 
conflict resolution markup. But the two can be separated and then it 
becomes clear that only conflict markup is affected, not commutation or 
merging itself.

This means we can remove the few cases where coalescing looses 
information without breaking compatibility in any essential way.

> This means we can remove the few cases where coalescing looses 
> information without breaking compatibility in any essential way.

I experimented with this and found that this change (removing the two
offending cases for coalesce) breaks tests/revert.sh. Investigating
further, I found that this is actually a bugfix!

I can cut the failure down to this simple example:

echo hello world > foo
darcs add foo
darcs replace hello goodbye foo
darcs whatsnew

I would expect to see, and with the patched darcs (where I removed the
two problematic cases for coalesce) this is what I get:

addfile ./foo
hunk ./foo 1
+hello world
replace ./foo [A-Za-z_0-9] hello goodbye

but what we currently get is

addfile ./foo
hunk ./foo 1
+goodbye world

which is also what tests/revert.sh is expecting.

Perhaps I am confused but I think that loosing the replace change here
is a bug. I am not sure how exactly the two coalesce cases end up
coalescing the hunk with the replace. Here they are, again, for reference:

coalesceFilePrim f (AddFile) (TokReplace{}) = Just $ FP f AddFile
coalesceFilePrim f (TokReplace{}) (RmFile) = Just $ FP f RmFile