Need better wrappers for Ediff

[tarsius]

Magits support for Ediff is severely lacking. I know, I know, many people think it is great, but all that greatness comes from Ediff itself, the wrappers that Magit provides suck.

The Ediff specific code in Magit can be split into two independent halves: one for conflict resolution (aka resolve) and one for just comparing different versions (aka compare).

Resolve and compare don't share any code. So effectively we have two independent Ediff wrappers in Magit. The only way in which they are connected is that the main entry point of compare can decide to step down and let resolve do its thing instead.

There are some things that are done for both resolve and compare, but this isn't done by sharing code. Instead these things are implemented twice.

Then there are things that are only done for resolve or compare. But these things are either unnecessary (and should therefor be removed) or they are actually helpful and should therefor be done in both cases (using shared code).

These things might not matter much for users but for me as the maintainer it is very bothersome to be confronted with not only one broken implementation but two, which are in random ways different from each other.

Ignoring the fact that there are two separate parts, each part even when looked at in isolation has serious issues.

Resolve

Resolve always starts from scratch. It doesn't matter whether Git has already resolved most conflicts, Magit always starts by getting the two commits to be merged and their merge-base from Git, then passes that to Ediff. Ediff then does some conflict resolution, which is not terrible but certainly not as good as that of Git.

We Git users have to trust git merge. If that says that it was able to resolve something then we should believe it in 99% of all cases - else we might just as well go back to svn.

Now I am not saying that it is entirely useless to provide a way to look at all differences including those already resolved by Git, but that's a "nice to have feature". The feature we need is "show the unresolved conflicts in Ediff so that I can manually resolve them". So the essential feature is missing and instead we have a feature which is only really useful in a few cases.

This has been reported in #491, over a year ago.

Compare

Just comparing two commits (or two commits plus their merge-base) should be trivial to do. Basically all Magit has to do is determine the two/tree revisions, populate buffers with them, and then tell Ediff to compare them.

Unfortunately the current implementation comes with some misguided "dwim" functionality. The main entry point, magit-ediff does neither try to guess the revisions itself nor does it ask the user for help.

Instead compare relies on fairly unrelated code to prepare that information ahead of time and store it in strange places. Later compare checks under some rock and uses the information it finds there. This is problematic for several reasons.

• It forces other code to be uglier than it need to be. The information that compare needs does not have to be prepared ahead of time, we could just as well determine it once we actually need it.
• The information prepared on behalf of compare is bad, e.g. it assumes a range can be represented as a cons-cell, but there e.g. is a difference between A..B and A...B.
• The fact that these unrelated pieces of code need to prepare data for compare in turn makes it impossible to refactor these pieces without breaking compare. And as it so happens these pieces of code are not exactly the best parts of Magit and actually need to be refactored.

So on the next branch I have decided to break out of this "chicken and egg problem" and removed compare without a replacement. This will have to be replaced before before next can be merged into master but I could not see a way of getting there without first nuking the old compare implementation.

But wait there is more, if you use this now you will also get this:

• If compare cannot find the information that it needs it will either go "I cannot do that Dave" or pretend everything is find and then refuse to open the bay. Obviously it should instead say "I am not sure what you expect of me, could you please tell me more".

The recently opened #1257 is an incarnation of this, but actually there are uncountable ways in which this could go wrong.

Stage

Actually there is a third part to Magit's Ediff support. Support for staging hunks has been bolted onto compare. Just staging though - unstaging doesn't work. So it's fairly useless.

Staging and unstaging has been requested in #256, over three years ago.

So how can we fix that?

As I have said I have removed compare from next to make it possible to fix other things. That means it has to be substituted before next can be merged into master.

That sucks. It might mean that I will have to write a new implementation that I am still not happy with, just so that I can merge without being criticized for "needlessly removing essential features".

I have not removed resolve from next, because unlike compare it is self-contained. So while I have no use for it in its current form there is no need for it to be removed. Unlike compare it does not cause any harm by making refactoring of theoretically unrelated things impossible/much harder/less atomic.

It also means that I am not going to work anymore on the old implementation. I am closing all issues that point out one of the above problems. Users will have to wait for me to write a new implementation. And that currently isn't a top priority.

So you wanna work on this?

Of course anyone is welcome to bang on the current implementation or (much preferred) to write a new implementation.

I would welcome that, but the time for some incremental improvements is over. You either implement something that is designed from the ground up or you let it be. Also be prepared that I will be rather critical and will only merge something that fixes most if not all of these issues. You should probably contact me before working on this. Some hints:

• Your implementation has to be safe from users saying "you have removed this super great feature" or my saying "this is still crap". Combining the two won't be easy.
• You should use magit-show to populate buffers with SOMEREV:SOMEFILE but be prepared for that to be replaced with something much better eventually. Have a look at magit-tramp for one way the "gimme some revision" functionality could be improved (but don't rely on it just yet, we might do it differently).
• For resolve you don't want to do use magit-show. If you don't understand why after the above, then don't even bother.
• Regular diff buffers contain information that can be used to "dwim". I intend to improve regular diff soon, so don't hardcode these parts to much.
• In fact you might want to leave out all the "lets guess that the user wants" stuff out initially and concentrate on getting the basics right.
• Try to implement this without requiring changes to existing code, unless these changes simplify the existing code.
• Work of the next branch.

[tarsius]

The new shiny implementation has arrived, see #1429.