VCS Interface classes

[Andreas Pakulat]

On 30.04.07 12:06:55, Matthew Woehlke wrote:
> Andreas Pakulat wrote:
> > I present to you our new VCS interface classes (that I finally managed
> > to write up, based on the enourmus thread some time ago).
> > 
> > * major problems for a given system with the proposed interface
> 
> IMO IBasicVCS is missing unedit(). Jakob already pointed out the gaping 
> flaw in commit() ;-).

What should unedit() do? I mean, revert() reverses to repository
version, commit() checks in the local version into repository. What else
is needed?

> Come to think of it, it would rock if we handled change sets... yes, 
> this is harder to do in e.g. svn since the plug-in would have to manage 
> it rather than being able to leverage VCS functionality, but I know /I/ 
> at least would find it really nice to have change sets in svn. :-)

Please feel free to present an interface for that. It shouldn't be part
of our basic - all vcs plugins have to do this - interface, IMHO.

> Is VCSState::Unknown equivalent to 'not under version control', or 
> should there be a seperate status (as I think there is in 3.4) for 
> 'unable to determine that information'? Or does that only need to exist 
> in the UI side?

I'm not sure, what is it used for in 3.4? I just took the 3.4 VCSState
and removed the stuff that is not of much interest or just doesn't map
to all vcs'es.

> VCSMapping is insufficient for perforce which needs exclusion as well as 
> inclusion rules. Is this just not available outside of a private (i.e. 
> not-matching-the-interface), perforce-specific checkout()? (Or is it in 
> the missing CliencMapping class? :-))

ClientMapping == VCSMapping, seems I forgot one or two places to rename
:)

So what you need for Perforce is something like

class VCSMapping
{
	addMapping( repoLocation, localLocation, recursive);
	addExclude( repoLocation, recursive);
}

(ommitting the other methods, like excludes() and so on), where the
checkout would check out all mappings, unless a mapping or part of it
matches an exclude? I'm fine with adding that. But I guess the VCS
interface then needs a function like:

supportsExcludes();

which tells the client wether the exclude's will be used or not. Because
else this is hard to do in svn plugin.

> > * return types for the methods, we need a way to communicate at least
> >   errors back, but I'm not sure how to do that best given that the 
> >   methods should execute asynchronously
> 
> Change void to int, return 0 for success, <0 for error (possibly allow 
> the return code to be implementation defined?). Or have an interface to 
> retrieve an error.

Actually I think 2 signals:

finished( Command );
error( Command, Message);

should be fine (command is an enum for the various methods we have).

> I'm not sure about async, I would image async in a script is not really 
> what you want :-). Therefore it seems like maybe we need both?

Yeah, I thought so to. For sync methods we'd return an error code +
message (like QPair<int,msg>) and for async there are the two signals.

> Certainly I would guess there should be a statusAsync() based on
> previous discussion of svn in 3.4.

Well, checkout also falls into the category of async methods, you don't
really want to be interrupted hacking kdevelop just because you check
out kdelibs where you want to do something later on. I'd like to see
mostly all things in an async way. Eric4 just gained the possibility to
have async commit() and that way you can initiate a commit() and then go
back to the IDE and look at the diff of your local changes to write a
proper commit message. Which IMO rocks. Not even Eclipse has this, IIRC.

> IMO it would be ideal if we can require plug-ins to be thread safe and
> have a generic async wrapper around whichever plugin. Or just allow
> inheriting a base class that uses the virtual synchronous methods to
> do async and allow a plug-in to overload as needed?

Not sure I follow you here.

> For move():
> > * depending on the VCS, only preserves history if copy preserves history
> 
> ...I thought I remember someone saying this is not necessarily the case 
> (as nonsensical as that seems), i.e. move might preserve history while 
> copy does not? Anyway that's just a doc nitpick.

I guess we should just add "might preserve history or not, depending on
the VCS" to both move and copy.

> > * Parameters for push/pull in the distributed vcs iface, no idea whats
> >   needed there
> > * diff and log parameters, am not 100% sure there
> 
> Should diff() pop up a UI, or return something useful in scripts?

The first I think. Mainly because representing a diff is not that easy.
For example a diff on binary files is not possible with svn, but might
be with other VCS systems and then you can hardly represent that
in a meaningful way, except with a GUI. (I'm thinking of KDE's commit
digest which has visual diff for .ui files)

> > * usage of simple QString's for any parameter that can be a repository
> >   url (anything that is always a local url should stay KUrl), if a VCS
> >   system can't use a url for describing a repository location
> 
> ...if diff() takes a repo location shouldn't the parameter be QString? 

Thats something I wasn't sure about, so I made my first write-up easier
by sticking to KUrl everywhere ;) The thing is I don't know wether the
"targeted" vcs systems (i.e. the ones we were talking about here) really
work with a url scheme. If not repo location needs to turn into a
QString and then we really need 2 different interfaces.

> (Is it useful to have a KUrl /and/ QString version to make it easier to 
> know if you have a url or a repo path?) Same comments for log().

See above and other mails from me, I think I just forgot last night that
I wanted to split the interfaces for local locations and repo locations.

Andreas

-- 
Beware of a tall blond man with one black shoe.