VCS Interface classes

[Matthew Woehlke]

Andreas Pakulat wrote:
> 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?

unedit() clears the effect of edit() iff* diff(BASE,WORKING) is empty, 
i.e. the file has been marked for edit but does not actually differ from 
the repo copy.

(*"if and only if" for you non-math-types :-))

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

I'll need to think about it for a bit. :-) Perforce almost has to have 
this. For VCS's that don't support it, it should be possible to write a 
generic version that uses the local file system to provide this 
functionality.

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

It is/would be used for "hasn't been queried yet" (esp. with an async 
status()), or for things that the VCS plugin wasn't consulted on, or 
didn't return an answer... I imagine the same cases would apply, but as 
I said all of those seem to be statuses that only apply to the UI, i.e. 
the VCS plugin would never return such a status.

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

Yes, you need some sort of 'what do you support' function. IIRC we had 
originally talked about using flags for the mapping instead of 'bool 
recursive'; you would then ask for supported flags. This would be most 
extensible in case some VCS supports other things as well as, or instead 
of, exclusions.

>>> * 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).

Hmm... ok, that makes sense, but what if you issue several calls to 
status()? I think we should return something more like a pid to use in 
the signal. In fact, that should work, because then all methods can be 
made async() and we can add a wait() method. I'm not sure how the 
scripting will work but it seems that it will be more common for scripts 
to want to be synchronous because I would think they usually rely on the 
previous command to complete before they can issue the next. I'm not 
necessarily saying we should change anything, just something to keep in 
mind.

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

See above; we could use task ID's and wait(tid). Scripts can either use 
this or we can somehow arrange a layer of indirection for scripts.

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

Agreed. I never said status should be the *only* async method. :-) Just 
that there should definately be an async status().

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

Isn't that just a matter of the pending commit() not blocking the rest 
of the interface, and the dialog being non-modal? p4v (the perforce gui) 
has been this way for I think as long as I have been using it. Basically 
until you hit 'ok' you really haven't done anything. 'svn ci' locks the 
list of files to commit but IMO we should take after p4v and lift this 
restriction, i.e. allow you to change what files are being committed as 
you are writing the message (this is one feature in p4v I really miss in 
svn, even 'p4 submit' allows you to change your mind while in the child 
editor process). p4v even updates the list of files that you *might* 
commit in real time, i.e. if you start a commit and then edit another 
file. I see no reason other plug-ins couldn't do this. (For ones without 
an explicit 'edit', it's ok if they don't always get file change 
notifications, but it's simple for edit() to add a file to the list of 
might-commit.)

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

If we only have async methods (and a wait()) you can safely ignore most 
of that, plug-ins will have to be thread-safe anyway. :-)

Basically, a plug-in has to support doing multiple instances of any 
combination of synchronous operations at the same time. One way to 
achieve this would be to only implement synchronous operations that are 
thread-safe and reentrant-safe, and inherit a class that wraps the 
synchronous calls around async invokers.

Does that make sense?

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

Right, something like that. :-)

>>> * 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)

...but a .ui isn't a binary file ;-). I guess I can live with that, 
though. :-)

>>> * 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 'svn+ssh://path/to/somewhere' a valid KUrl? (I guess if '+' is 
allowed in a protocol name and the protocol doesn't need to be known.) 
Perforce repo paths, however, look like '//path/to/somewhere'. Of course 
if you needed to do something silly like use 'p4://path/to/somewhere' in 
all external code, I don't see a problem with that, the back end can 
just strip the protocol.

-- 
Matthew
If you believe you received this e-mail in error, you are probably sadly 
mistaken, but if not, aren't you lucky?