VCS Interface classes

[Matthew Woehlke]

Andreas Pakulat wrote:
> On 01.05.07 19:02:17, Matthew Woehlke wrote:
>> Andreas Pakulat wrote:
>>> On 01.05.07 15:43:37, Matthew Woehlke wrote:
>>>> Andreas Pakulat wrote:
>>>>> On 01.05.07 12:40:09, Matthew Woehlke wrote:
>>>> 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?
>>> But I also think it might make sense to have some this feature. So how
>>> about this:
>>>
>>> fetchLog( localOrRepoPath, revision, limit)
>>> logWindow( localOrRepoPath )
>> I've been thinking about that, too; certain methods I think it makes 
>> sense to have a UI and non-UI version.
>>
>> I'm not really crazy about the names (but then, they're just names :-)), 
>> what do you think about log() and showLog()?
>  
> Sounds a bit better, although we'd then have to apply this everywhere.

True, but...

> BTW: It just occured to me that our commit() doesn't have a QString
> argument for the commit message ;)

...crud. :-) And don't we /want/ commit() to be scriptable? So we need 
commit() and showCommit().

>> I'm still not convinced about diff()... would you be OK making that one 
>> diff() and showDiff()?
> 
> Sure.

Ok. Do you have revisions in progress or should I draft something real 
quick covering what we've talked about so far?

>> For VCS's that don't support binary diff, you could provide such a
>> thing in the plugin if it supports IBVC::cat, which would allow fancy
>> diffs independent of the VCS. Otherwise just document it as allowed to
>> fail (what would it do normally on binary files if the VCS doesn't
>> support binary diff?).
> 
> svn for example tells you that it doesn't diff binary files. But I'm
> following you here, if there's a IBVC::cat implemented the plugin may
> have this feature by doing it for certain mimetypes itself.

Right, just because svn doesn't do it doesn't mean you can't (with 
cat()) do it locally.

>> Um... and we're going about this the wrong way, I think. How many 
>> instances need /just/ the diff, anyway? Scripted, *maybe*, but I would 
>> think showDiff() is really going to pop up the two versions in e.g. 
>> Kompare, no?
> 
> Yeap, although I hope its rather our own diff-lib (which should be
> derived from kdiff3 to have 3-way diff for merging). We already have
> libdiff2 from kompare in the teamplugin atm.

Right. :-)

>> IOW, diff() isn't returning "a diff", what it /really/ 
>> needs is the complete copy of each item.
> 
> Yeap, I think diff returning a real diff is rather hard, unless we
> settle on unified diff format - for text files - which may not play well
> with some users/vcs systems.

It's the users I'd be worried about, we /do/ want to work out The Right 
Way here.

Why does it matter to the VCS? Am I missing something?

>> Now, it is more efficient to 
>> fetch /one/ copy and the patch that generates the other, but the end 
>> result is that you have a copy of both items. IOW format standardization 
>> is a non-issue because we can always generate both items, and from there 
>> translate that into any standard we want. (We might want to pass to 
>> diff() if we need both items, or item A and a diff, or if we don't care, 
>> so that we don't translate the hard way from one when we actually want 
>> the other. That would allow us to ask for 'pure' as well if we really 
>> /don't/ care what we started with, just the changes.)
> 
> Not sure I completely follow you here, but if I understood correctly
> you'd like to have an option on diff (i.e. a flag) to say:
> 
> return versionA + versionB - completely
> return versionA + diff to versionB
> return whatever is easiest for the plugin to compute (i.e. don't care).

Exactly. (I'm thinking we will also have a diff-support library that can 
wrangle various types of diff to get the kind of output you need, or to 
convert between A+B and A+diff... I don't see us *not* needing that at 
some point and at some level.)

>> Although I guess this indirectly means that cat() should move to IBasic? 
> 
> No, cat is a function that takes a remote location and IMHO we should
> have that in a separate interface. I only left the QVariant in diff() in
> the basic interface because thats something people will often want to
> do, diff a local file against some remote version. But if a plugin
> cannot do such things (i.e. vcs doesn't support directly working on the
> repository) it will only provide diffing against whatever is in the
> working copy (for example show changes between last update and now - svn
> can do this without querying the repository).

Hmm... ok, so IOW diff() in IBasic is already "shaky". But there were 
two reasons I suggested cat() in IBasic (although they go together). 
Basically, if we make diff() return A+something, i.e. you probably need 
cat(A) in order to produce diff(A, foo), and also you can get cat(A) 
/from/ diff(A, foo). IOW if you have one, you have the other.

That said, svn being able to diff w/o a connection (btw, perforce at 
least is useless without a connection, you don't even know you have a 
working copy much less can you do any VCS operations) is a good point, 
but this is a rather limited use-case of diff. If you don't want a 
function that needs a connection in IBasic, then diff() can't be in 
IBasic because it only works w/o a connection for svn, and even then 
only in one particular instance.

>> (And... if cat() moves to IBasic, we can have a generic diff() 
>> implementation... Yes, in case you hadn't noticed, I'm maniacal about 
>> code reuse. :-))
> 
> I'm pretty much against this for two reasons:
> 
> a) we have a pure interface, so each plugin would have a copy of the
> code - unless I misunderstood something

I agree that the interface itself should be pure. What I was thinking is 
that we could provide a generic (and incomplete) implementation 
/additionally/ that plugins could choose to use or not use if it makes 
their lives easier.

> b) a plugin may have really optimized methods to get various types of
> diffs, for example svn knows the original content of a file and thus
> doesn't need to contact the repository to tell you what you changed
> between the last update and now. This is important, IMHO, we shouldn't
> limit the freedom of the plugin just to share some more lines of code.

I don't understand how this would be prevented? If you don't want the 
generic implementation, just overload it.

>> It also occurs to me, I guess this would follow branches by default (if 
>> supported by the VCS)? Or should that be an option?
> 
> What do you mean follow branches?

The opposite of 'svn log --stop-on-copy' :-). (Sorry, perforce 
terminology slipping in again.) In perforce you have to specifically 
request this, although I think (like svn) it should be default. I was 
just wondering if you think there should be an option to disable this?

>> Ok, addition: IFooVersionControl (AtomicBrowsing?):
>> revision(Revision) // gets information about a specified revision.
> 
> Thats rather logMessage(Revision) IMHO

...logMessage() sounds to me like a subset, not a superset, of log() 
(referring to what information you get back) :-). Not that I'm really 
crazy about revision() either, to be honest.

> and should go into our IRepositoryVersionControl.

...but do repos with non-global versioning support it? (I'm thinking 
about cvs, I thought cvs didn't have such a critter but my cvs knowledge 
is almost nil :-).) That's why I suggested a separate interface. (Or I 
guess it can just fail on such a VCS. After all any revision type other 
than GlobalNumber is probably unsupported as well. Incidentally this 
would be one case for knowing that you asked for a GlobalNumber and 
actually got a FileNumber, this would indicate that you are talking to a 
VCS where you can't use revision()/logMessage() :-).)

>>>>> 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.
>>> Yeap, I think recursive should be false on many of our actions by
>>> default. stat on an svn-versioned directory without recursive will give
>>> you information about all files in that dir that have changed, are
>>> unversioned, are in conflict and so on. Things like "needs update" needs
>>> a communication with the repository anyway. So I guess the answer to my
>>> question is: status() on a dir without recursive is giving you the list
>>> of entries in that dir + their status information. The only thing left:
>>> Do we also want the status information on the dir itself?
>> Um, actually I was assuming status told you about exactly what you asked 
>> about, i.e. if you want to know about files 1 level deep, you give in 
>> the list files 1 level deep. Otherwise it gives you one result; the 
>> directory. :-)
> 
> Well, for the working copy root dir you'd get the list of files with
> their respective version as result:
> andreas at morpheus:~/KDE-work/4.0/kdevelop>svn stat -u -N .
> ?                   KDevelop4.kdevelop.filelist
> ?                   .kdev4
> ?                   svn-commit.2.tmp
> ?                   svn-commit.3.tmp
> ?                   KDevelop4.kdevses
> ?                   KDevelop4.kdevelop
> ?                   KDevelop4.tag
> M          660342   kdevelop.kdev4
> ?                   KDevelop4.kdevelop.pcs
> ?                   svn-commit.tmp
> Status bezogen auf Revision: 660464
> 
> For another subdir you'd get:
> 
> andreas at morpheus:~/KDE-work/4.0/kdevelop>svn stat -u -N lib
> Status bezogen auf Revision: 660465

I'd be ok with that, although for at least perforce it means the plugin 
will have to list the directory itself. That's why I was leaning towards 
having the UI do it. (Also, does this mean there is no way to ask about 
/just/ a directory? Is that desirable?)

>>> Actually I'm fine with supporting just the common stuff, i.e.
>>> number+date, in the basic interface and let plugin's provide extensions
>>> to that. I'm not sure however how the Qt flags stuff works and what it
>>> needs to extend it, i.e. if we can keep the enum in the class or make it
>>> part of the Kdevelop namespace.
>> Ok, I'm fine with that too (as long as it works). :-)
> 
> The only "problem" here is that number may include some arbitrary
> characters, like dots. Maybe it should be more something like
> FormattedNumber or .... OTOH we can just document that this needs to be
> treated as a string and never tried to convert into a real number - in
> the general case.

I assumed we were doing that anyway? :-) (Treating it as QString only, 
that is.)

-- 
Matthew
"Doggy!" -- Robots from Freefall (http://freefall.purrsia.com)