VCS Interface classes

[Matthew Woehlke]

Andreas Pakulat wrote:
> On 01.05.07 12:40:09, Matthew Woehlke wrote:
>> Andreas Pakulat wrote:
>>> I've updated the interfaces to reflect the stuff that was discussed
>>> until now.
>> To clarify: isVersionControlled does not distinguish between "no" and "an error 
>> occurred", or...?
> 
> No it doesn't.

Ok.

>> Would it be possible to change the returns from int to something that more 
>> clearly indicates that the value is a task ID? A typedef would be fine, 
>> although I can see using some form of class so that you could query e.g. the 
>> result, status, etc, from the task object (i.e. more like a job). Hmm, that 
>> would mean wait() would become a member of the task, I guess.
> 
> Hmm, I'm starting to think we should have a VCSJob here, subclassed from
> KJob (which is already the case in svn plugin - AFAIK).

That would work, I was leaning that way already. :-) (And it might make 
it easier to write the plugins :-).)

>> Should localRevision() have a default for the second argument? (I'm not sure if 
>> this needs to be in the interface?) If this is to be synchronous (should it 
>> be?), Revision needs a way to specify that an error occurred.
> 
> Invalid Revision should do for that, no?

What's "Invalid Revision"?

>> Thinking more about RevisionType, I don't think the current system works. I 
>> think we need to say that plug-ins will subclass Revision and provide an 
>> overload of RevisionType to add their own values.
> 
> Then we should use QFlag(s), those can be extended. However how does a
> user of the vcs plugin get to know that? Right, it doesn't. Users of
> our VCS interface only have the interface. 

QFlags is OK. If you are using the generic interface I don't think you 
should have access to any revision types that are not "universal"? 
That's what I was saying; if you need something more specific, you need 
to know what plugin you are using and use its interface directly. 
(But... see below)

> [snip] although code doesn't really have a way to find out
> which plugin it talks to at the moment.

Um... that needs to be fixed, otherwise how can you use things not in 
the generic interfaces? (I guess it is enough to ask if a particular 
interface is implemented, where "particular interface" can be an 
interface defined by a plugin, i.e. IPerforceInterface, 
ISubversionInterface, etc.?)

> Another option would be to completely remove the Revision argument from
> the interface and let the plugin always ask the user via a customized
> dialog for this. And actually I think this might be more sane than to
> try to find something common about revisions.

Actually, I'd rather not do that. :-) The only complaint I had about the 
current system is I don't think tag/branch/keyword should be in the 
basic interface.

> Then diff() would only take a source and destination to prefill that
> dialog, for example.

...but then diff() is impossible to script, not even with e.g. WORKING 
and PREV which should be supportable by all VCS's. (Note: by "supported" 
I mean a plugin can figure out what PREV means, not that the VCS itself 
has to have a concept of "PREV".)

That reminds me, I /really/ would like to see log() return a standard 
format, so we could have The One True Log Window, and add features like 
diffing two versions straight from said window. I think this is all 
going to be stuff talking about IBrowsingVersionControl, would it be OK 
to call that 'experimental/volatile' for now?

> Ok, well I'll comment on the diffs.
> 
>> +    // Make async, take a list again... the list is probably more efficient, esp.
>> +    // for server-side VCS's like perforce
> 
> That was async already,

D'oh, so it was... my bad. :-) I think that's left over from thinking 
the return was the status, and when I realized it was a task, my brain 
adjusted everything *but* status.

> the list will make the semantics a bit more
> complicated: If run on a dir with recursive == off, will it return a map
> with 1 entry for the dir or will it return a map with all dirs/files in
> that dir, or both?

What's the status of a directory, anyway? (Oh... right, in svn, that 
means something). I guess it would give you just the directory? Also I 
assume most UI's would never /want/ to recurse for performance reasons, 
i.e. don't ask until you need to show the files.

>> --- ibrowsingversioncontrol.h.orig	2007-05-01 12:33:54.281144234 -0500
>> +++ ibrowsingversioncontrol.h	2007-05-01 12:33:26.000000000 -0500
> 
> Hmm, yeah splitting that makes sense.

Definitely. :-) Perforce can support IBrowsingVersionControl but *not* 
IRemoveVersionControl.

>> @@ -39,17 +39,19 @@
>> -        Number,
>> +        GlobalNumber    /**<Global repository version when file was last changed./*/,
>> +        FileNumber      /**<File version number, may be the same as GlobalNumber.*/,
> 
> I added these two for now

Ok.

>> +        // Remove Keyword, Branch, Tag... not portable
>> +        // Remove PluginSpecific, plug-ins will define a superset of this list
>> +        // if type is none of the above, that means plugin specific
> 
> As I said above currently users don't know which plugin and thus which
> values in this enum are available.

Yeah, I'm thinking we should do like with VCSMapping and have a 
supportedFlags() method somewhere (hmm, need to add that, or did I miss 
it?). Then I guess we add everything to RevisionType with the caveat 
that you have to check if it's supported. New plugins add new values 
(not a BC issue, right?) as needed; if you want to use that plugin, you 
have to build against the interfaces that have its stuff. Then we 
clearly document what support is required and what is optional.

Maybe that's not the best way, but it's all I'm coming up with right 
now; better suggestions welcome :-).

That said, if we go this way, I'm thinking GlobalNumer and FileNumber 
should be required support with the well-documented caveat that asking 
for one might give you the other (the returned version object will 
always have the appropriate type set). So in case it matters where one 
is supported and the other isn't, there is a way to find out.

-- 
Matthew
"Will somebody get this walking carpet out of my way?!"
   -- Princess Leia Organa