darcs

Patch 1719 simplify canonizeFL for V1 prim patches

Title simplify canonizeFL for V1 prim patches
Superseder Nosy List bf
Related Issues
Status accepted Assigned To
Milestone

Created on 2018-08-27.15:56:19 by bf, last changed 2018-09-14.08:07:02 by ganesh.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt bf, 2018-08-27.15:56:19 text/x-darcs-patch
simplify-canonizefl-for-v1-prim-patches.dpatch bf, 2018-08-27.15:56:19 application/x-darcs-patch
unnamed bf, 2018-08-27.15:56:19 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg20272 (view) Author: bf Date: 2018-08-27.15:56:19
See my belated comment (msg20270) to issue525.

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

patch 6fd909a5eb7f2f82bc7cf4d0d71a3d39d578a968
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Aug 27 17:09:22 CEST 2018
  * simplify canonizeFL for V1 prim patches
Attachments
msg20285 (view) Author: bf Date: 2018-08-28.18:53:25
I am hesitating to screen this. It is a simple change but if my
reasoning is faulty this may bite us later. So I would like someone else
to give it a sanity check first.
msg20290 (view) Author: ganesh Date: 2018-08-28.21:46:16
Just doing a bit of archaeology:

In this patch, I introduced canonizeFL in substantially its current form, from abstracting 
this code, and added that comment about issue525 which was presumably discovered from 
running the test suite.

patch de7aa3bf018e75e74efee23f98715ab98012c87e
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Sep 19 01:40:11 GMT Summer Time 2009
  * add canonization function for FL Prim

    hunk ./src/Darcs/Commands/AmendRecord.lhs 192
    -    in n2pia $ infodepspatch new_pinf pdeps $ fromPrims $ concatFL $ mapFL_FL canonize
    -           $ sort_coalesceFL $ concatFL $ mapFL_FL canonize $ oldchs +>+ chs
    +    in n2pia $ infodepspatch new_pinf pdeps $ fromPrims $ canonizeFL
    +             $ oldchs +>+ chs

The repeated calls were introduced by David here:

patch 7068e05a83ff602205087495c03ac61e70a98ad2
Author: David Roundy <droundy@darcs.net>
Date:   Sat Nov 15 21:19:25 GMT Standard Time 2008
  * resolve issue525: canonize output of sort_coalesceFL in AmendRecord.
    hunk ./src/Darcs/Commands/AmendRecord.lhs 193
    -    in n2pia $ infodepspatch new_pinf pdeps $ fromPrims $ sort_coalesceFL $
    -       concatFL $
    -       mapFL_FL canonize $ oldchs +>+ chs
    +    in n2pia $ infodepspatch new_pinf pdeps $ fromPrims $ concatFL $ mapFL_FL canonize
    +           $ sort_coalesceFL $ concatFL $ mapFL_FL canonize $ oldchs +>+ chs


So the original code before the issue525 fix was "canonize then coalesce". That fix then 
made it "canonize then coalesce then canonize". And now you are saying that actually "coalesce then canonize" is enough.

So your change seems plausible if the tests pass, and it also doesn't seem that dangerous: 
I think in the worst case we'll get some ugly diffs again, rather than producing corrupt 
repositories or something like that.
msg20291 (view) Author: bf Date: 2018-08-28.22:05:47
Okay, thanks for documenting the history here. And yes, that is what I
am saying. The tests pass, so I'm screening it now.
msg20304 (view) Author: ganesh Date: 2018-09-14.07:09:43
Fine
History
Date User Action Args
2018-08-27 15:56:19bfcreate
2018-08-28 18:53:25bfsetmessages: + msg20285
2018-08-28 21:46:16ganeshsetmessages: + msg20290
2018-08-28 22:05:47bfsetstatus: needs-screening -> needs-review
messages: + msg20291
2018-09-14 07:09:43ganeshsetstatus: needs-review -> accepted-pending-tests
messages: + msg20304
2018-09-14 08:07:02ganeshsetstatus: accepted-pending-tests -> accepted