VCS Interfaces, round 3

[Matt Rogers]

On May 3, 2007, at 4:23 PM, Matthew Woehlke wrote:

> Here are the latest incarnations of the VCS interfaces, now living in
> trunk/KDE/kdevelop/lib/plugins/vcs/interfaces
>
> Let's stick to Andreas' original discussion guidelines:
>
> Andreas Pakulat wrote:
>> I'd like to keep the discussion to a minimum, so here are the points
>> that are allowed to be discussed ;)
>> * major problems for a given system with the proposed interface
>> * 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
>
> This is mostly covered; we are using "VcsJob". We know  
> approximately, but not exactly, what this will be. At the very  
> least it will support a wait() method, and will be the container  
> for completed() and failed() signals. Thus wait() and said signals  
> have been removed from IBasicVC.
>
>> * Parameters for push/pull in the distributed vcs iface, no idea  
>> whats
>>   needed there
>
> This still needs to be addressed. The good news is that there are  
> no BC issues until a plugin is actually implementing it, so this  
> isn't urgent. (Similar to how we expect individual plugins will  
> create their own interfaces in addition to the shared ones, e.g.  
> IAegisVersionControl.)
>

This assumption that there are no BC issues until a plugin is  
implementing it is incorrect. Being BC means that the interface  
_does_ _not_ change after releasing, even if there is nobody using  
it. BC is a contract between the interface writers and the users of  
that interface. Once we break that, we lose all trust, frustrate  
other developers, etc.

>> * diff and log parameters, am not 100% sure there
>
> ...are much more refined now, but still open for comment.
>
>> * 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
>
> Some of these might be wrong, if so, please point them out or fix  
> them.
>
> Additional Questions/Notes:
>
> - Right now we have log()/showLog() in both IBasicVC (takes local  
> paths) and IBrowsableVC (takes repo paths). Everyone OK with this?  
> Note that the only difference between the two is the parameter  
> type, which means (depending on how cleanly you've written your  
> code, I guess :-)) casting may be needed to ensure the correct one  
> is called.

I missed a few things, so I have some questions.

Why do we have log, and showLog?
Why do we have one interface that takes local paths and one that  
takes repo paths?

>
> - Are there VCS's that support log() but *not* ls() and/or cat()?  
> If so we might need to do another interface split.
>
> - Is it OK for diff() to ignore DiffUnified and return DiffRaw  
> anyway in the case of binary files? We need to either do this,  
> specify that diff() will ALWAYS return DiffRaw (but this is  
> probably less efficient for VCS's that will retrieve a diff and  
> need to then generate the second item by patching the first), or  
> define a standard binary diff format.
>

this seems more simple. If a file is a binary file, don't do a diff  
on it at all.

> - Better names for IAtomicBrowsableVC, VcsEvent, VcsItemEvent,  
> VcsChange and change() will be happily entertained; I'm ok with  
> those names but am not convinced they are ideal.
>
> - Is VcsAction missing anything? Note that it is expected that many  
> plugins will only support a subset of these.
>

--
Matt