unifying libdiff2

[Jeremy Whiting]

On Thu, Oct 27, 2011 at 7:14 PM, Kevin Kofler <kevin.kofler at chello.at> wrote:
> 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.

>From what I understand they (kdevelop devs) told me it was just a
misunderstanding.  So lets fix it :)

>
>> 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.

That would probably work.  KDevelop guys, is there a problem with
depending on kdesdk?
I see in the patchreview plugin there is some #if 0 around #ifdef
HAVE_KOMPARE could that dependency come back?

>> 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.

Awesome.

>> 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.

Cool.

>> 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?

Good question. kdevelop guys?

> 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.)

That doesn't sound too tricky to fix.  If you don't get to it I'll
take a look soonish.

>> 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
>

Agreed.

That said in comparing the two versions I merged some things locally
from kdevelop version to kdesdk version.  I've attached a patch to
kdesdk version with those improvements.  I suggest these changes get
applied to kdesdk version and after the dependency issue is resolved
the forked version go away.

BR,
Jeremy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: komparefixes.diff
Type: text/x-patch
Size: 6349 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kompare-devel/attachments/20111027/ddeedd09/attachment.diff>