Issue 2570 coalesceHunk is programmed in a very strange way

Title coalesceHunk is programmed in a very strange way
Priority bug Status unknown
Milestone Resolved in
Superseder Nosy List
Assigned To

Created on 2018-03-03.19:19:07 by bf, last changed 2018-03-03.20:53:39 by bf.

msg19925 (view) Author: bf Date: 2018-03-03.20:33:39
As far as I can see coalescing of hunks does not do what I (and others,
according to comments in the code) thought it does! It behaves in very
strange ways that look completely broken to me.

Here is the code, re-written to make it easier to see what's going on
but otherwise equivalent to the old code (all the examples I give below
have been tested with the original and the re-written code). (The 'c' in
oc1, nc1 etc stands for "content"):

coalesceHunk f l1 oc1 nc1 l2 oc2 nc2
  | l1 == l2 =
    case compare o1 n2 of
      LT | take o1 nc2 == oc1 ->
        returnFP $ Hunk l1 oc2 (nc1 ++ drop o1 nc2)
      GT | take n2 oc1 == nc2 ->
        returnFP $ Hunk l1 (oc2 ++ drop n2 oc1) nc1
      EQ | nc2 == oc1 ->
        returnFP $ Hunk l1 oc2 nc1
      _ -> Nothing
  | l1 < l2 && l1 + o1 >= l2 =
    let only1 = take (l2 - l1) oc1
    in coalesceHunk f l1 oc1 nc1 l1 (only1 ++ oc2) (only1 ++ nc2)
  | l1 > l2 && l2 + n2 >= l1 =
    let only2 = take (l1 - l2) nc2
    in coalesceHunk f l2 (only2 ++ oc1) (only2 ++ nc1) l2 oc2 nc2
  | otherwise = Nothing
    returnFP = Just . FP f
    o1 = length oc1
    o2 = length oc2
    n1 = length nc1
    n2 = length nc2

This will return Nothing in many cases where I thought it would
coalesce. Here is an example (with OverLoadedStrings in effect and a
slightly improved Show instance):

*Darcs.Patch.Prim.V1.Coalesce> coalesceHunk (fp2fn "bla") 1 ["a","b"]
["x"] 2 [] ["y"]

I expected to get  Just (FP (fp2fn "bla") 1 ["a","b"] ["x","y"])

It is easy to get complete nonsense out of perfectly valid patch
sequences, such as

*Darcs.Patch.Prim.V1.Coalesce> coalesceHunk (fp2fn "bla") 1 ["a","b"]
["x"] 1 ["x"] ["a"]
Just (FP (fp2fn "bla") (Hunk 1 ["x","b"] ["x"]))

or, even weirder:

*Darcs.Patch.Prim.V1.Coalesce> coalesceHunk (fp2fn "bla") 1 ["a","b"]
["x"] 1 ["x","y"] ["a"]
Just (FP (fp2fn "bla") (Hunk 1 ["x","y","b"] ["x"]))

This should result in lots of hunk application errors. But for some
reason the QC tests don't seem to generate such cases. And why it
doesn't crash in practice is beyond me. My first guess was that
coalesceHunk is only called after some preprocessing of patch lists
(re-ordering?). However, the nonsense examples above do not (currently)
commute (relaxed hunk commutation is disabled).

I am stupefied and would appreciate any kind of input on this.
msg19926 (view) Author: bf Date: 2018-03-03.20:42:01
I found the missing piece. This is how it gets called:

coalesceFilePrim f (Hunk line1 old1 new1 :> Hunk line2 old2 new2)
    = coalesceHunk f line2 old2 new2 line1 old1 new1

See the problem?

Writing code in such a way is just evil. Shame on you (whoever did
that). I will fix this, and perhaps I can finally start making sense of it.
msg19927 (view) Author: bf Date: 2018-03-03.20:53:37
Ahem. The idiot who produced this mess was me:

14: patch 761abc0e52f8175746153415f0486799161cb2b6
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Sun Jun 14 17:22:23 CEST 2015
  * Darcs.Patch.Prim.V1.Coalesce: replaced :< with :>

My only excuse is that establishing a readable standard order for the
patch combination operators (apply left first, then right) was a /huge/
operation and it seems here I took a short-cut. And as we all know,
short-cuts like that will bite back, sooner or later...
Date User Action Args
2018-03-03 19:19:07bfcreate
2018-03-03 20:33:41bfsetmessages: + msg19925
title: coalesceHunk is doing very strag -> coalesceHunk is doing very strange things
2018-03-03 20:42:02bfsetpriority: bug
messages: + msg19926
2018-03-03 20:53:39bfsetmessages: + msg19927
title: coalesceHunk is doing very strange things -> coalesceHunk is programmed in a very strange way