Patch for preliminary Mercurial integration in kdevplatform

[Andreas Pakulat]

On 14.03.09 13:46:49, Fabian Wiesel wrote:
> On Fri, 13 Mar 2009 22:14:50 +0100
> Andreas Pakulat <apaku at gmx.de> wrote:
> > Thats for example wrong. If rawOutput contains non-ascii text this
> > will break horribly. 
> 
> I admit, I haven't thought about non-single-charsets. Fixed that in the
> new version (now submitted over commited).
> 
> Speaking about multi-byte character sets, doesn't the approach in
> ProcessLineMaker has its own problems?

It better not has, I've written that some time ago and ran it by one of
KDE's "encoding guru's" (Oswald Buddenhagen, maintains KProcess in
kdelibs).

> First one, what happens, when the output is not terminated by a final
> newline? As it currently stands, the final will not be passed by
> output().

Thats why you have to call flushBuffers or discardBuffers when the process
ends. Unfortunately there's no way to make that work automatically, as the
linemake cannot use the process safely in the slot connected to the exit
signals (as the user of the process might already have deleted the process
at that point).

> Second, do we care about about 2-byte (or four-byte) character sets?

Of course we do, the default charset on any modern linux distribution is
2bytes on anything except us-ascii characters (i.e. utf-8). Of course you
don't necessarily need to care if you can enforce the app that you're
running to output only us-ascii.

> > [...]
> > 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.
> 
> I like RAII, and have an aversion of unowned pointers.
> As I gathered from the code DVCSPlugin, a job has to be deleted unless
> it is started. Then the job itself will take ownership. When I return
> the pointer, I will relinquish the ownership to the caller.
> Should prepareJob() (or anything else before returning the pointer fail,
> I have to delete the pointer.
> I'd rather do that by an RAII-object than manually, especially since it
> is practically the same amount of code. Doing so even in a three-liner
> is just a matter of consistency.
> 
> Of course, I can change that, if it goes against code-style here.

No, I was mostly just curious and I do agree actually (didn't think about
that). Oh but one thing: Please use 0 instead of NULL as return value,
thats in fact convention of our code in general.
 
> > 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?
> 
> Generally, yes. I could maintain it. I don't have an SVN account yet.

Nice :), so if you think you've got a patch that should be added let me know and
I'll commit it as a start. Feel free to apply for an svn account (see the
techbase website for how that works) as well as it'll make maintainence a
lot easier :)

Andreas

-- 
Stay away from hurricanes for a while.