unifying libdiff2

[Kevin Kofler]

On Friday 28 October 2011, Jeremy Whiting wrote:
> Sorry for cross-posting, I'm not sure who is on which list.  We can
> continue this discussion on one or the other, but I wanted anyone
> interested to be able to chime in.

I'm only on kompare-devel. I'm the current maintainer of Kompare.

> As you may or may not know, libdiff2 is used in kdesdk/kompare and
> kdevelop/kdevplatform/plugins/patchreview (one is a fork/copy of the
> other).  I'd like to bring these back together into one library that
> is exported like a real library so both places can use it.

I'm very much in favor of that. In fact, I still don't understand why KDevelop 
forked the library in the first place, since I accepted all the API additions 
they sent me patches for, and also told them that I won't complain if they 
just commit the API additions directly as long as they don't break Kompare.

> Where it lives is still up for debate, but probably kdelibs or kdesupport or
> something.

Why can't kdevelop just depend on kdesdk? It's the natural place for libdiff2 
to live. And there's already the optional dependency on Okteta which is also 
in kdesdk. For packagers (and there I can speak with my Fedora packager hat 
on), optional vs. required doesn't make that much of a difference, we want to 
build against kdesdk anyway.

> In komparemodellist.cpp saveDestination the version in kdesdk checks
> for opening a file and emits an error if it can't open.  Curiously the
> version in kdevelop does the opposite, it sends an error if it is able
> to open the file.  My only guess is that this code is not used in the
> kdevelop patchreview code.  If anyone knows why it is the way it is in
> kdevelop copy I'd like to hear otherwise I'll use the version from
> kdesdk.

IIRC, this is a bug we fixed in Kompare, the fix didn't reach the KDevelop fork.

> Towards the end of that method in the kdevelop version it has
> an if false where in the kdesdk version it checks for a temp close
> error, not sure why.

That, too, is a bugfix in Kompare which didn't reach the fork.

> Similarly in the kompare version of komparemodellist.cpp there are a
> bunch of actions in the ctor that are just commented out in the
> kdevelop version.  Maybe komparemodellist should move out of libdiff2
> and into kompare itself?  Alternatively we could check at runtime if
> our parent is a KomparePart and initialize the actions only then, so
> kdevelop could still use the model.

Now the question is, why are the actions not wanted in KDevelop?

Of course, we could use qobject_cast or QObject::inherits or something like 
that on the parent.

> One final difference is that the kompare version has the idea of a
> malformed patch and keeps track of that throughout komparemodellist,
> parser, and parserbase.  My instinct is to take the kdesdk version as
> that seems like an addition.

Yes, this is something I added to Kompare.

That said, there's a known issue with this: the detection is too strict and 
reports some patches as malformed when they aren't actually malformed, but 
just have legitimate text such as "Binary files foo and bar differ" between 
hunks. I haven't gotten around to fixing this. (I think it should only complain 
about junk lines if they're diff lines, i.e. start with a ' ', '+', '-', '<', 
'>' or such character.)

> comments on any of the above are welcome.

My main comment: folks, forking is bad. Don't do it! Just depend on libdiff2 
from kdesdk, packagers can easily accomodate that.

        Kevin Kofler