Drop unfinished support for binary & before/after diffs from VcsDiff

Summary:
VcsDiff and IBasicVersionControl::diff(...) many years ago got initial
support for requesting and storing raw copies of files changed in a
revision.

Nothing in KDevelop though makes use of that feature, and all the VCS
plugins themselves do not support it either, even ignore explicit requests.

To ease maintainance of the used and working code for creating unified
text diffs, this patch drops the dead code for that unfinished feature.
When someone ever starts working on it again, the needed structures
in VcsDiff can be easily readded and also done matching whatever approach
is taken then.

After the initial support was introduced in 2007 by e.g. commits
5ffc3c23 and 7b09fc2f
no further related work was ever commited to the codebase.

SvnDiffJob even has initial code for fetching full before-versions of
the affected files. But this seems premature test code which accidentally
got committed, as the full copies are always fetched and stored
additionally to the unified diff, and without respecting the diff type
parameter passed to the method. Also were the after-versions not stored,
and no other code also has ever been fetching those full copies
from the returned VcsDiff object.

Worse, most other VCS plugins simply ignored the diff type parameter passed
to the IBasicVersionControl::diff(...) method, and always returned a
unified diff, rendering the type parameter useless.

Reviewers: #kdevelop, apol

Reviewed By: #kdevelop, apol

Subscribers: kdevelop-devel

Differential Revision: https://phabricator.kde.org/D8962
parent aaccc44a
......@@ -17,6 +17,7 @@
#include <vcs/interfaces/ibasicversioncontrol.h>
#include <vcs/widgets/vcslocationwidget.h>
#include <vcs/vcsjob.h>
#include <vcs/vcslocation.h>
#include <QVBoxLayout>
......
......@@ -189,12 +189,11 @@ public:
/**
* Retrieves a diff between two revisions of a file
*
* The diff is in unified diff format for text files by default
* The diff is in unified diff format for text files.
*/
virtual VcsJob* diff( const QUrl& fileOrDirectory,
const VcsRevision& srcRevision,
const VcsRevision& dstRevision,
VcsDiff::Type = VcsDiff::DiffUnified,
IBasicVersionControl::RecursionMode recursion
= IBasicVersionControl::Recursive ) = 0;
......
......@@ -27,142 +27,102 @@
using namespace KDevelop;
void TestVcsDiff::setDiff(VcsDiff& diff,
VcsDiff::Type type,
VcsDiff::Content contentType,
const QString& diffString,
const QUrl& baseDiff,
uint depth,
const QHash<VcsLocation,QByteArray>& leftBinaries,
const QHash<VcsLocation,QByteArray>& rightBinaries,
const QHash<VcsLocation,QString>& leftTexts,
const QHash<VcsLocation,QString>& rightTexts)
uint depth)
{
diff.setType(type);
diff.setContentType(contentType);
diff.setDiff(diffString);
diff.setBaseDiff(baseDiff);
diff.setDepth(depth);
for (auto it = leftBinaries.begin(); it != leftBinaries.end(); ++it)
diff.addLeftBinary(it.key(), it.value());
for (auto it = rightBinaries.begin(); it != rightBinaries.end(); ++it)
diff.addRightBinary(it.key(), it.value());
for (auto it = leftTexts.begin(); it != leftTexts.end(); ++it)
diff.addLeftText(it.key(), it.value());
for (auto it = rightTexts.begin(); it != rightTexts.end(); ++it)
diff.addRightText(it.key(), it.value());
}
void TestVcsDiff::compareDiff(const VcsDiff& diff,
VcsDiff::Type type,
VcsDiff::Content contentType,
const QString& diffString,
const QUrl& baseDiff,
uint depth,
const QHash<VcsLocation,QByteArray>& leftBinaries,
const QHash<VcsLocation,QByteArray>& rightBinaries,
const QHash<VcsLocation,QString>& leftTexts,
const QHash<VcsLocation,QString>& rightTexts)
uint depth)
{
QCOMPARE(diff.type(), type);
QCOMPARE(diff.contentType(), contentType);
QCOMPARE(diff.diff(), diffString);
QCOMPARE(diff.baseDiff(), baseDiff);
QCOMPARE(diff.depth(), depth);
QCOMPARE(diff.leftBinaries(), leftBinaries);
QCOMPARE(diff.rightBinaries(), rightBinaries);
QCOMPARE(diff.leftTexts(), leftTexts);
QCOMPARE(diff.rightTexts(), rightTexts);
}
void TestVcsDiff::testCopyConstructor()
{
// test plain copy
const VcsDiff::Type type = VcsDiff::DiffRaw;
const VcsDiff::Content contentType = VcsDiff::Binary;
const QString diffString("diff");
const QUrl baseDiff("git://1");
const uint depth = 1;
const VcsLocation location("server");
const QHash<VcsLocation,QByteArray> leftBinaries({{location, "leftbinary"}});
const QHash<VcsLocation,QByteArray> rightBinaries({{location, "rightbinary"}});
const QHash<VcsLocation,QString> leftTexts({{location, "lefttext"}});
const QHash<VcsLocation,QString> rightTexts({{location, "righttext"}});
{
VcsDiff diffA;
setDiff(diffA,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
VcsDiff diffB(diffA);
compareDiff(diffA,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
compareDiff(diffB,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
}
const VcsDiff::Type typeNew = VcsDiff::DiffUnified;
const QString diffStringNew("diffNew");
// test detach after changing A
{
VcsDiff diffA;
setDiff(diffA,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
VcsDiff diffB(diffA);
// change a property of A
diffA.setType(typeNew);
diffA.setDiff(diffStringNew);
compareDiff(diffA,
typeNew, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffStringNew, baseDiff, depth);
compareDiff(diffB,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
}
}
void TestVcsDiff::testAssignOperator()
{
// test plain copy
const VcsDiff::Type type = VcsDiff::DiffRaw;
const VcsDiff::Content contentType = VcsDiff::Binary;
const QString diffString("diff");
const QUrl baseDiff("git://1");
const uint depth = 1;
const VcsLocation location("server");
const QHash<VcsLocation,QByteArray> leftBinaries({{location, "leftbinary"}});
const QHash<VcsLocation,QByteArray> rightBinaries({{location, "rightbinary"}});
const QHash<VcsLocation,QString> leftTexts({{location, "lefttext"}});
const QHash<VcsLocation,QString> rightTexts({{location, "righttext"}});
{
VcsDiff diffA;
setDiff(diffA,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
VcsDiff diffB;
diffB = diffA;
compareDiff(diffA,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
compareDiff(diffB,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
}
const VcsDiff::Type typeNew = VcsDiff::DiffUnified;
const QString diffStringNew("diffNew");
// test detach after changing A
{
VcsDiff diffA;
setDiff(diffA,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
VcsDiff diffB;
diffB = diffA;
// change a property of A
diffA.setType(typeNew);
diffA.setDiff(diffStringNew);
compareDiff(diffA,
typeNew, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffStringNew, baseDiff, depth);
compareDiff(diffB,
type, contentType, diffString, baseDiff, depth, leftBinaries, rightBinaries, leftTexts, rightTexts);
diffString, baseDiff, depth);
}
}
......
......@@ -35,25 +35,13 @@ private Q_SLOTS:
private:
void setDiff(KDevelop::VcsDiff& diff,
KDevelop::VcsDiff::Type type,
KDevelop::VcsDiff::Content contentType,
const QString& diffString,
const QUrl& baseDiff,
uint depth,
const QHash<KDevelop::VcsLocation,QByteArray>& leftBinaries,
const QHash<KDevelop::VcsLocation,QByteArray>& rightBinaries,
const QHash<KDevelop::VcsLocation,QString>& leftTexts,
const QHash<KDevelop::VcsLocation,QString>& rightTexts);
uint depth);
void compareDiff(const KDevelop::VcsDiff& diff,
KDevelop::VcsDiff::Type type,
KDevelop::VcsDiff::Content contentType,
const QString& diffString,
const QUrl& baseDiff,
uint depth,
const QHash<KDevelop::VcsLocation,QByteArray>& leftBinaries,
const QHash<KDevelop::VcsLocation,QByteArray>& rightBinaries,
const QHash<KDevelop::VcsLocation,QString>& leftTexts,
const QHash<KDevelop::VcsLocation,QString>& rightTexts);
uint depth);
};
#endif // KDEVPLATFORM_TESTVCSDIFF_H
......@@ -21,7 +21,7 @@
#include "vcsdiff.h"
#include <QString>
#include <QByteArray>
#include <QUrl>
#include <QSharedData>
namespace KDevelop
......@@ -30,14 +30,8 @@ namespace KDevelop
class VcsDiffPrivate : public QSharedData
{
public:
QHash<VcsLocation,QByteArray> leftBinaries;
QHash<VcsLocation,QByteArray> rightBinaries;
QHash<VcsLocation,QString> leftTexts;
QHash<VcsLocation,QString> rightTexts;
QUrl baseDiff;
QString diff;
VcsDiff::Type type = VcsDiff::DiffDontCare;
VcsDiff::Content content = VcsDiff::Text;
uint depth = 0;
};
......@@ -55,39 +49,7 @@ VcsDiff::VcsDiff( const VcsDiff& rhs )
bool VcsDiff::isEmpty() const
{
return d->diff.isEmpty() && d->leftBinaries.isEmpty() && d->rightBinaries.isEmpty()
&& d->leftTexts.isEmpty() && d->rightTexts.isEmpty();
}
VcsDiff::Type VcsDiff::type() const
{
return d->type;
}
VcsDiff::Content VcsDiff::contentType() const
{
return d->content;
}
QHash<VcsLocation, QByteArray> VcsDiff::leftBinaries() const
{
return d->leftBinaries;
}
QHash<VcsLocation, QByteArray> VcsDiff::rightBinaries() const
{
return d->rightBinaries;
}
QHash<VcsLocation, QString> VcsDiff::leftTexts() const
{
return d->leftTexts;
}
QHash<VcsLocation, QString> VcsDiff::rightTexts() const
{
return d->rightTexts;
return d->diff.isEmpty();
}
QString VcsDiff::diff() const
......@@ -101,56 +63,6 @@ void VcsDiff::setDiff( const QString& s )
d->diff = s;
}
void VcsDiff::addLeftBinary( const VcsLocation& loc, const QByteArray& b )
{
d->leftBinaries[loc] = b;
}
void VcsDiff::addRightBinary( const VcsLocation& loc, const QByteArray& b )
{
d->rightBinaries[loc] = b;
}
void VcsDiff::removeLeftBinary( const VcsLocation& loc )
{
d->leftBinaries.remove( loc );
}
void VcsDiff::removeRightBinary( const VcsLocation& loc )
{
d->rightBinaries.remove( loc );
}
void VcsDiff::addLeftText( const VcsLocation& loc, const QString& b )
{
d->leftTexts[loc] = b;
}
void VcsDiff::addRightText( const VcsLocation& loc, const QString& b )
{
d->rightTexts[loc] = b;
}
void VcsDiff::removeLeftText( const VcsLocation& loc )
{
d->leftTexts.remove( loc );
}
void VcsDiff::removeRightText( const VcsLocation& loc )
{
d->rightTexts.remove( loc );
}
void VcsDiff::setType( VcsDiff::Type t )
{
d->type = t;
}
void VcsDiff::setContentType( VcsDiff::Content c )
{
d->content = c;
}
VcsDiff& VcsDiff::operator=( const VcsDiff& rhs)
{
d = rhs.d;
......
......@@ -22,16 +22,14 @@
#ifndef KDEVPLATFORM_VCSDIFF_H
#define KDEVPLATFORM_VCSDIFF_H
//Needed first as it provides a hash-function for QHash
#include "vcslocation.h"
#include <QHash>
#include <QSharedDataPointer>
#include <QtGlobal>
#include <QMetaType>
#include "vcsexport.h"
class QUrl;
class QString;
class QByteArray;
namespace KDevelop
{
......@@ -39,62 +37,10 @@ namespace KDevelop
class KDEVPLATFORMVCS_EXPORT VcsDiff
{
public:
/**
* Specify the type of difference the diff() method should create. Note that a
* request for DiffUnified may not be honored, e.g. if the items being diffed are
* binary rather than text.
*/
enum Type
{
DiffRaw /**< Request complete copies of both items. */,
DiffUnified /**< Request copy of first item with diff. */,
DiffDontCare /**< Don't care; plugin will return whichever is easiest. */
};
enum Content
{
Binary /**< Binary diff, using the full content of both files.*/,
Text /**< Textual diff.*/
};
VcsDiff();
virtual ~VcsDiff();
VcsDiff( const VcsDiff& );
/**
* @returns the type of diff, i.e. raw or unified
*/
Type type() const;
/**
* @returns the content type, i.e. binary or text
*/
Content contentType() const;
/**
* @returns the binary content of the first file of the difference or
* an empty QByteArray if this is a textual diff
*/
QHash<KDevelop::VcsLocation, QByteArray> leftBinaries() const;
/**
* @returns the binary content of the second file of the difference or
* an empty QByteArray if this is a textual diff
*/
QHash<KDevelop::VcsLocation, QByteArray> rightBinaries() const;
/**
* @returns the textual content of the first file of the difference or
* an empty QString if this is a binary diff
*/
QHash<KDevelop::VcsLocation, QString> leftTexts() const;
/**
* @returns the textual content of the second file of the difference or
* an empty QString if this is a unified or binary diff
*/
QHash<KDevelop::VcsLocation, QString> rightTexts() const;
/**
* @returns the difference between the first and the second file in
* unified diff format or an empty QString if this is a binary diff
......@@ -117,18 +63,7 @@ public:
void setDepth(const uint depth);
void setDiff( const QString& );
void addLeftBinary( const KDevelop::VcsLocation&, const QByteArray& );
void removeLeftBinary( const KDevelop::VcsLocation& );
void addRightBinary( const KDevelop::VcsLocation&, const QByteArray& );
void removeRightBinary( const KDevelop::VcsLocation& );
void addLeftText( const KDevelop::VcsLocation&, const QString& );
void removeLeftText( const KDevelop::VcsLocation& );
void addRightText( const KDevelop::VcsLocation&, const QString& );
void removeRightText( const KDevelop::VcsLocation& );
void setType( Type );
void setContentType( Content );
VcsDiff& operator=( const VcsDiff& rhs);
/** @returns whether there are not changes on the diff */
......
......@@ -49,6 +49,7 @@
#include <vcs/widgets/vcscommitdialog.h>
#include <vcs/vcsjob.h>
#include <vcs/vcsrevision.h>
#include <vcs/vcslocation.h>
#include <vcs/vcsdiff.h>
#include "interfaces/idistributedversioncontrol.h"
......
......@@ -63,10 +63,7 @@ public:
delete patch;
}
qCDebug(VCS) << "diff:" << diff.leftTexts().count();
qCDebug(VCS) << "diff:" << diff.diff();
qCDebug(VCS) << "diff:" << diff.type();
qCDebug(VCS) << "diff:" << diff.contentType();
m_ui->diffDisplay->setPlainText( diff.diff() );
m_ui->diffDisplay->setReadOnly( true );
......
......@@ -29,6 +29,7 @@
#include <interfaces/iplugincontroller.h>
#include <interfaces/iplugin.h>
#include <vcs/vcslocation.h>
#include <vcs/widgets/vcsimportmetadatawidget.h>
#include <vcs/interfaces/ibasicversioncontrol.h>
......
......@@ -29,6 +29,7 @@
#include <vcs/widgets/standardvcslocationwidget.h>
#include <vcs/dvcs/dvcsjob.h>
#include <vcs/vcsstatusinfo.h>
#include <vcs/vcslocation.h>
#include <vcs/dvcs/ui/dvcsimportmetadatawidget.h>
#include <interfaces/contextmenuextension.h>
#include <interfaces/context.h>
......@@ -120,7 +121,7 @@ VcsJob* BazaarPlugin::createWorkingCopy(const VcsLocation& sourceRepository, con
return job;
}
VcsJob* BazaarPlugin::diff(const QUrl& fileOrDirectory, const VcsRevision& srcRevision, const VcsRevision& dstRevision, VcsDiff::Type, IBasicVersionControl::RecursionMode recursion)
VcsJob* BazaarPlugin::diff(const QUrl& fileOrDirectory, const VcsRevision& srcRevision, const VcsRevision& dstRevision, IBasicVersionControl::RecursionMode recursion)
{
Q_UNUSED(recursion);
VcsJob* job = new DiffJob(BazaarUtils::workingCopy(fileOrDirectory), BazaarUtils::getRevisionSpecRange(srcRevision, dstRevision), fileOrDirectory, this);
......
......@@ -50,7 +50,7 @@ public:
KDevelop::VcsJob* copy(const QUrl& localLocationSrc, const QUrl& localLocationDstn) override;
KDevelop::VcsImportMetadataWidget* createImportMetadataWidget(QWidget* parent) override;
KDevelop::VcsJob* createWorkingCopy(const KDevelop::VcsLocation& sourceRepository, const QUrl& destinationDirectory, RecursionMode recursion=Recursive) override;
KDevelop::VcsJob* diff(const QUrl& fileOrDirectory, const KDevelop::VcsRevision& srcRevision, const KDevelop::VcsRevision& dstRevision, KDevelop::VcsDiff::Type, RecursionMode recursion=Recursive) override;
KDevelop::VcsJob* diff(const QUrl& fileOrDirectory, const KDevelop::VcsRevision& srcRevision, const KDevelop::VcsRevision& dstRevision, RecursionMode recursion=Recursive) override;
KDevelop::VcsJob* init(const QUrl& localRepositoryRoot) override;
bool isVersionControlled(const QUrl& localLocation) override;
KDevelop::VcsJob* log(const QUrl& localLocation, const KDevelop::VcsRevision& rev, long unsigned int limit) override;
......
......@@ -11,6 +11,7 @@
#include "cvsdiffjob.h"
#include <QRegExp>
#include <QUrl>
CvsDiffJob::CvsDiffJob(KDevelop::IPlugin* parent, KDevelop::OutputJob::OutputJobVerbosity verbosity)
: CvsJob(parent, verbosity)
......@@ -29,11 +30,6 @@ QVariant CvsDiffJob::fetchResults()
diff.setDiff( output() );
/// @todo check output of "cvs diff" if it reported binary files
diff.setContentType( KDevelop::VcsDiff::Text );
/// @todo hmmm, we always call "cvs diff" with it's -u option
/// but if this option would be omitted cvs would return an other format
diff.setType( KDevelop::VcsDiff::DiffUnified );
return qVariantFromValue( diff );
}
......
......@@ -392,7 +392,7 @@ KDevelop::VcsJob * CvsPlugin::commit(const QString & message, const QList<QUrl>
return job;
}
KDevelop::VcsJob * CvsPlugin::diff(const QUrl & fileOrDirectory, const KDevelop::VcsRevision & srcRevision, const KDevelop::VcsRevision & dstRevision, KDevelop::VcsDiff::Type, KDevelop::IBasicVersionControl::RecursionMode)
KDevelop::VcsJob * CvsPlugin::diff(const QUrl & fileOrDirectory, const KDevelop::VcsRevision & srcRevision, const KDevelop::VcsRevision & dstRevision, KDevelop::IBasicVersionControl::RecursionMode)
{
CvsJob* job = d->m_proxy->diff(fileOrDirectory, srcRevision, dstRevision, QStringLiteral("-uN")/*always unified*/);
return job;
......
......@@ -71,7 +71,6 @@ public:
KDevelop::VcsJob* diff(const QUrl& fileOrDirectory,
const KDevelop::VcsRevision& srcRevision,
const KDevelop::VcsRevision& dstRevision,
KDevelop::VcsDiff::Type,
KDevelop::IBasicVersionControl::RecursionMode = KDevelop::IBasicVersionControl::Recursive) override;
KDevelop::VcsJob* log(const QUrl& localLocation,
const KDevelop::VcsRevision& rev,
......
......@@ -29,6 +29,7 @@
#include <interfaces/icore.h>
#include <interfaces/iplugincontroller.h>
#include <vcs/interfaces/ibasicversioncontrol.h>
#include <vcs/vcslocation.h>
#include <KLocalizedString>
#include <ghaccount.h>
......
......@@ -39,6 +39,7 @@
#include <vcs/vcsjob.h>
#include <vcs/vcsrevision.h>
#include <vcs/vcsevent.h>
#include <vcs/vcslocation.h>
#include <vcs/dvcs/dvcsjob.h>
#include <vcs/vcsannotation.h>
#include <vcs/widgets/standardvcslocationwidget.h>
......@@ -385,10 +386,8 @@ KDevelop::VcsJob* GitPlugin::status(const QList<QUrl>& localLocations, KDevelop:
}
VcsJob* GitPlugin::diff(const QUrl& fileOrDirectory, const KDevelop::VcsRevision& srcRevision, const KDevelop::VcsRevision& dstRevision,
VcsDiff::Type /*type*/, IBasicVersionControl::RecursionMode recursion)
IBasicVersionControl::RecursionMode recursion)
{
//TODO: control different types
DVcsJob* job = new GitJob(dotGitDirectory(fileOrDirectory), this, KDevelop::OutputJob::Silent);
job->setType(VcsJob::Diff);
*job << "git" << "diff" << "--no-color" << "--no-ext-diff";
......
......@@ -104,7 +104,7 @@ public:
KDevelop::IBasicVersionControl::RecursionMode recursion = KDevelop::IBasicVersionControl::Recursive) override;
KDevelop::VcsJob* diff(const QUrl& fileOrDirectory, const KDevelop::VcsRevision& srcRevision, const KDevelop::VcsRevision& dstRevision,
KDevelop::VcsDiff::Type, RecursionMode recursion) override;
RecursionMode recursion) override;
KDevelop::VcsJob* log( const QUrl& localLocation, const KDevelop::VcsRevision& rev, unsigned long limit) override;
KDevelop::VcsJob* log(const QUrl& localLocation, const KDevelop::VcsRevision& rev, const KDevelop::VcsRevision& limit) override;
......
......@@ -574,7 +574,7 @@ void GitInitTest::testDiff()
VcsRevision srcrev = VcsRevision::createSpecialRevision(VcsRevision::Base);
VcsRevision dstrev = VcsRevision::createSpecialRevision(VcsRevision::Working);
VcsJob* j = m_plugin->diff(QUrl::fromLocalFile(gitTest_BaseDir()), srcrev, dstrev, VcsDiff::DiffUnified, IBasicVersionControl::Recursive);
VcsJob* j = m_plugin->diff(QUrl::fromLocalFile(gitTest_BaseDir()), srcrev, dstrev, IBasicVersionControl::Recursive);
VERIFYJOB(j);
KDevelop::VcsDiff d = j->fetchResults().value<KDevelop::VcsDiff>();
......
......@@ -26,6 +26,8 @@
#include <interfaces/icore.h>
#include <interfaces/iplugincontroller.h>
#include <vcs/interfaces/ibasicversioncontrol.h>
#include <vcs/vcslocation.h>