[darcs-devel] How to merge rebase refactoring

Ganesh Sittampalam ganesh at earth.li
Fri Feb 19 07:26:20 UTC 2016


Hi,

The new "stash" feature is still some way from merging as the UI needs
sorting out. In the meantime I'm hoping to get the major refactoring to
rebase that it depends on merged, to reduce future conflicts.

I've put up a draft here:

http://hub.darcs.net/ganesh/darcs-rebase-refactor-20160218/compare/darcs/darcs-screened

There's a chunk of patches in the middle that are split into logical
parts, but can't be applied without each other - the intermediate states
have missing cases etc and would fail at runtime:

- add RebaseP
- introduce RebaseType
- replace MaybeInternal with IsRepoType
- fix fmapFL_WrappedNamed
- get rid of Rebasing
- read rebase patches into RebaseP
- sort out Commute/Merge instances

When I was originally developing the refactoring I introduced a
temporary type class ("MatchRebasing") that I used to keep the code
compiling at each stage, but that isn't included in the patches above.
Having it involved adding the type class temporarily to various
constraints all over the codebase and then later removing it again.

So, I see three options for submitting the patches above:

 - Submit roughly as they are now. Easy for me to do, but this doesn't
feel like a good option given the intermediate broken states in the repo
that would obstruct tracking down problems in future.

 - Merge them all into one patch and submit that. Harder to review and
to understand and harder in future to understand the history, but also
easy for me to do.

 - Update them to use the "MatchRebasing" temporary typeclass again.
More work for me, and introduces a weird temporary thing that will
clutter history a bit, but keeps each patch individually reviewable and
understandable in the history.

Please let me know what you think would be best.

Further background on the changes:

The essence of the changes is a shift in the internal representation of
repositories with a rebase in progress - the type changes from:

    Named (Rebasing p)

to

    WrappedNamed ('RepoType 'IsRebase) p

WrappedNamed is a new type that goes around Named:

    data WrappedNamed (rt :: RepoType) p wX wY where
        NormalP :: !(Named p wX wY) -> WrappedNamed rt p wX wY
        RebaseP
          :: (PrimPatchBase p, Show2 (PrimOf p), FromPrim p, Effect p)
          => !PatchInfo
          -> !(Rebase.Suspended p wX wX)
          -> WrappedNamed ('RepoType 'IsRebase) p wX wX

So where before the "rebase container" patch would have been

    NamedP pi (Suspended s :>: NilFL)

now it's

    RebaseP pi s

and where normal patches would have been

   NamedP pi (Normal p)

they now become

   NormalP (NamedP pi p)

The rebase container patch is still a hack, but now in the code it lives
closer to where it should. Ideally it would just live at the Repository
level rather than the Patch level, but that's for another day.

The sequence of patches above starts from the introduction of the
RebaseP constructor in WrappedNamed, and finishes when that constructor
is properly handled everywhere in the code.

Cheers,

Ganesh


More information about the darcs-devel mailing list