unifying libdiff2

[Aleix Pol]

On Fri, Oct 28, 2011 at 4:27 AM, Jeremy Whiting <jpwhiting at kde.org> wrote:

> 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?
>
Isn't kdesdk moving to git at all? I think depending on such a big module is
bad and I don't see the benefit of not having a libdiff project.


> I see in the patchreview plugin there is some #if 0 around #ifdef
> HAVE_KOMPARE could that dependency come back?
>
That was something different (trying to integrate the KPart), I don't think
the patchreview can be built without libdiff. We can make the patchreview
building optional if libdiff is not found, for sure.


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

I'm not sure why is that, probably we can have them if they don't appear in
the UI.


>
> > 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
>
> --
> KDevelop-devel mailing list
> KDevelop-devel at kdevelop.org
> https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel
>
>
Thanks a lot for your work, Jeremy, I don't like to have two competing code
bases in KDE, it's healthier to share both binaries and source code, there's
no doubt of that.

Regarding dependencies, I'd like to note that in KDE we're shifting to aim
for smaller dependencies, I won't make any speech out of that, I just think
other people might be interested in this library too and a small dependency
is always better.

I hope that helped :)
Aleix
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kompare-devel/attachments/20111028/a5429c76/attachment-0001.html>