Patch for preliminary Mercurial integration in kdevplatform

[Andreas Pakulat]

On 09.03.09 09:07:22, Fabian Wiesel wrote:
> First, DVCSjob is changed from parsing the output for lines and later
> rejoining them to a single string to simply accumulating a QByteArray.

While thats fine in general it puts a lot of burden onto the plugin
writer as he has to convert the bytearray into a string. If you look at
ProcessLineMaker in utils you'll see that thats not quite as easy as one
might think. Individual comments below.

>  QString DVCSjob::output() const
>  {
> -    return d->outputLines.join("\n");
> +    return QString(rawOutput());

Thats for example wrong. If rawOutput contains non-ascii text this will
break horribly. You need at least QString::fromLocal8Bit(), but even
that might break because the output might contain an unfinished line
thats stops in the middle of a character sequence (think for example 2/3
byte utf8). So you should only return the output until and including the
last included "\n" and convert that with fromLocal8Bit().

> +bool MercurialExecutor::isValidDirectory(const KUrl & directory)
> +{
> +    std::auto_ptr<DVCSjob> job(new DVCSjob(vcsplugin));
> +
> +    if (!prepareJob(job.get(), directory.toLocalFile(), MercurialExecutor::Init)) {
> +        return false;
> +    }
> +
> +    *job << "hg" << "status" << "-q";
> +
> +    return job->exec() && job.release()->status() == KDevelop::VcsJob::JobSucceeded;

Just curious whats the point of using std::auto_ptr here?

> +DVCSjob* MercurialExecutor::init(const KUrl &directory)
> +{
> +    std::auto_ptr<DVCSjob> job(new DVCSjob(vcsplugin));
> +
> +    if (!prepareJob(job.get(), directory.toLocalFile(), MercurialExecutor::Init)) {
> +        return NULL;
> +    }
> +
> +    *job << "hg" << "init";
> +
> +    return job.release();

Or especially here as you're giving it away anyway.

> +void MercurialExecutor::parseOutput(const QByteArray& jobOutput, QList<DVCScommit>& commits) const
> +{
> +    static unsigned int entriesPerCommit = 6;
> +    QList<QByteArray> items = jobOutput.split('\0');

Interesting, mercurial supplies \0 as separation character. Thats rather
nice for stdin/stdout communication.

Other than that the code seems ok (I didn't do a really deep review as I
don't know too much about the dvcs stuff). Are you willing to maintain
this if it goes into kdevplatform? Do you already have an svn account?

Andreas

-- 
You're at the end of the road again.