[darcs-devel] [patch1083] name change Lcs.hs -> Diff/Myers.hs (and 2 more)

[Guillaume Hoffmann]

Guillaume Hoffmann <guillaumh at gmail.com> added the comment:

* name change Lcs.hs -> Diff/Myers.hs

Good to go. There's a useless hunk in ./src/Darcs/Util/Diff/Myers.hs ,
try to not add this kind of changes in the future.

* resolve issue346: implement "patience diff" from bzr

I have above all superficial, cleanup comments that you could take into
account
for the next iteration of this patch.

Maybe the long description could do without the " in the commands that
it could be useful".

> hunk ./harness/Darcs/Test/Patch/Properties/V1Set1.hs 100
> -    where p1_ = mapFL_FL fromPrim $ concatFL $ mapFL_FL canonize $
sortCoalesceFL $ effect p1
> +    where p1_ = mapFL_FL fromPrim $ concatFL $ mapFL_FL (canonize
D.MyersDiff) $ sortCoalesceFL $ effect p1

You could add a comment telling that the diff algorithm does not matter
if it's the case, or justifying why Myers is used.


> hunk ./src/Darcs/Patch/Annotate.hs 143
> -        reannotate $ getChanges before after
> +        dac <- gets diffAlgorithm
> +        reannotate $ getChangesWith dac before after

Why not use the name `da`? What is the `c` for in `dac`? Same obseration
for the following hunks.


> hunk ./src/Darcs/Patch/Named.hs 165
> -                let (informAdd, informDel) = fromMaybe (const id,
const id) nameHack
> +                -- TODO: see how to integrate DiffAlgorithm or if it
is necessary at all
> +                let (informAdd, informDel) = fromMaybe (const id,
const id) (nameHack MyersDiff)

This comment does not really make sense to me. Is is a way to tell that
you are not sure about the use of `nameHack MyersDiff` and that it may
change in the future?

> hunk ./src/Darcs/Patch/Prim/V1/Coalesce.hs 243
> -
> +   [_$_]

Please remove this hunk (darcs amend --unrecord).


> hunk ./src/Darcs/Patch/Rebase.hs 211
> -  commuteNoConflicts = commuterRebasing commuteNoConflicts
> +  -- TODO: see how to integrate D.DiffAlgorithm or if it is necessary
at all
> +  commuteNoConflicts = commuterRebasing D.MyersDiff commuteNoConflicts
> hunk ./src/Darcs/Patch/Rebase.hs 218
> -  merge (Normal p :\/: Suspended qs) = unseal Suspended (addFixup
(invert p) qs) :/\: Normal p
> -  merge (Suspended ps :\/: Normal q) = Normal q :/\: unseal Suspended
(addFixup (invert q) ps)
> +  -- TODO: see how to integrate D.DiffAlgorithm or if it is necessary
at all
> +  merge (Normal p :\/: Suspended qs) = unseal Suspended (addFixup
D.MyersDiff (invert p) qs) :/\: Normal p
> +  merge (Suspended ps :\/: Normal q) = Normal q :/\: unseal Suspended
(addFixup D.MyersDiff (invert q) ps)

Same observation about the TODO comment.

> hunk ./src/Darcs/Patch/Rebase.hs 369
> -   recontextRebase = Just (RecontextRebase1 recontext)
> +   recontextRebase = Just $ RecontextRebase1 $ recontext

You may remove this hunk if it does not bother you.

> hunk ./src/Darcs/Patch/Rebase.hs 374
> -              = (IsEq, RecontextRebase2 (\fixups -> unseal
mkSuspended (simplifyPushes (mapFL_FL translateFixup fixups) ps)))
> +              -- TODO: see how to integrate D.DiffAlgorithm or if it
is necessary at all
> +              = (IsEq, RecontextRebase2 (\fixups -> unseal
mkSuspended(simplifyPushes D.MyersDiff (mapFL_FL translateFixup fixups)
ps)))

TODO comment.

> hunk ./src/Darcs/Patch/Split.hs 107
> --- |Never splits. In other code we normally pass around Maybe
Splitter instead of using this
> +-- |Never splits. In other `de we normally pass around Maybe Splitter
instead of using this

Please remove.

> hunk ./src/Darcs/UI/Commands/AmendRecord.hs 81
> -                   , diffingOpts, compression, verbosity,
removeFromAmended, useCache, umask )
> -import Darcs.Repository.Flags ( UpdateWorking(..), DryRun(NoDryRun) )
> +                   , diffingOpts, compression, verbosity,
removeFromAmended, useCache, umask, diffAlgorithmOptsChoice )
> +import Darcs.Repository.Flags ( UpdateWorking(..), DryRun(NoDryRun))

Please remove the second line of this hunk.

> hunk ./src/Darcs/UI/Commands/Move.hs 37
> -import Darcs.UI.Flags ( doAllowCaseOnly, doAllowWindowsReserved,
useCache, dryRun, umask)
> +import Darcs.UI.Flags ( doAllowCaseOnly, doAllowWindowsReserved,
useCache, dryRun, umask )


Please remove.

> hunk ./src/Darcs/UI/Commands/Remove.hs 43
> +
> hunk ./src/Darcs/UI/Commands/Remove.hs 49
> +

Please remove.

> hunk ./src/Darcs/UI/Commands/SetPref.hs 28
> -import Darcs.UI.Flags ( useCache, dryRun, umask)
> +import Darcs.UI.Flags ( useCache, dryRun, umask )

Please remove.

hunk ./src/Darcs/UI/Flags.hs 212
-

> hunk ./src/Darcs/UI/SelectChanges.hs 228
> -                WhichChanges -> FL p wX wY
> -                             -> PatchSelection p wX wY
> +                 WhichChanges -> FL p wX wY
> +                              -> PatchSelection p wX wY
> hunk ./src/Darcs/UI/SelectChanges.hs 241
> -                WhichChanges -> FL p wX wY
> -                             -> PatchSelection p wX wY
> +       WhichChanges -> FL p wX wY
> +                    -> PatchSelection p wX wY

Please remove.

hunk ./src/Darcs/Util/Diff.hs 6
+import Darcs.Util.Diff.Myers ( getChanges )
+import qualified Darcs.Util.Diff.Patience as P ( getChanges )
+import qualified Data.ByteString as B ( ByteString )
+
+
+data DiffAlgorithm = PatienceDiff | MyersDiff
+    deriving ( Eq, Show )
+
+getChangesWith :: DiffAlgorithm -> [B.ByteString] -> [B.ByteString]
+               -> [(Int,[B.ByteString],[B.ByteString])]
+getChangesWith dac = case dac of
+                       PatienceDiff -> P.getChanges
+                       MyersDiff -> getChanges

I think you could simply call this function getChanges.



Now, I'd rather have the following change not part of this patch:

> hunk ./src/Darcs/Patch/Prim/V1/Coalesce.hs 241
> -   canonizeFL = concatFL . mapFL_FL canonize . sortCoalesceFL .
> -                concatFL . mapFL_FL canonize
> +   canonizeFL diffa = concatFL . mapFL_FL (canonize diffa) .
sortCoalesceFL

You seem to be fixing the double use of canonize. I'd rather see it
happen in a separate
patch, that also modificates the comment right above this piece of code.
And with explanations
to justify the change.



Also, you enable functions of Darcs.Repository.Repair to receive a flag
enabling to choose the diff algorithm, however the command repair does
not receive this flag. Should it? Or should functions of
Darcs.Repository.Repair use a default diff algorithm?

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