[darcs-devel] Code quality

[Ganesh Sittampalam]

Hi Ben,

I entirely agree with the substance of your rant, so I'll just inject a
few comments on odd details:

On 17/02/2015 05:32, Ben Franksen wrote:
> 
> Today I stumbled over this ingenious piece of programming in Darcs:
> 
>     toobig :: Int -> [a] -> Bool
>     toobig 0 _ = True
>     toobig _ [] = False
>     toobig n (_ : xs) = toobig (n - 1) xs
> 
> I must say I was dismayed. I mean, I have seen strange and wondrous things 
> in Darcs (more on that below) but the "length" function is in the blooming 
> Prelude for heaven's sake. How can stuff like that pass code review?

Bear in mind this isn't quite replaceable by length, because it'll
terminate early. You'd have to use genericLength on a lazy natural
type to get the same behaviour.

> Most modules under Darcs.UI and Darcs.Repository merely lump together a 
> bunch of functionally related procedures. There are no (useful, clever, 
> beautiful) abstractions to be found anywhere. Much of the code is needlessly 
> IO-centric and happily uses and mutates hidden state like 
> get/setCurrentDirectory (try "grep -rho CurrentDirectory src | wc -l", look 
> at the number, lean back, and try to conceive any possible rationale for it) 
> so that one carefully has to respect all the implicit, undocumented side-
> conditions w.r.t. the order of calls or it all falls apart. (I guess most of 
> the code is from a time before monad transformers became widely known and 
> used, not to speak of streaming instead of lazy IO, but that's another 
> story...)

get/setCurrentDirectory is a real problem because it makes darcs
impossible to use as a library from a multithreaded program. However, it
is also a significant optimisation because otherwise we'd have to
construct much larger absolute paths for each filesystem access. On the
other hand, perhaps nowadays this is a minor optimisation that we could
do without.

We did have a grand plan to introduce a "Darcs monad" that would
encapsulate this kind of hidden state, but when I had a go at
introducing it I ran into too many odd things darcs was doing so decided
a better strategy would be to try to get rid of those things over time
and then try again. The most significant were the HTTP downloading code
and the "atexit" handlers that are used for cleanup afterwards.

> However, I am more and more coming to the conclusion that repairing Darcs.UI 
> and Darcs.Repository piece by piece is hopeless and the only productive way 
> forward is to re-write it from scratch, perhaps salvaging bits of code here 
> and there.

I'd certainly be happy with a complete rewrite of significant chunks,
but I would mention that some progress has been made (IMO) by
incremental improvements over the years.

Inside Darcs.Repository, look at Darcs.Repository.State and
Darcs.Repository.Job for examples of things that we've separated out
from Darcs.Repository [Though I noticed the other day that there's been
a copy-and-paste job in Darcs.Repository.State to add look-for-moves and
look-for-replaces, that needs fixing :-(]

One reason I tend to favour incremental work myself is that my available
time comes and goes, and so if I try to do something major either I have
to do it on a branch and have lots of merge problems, or I have to have
something half-done in mainline with no clear end-point in sight for
everyone else.

> But after 
> 2.10 is out we should post-pone ideas for new features for a while and 
> concentrate on re-designing the UI and Repo code with the goal of inventing 
> cool and elegant abstractions and getting rid of the ugly and IO-centric 
> legacy.

I'm a bit reluctant to explicitly say "stop the world until we've
refactored", particularly as finishing the refactoring will depend on
volunteer time. In my experience big refactoring projects can end up
taking on a life of their own that stop anyone doing anything else for
too long. On the other hand stability is necessary for significant
changes - how about if we set a specific time limit on refactorings in a
given area?

> With the Repo part I am not yet familiar enough (its utter lack of 
> any high-level description makes this very difficult) to present anything 
> positively useful, but the UI part I grok fairly well now and I do have a 
> number of ideas for a new design, basically centering on a strict separation 
> between (user) interface and (command) logic, but still too raw for 
> presenting them coherently.

I think it'd be great. I've been wanting to evolve it that way myself,
in part to make it easier to use in more sophisticated ways e.g. from
darcsden.

> I guess realizing this plan will break complete 
> functional backwards compatibility (as e.g. expressed with the existing test 
> code), but I would be willing to accept that if the end result is that we 
> can extend Darcs with new features in a clean manner i.e. without heaping 
> ever more work-arounds on top of the mess.

I don't see any problem with the odd change to the command-line
interface to make the code more rational.

Cheers,

Ganesh