VCS Interface classes

[Matthew Woehlke]

Andreas Pakulat wrote:
> On 02.05.07 15:45:02, Matthew Woehlke wrote:
>> Andreas Pakulat wrote:
>>> On 01.05.07 19:02:17, Matthew Woehlke wrote:
>> Ok. Do you have revisions in progress or should I draft something real 
>> quick covering what we've talked about so far?
> 
> Please do, I don't want to work on this tonight (rather get some other
> things done).

Heh, well at this point I won't be able to work on it for about 14 
hours, got too late. :-) (I've been distracted hacking on the broken 
cmake svn detection in kdesdk ;-), trying to get the dang thing to 
build.) But I'll do something in the (my) morning.

>>>> 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?
> 
> IIRC Kuba said with aegis he can use whatever diff tool he likes to use
> and unified diff is certainly just one text-diff-format. Of course we
> can settle for that - its pretty wide-spread in unix world. The thing
> I'd like to emphasize here is that we don't try to support more than one
> format - if we allow to extract a textual diff.

Ok, I can live with that. I would say if aegis wants to get fancy here, 
let it, that's what per-plugin stuff is for. Otherwise I prefer being 
able to write a generic showDiff that lets *all* VCS's take advantage of 
being able to diff binary files in a by-mimetype manner.

>> (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.)
> 
> Hmm, see above. I'd rather see us only produce 1 format of diff (if we
> do).

I'm fine limiting it to that. I was mainly thinking about converting 
A+unified to/from A+B.

> We may allow to import from other formats if there are tools to do
> this. You know all this stuff needs manpower and currently we're lacking
> it badly. (Neither Alex, nor Matt or Jens have the time at the moment).

I certainly hope to find time once this is more mature (read: after all 
the branches/3.5 selection fixes for KATE get ported to /trunk, at the 
least :-)). Although I expect some things (this, in particular) to come 
in on an as-needed basis.

>>>> 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),
> 
> Maybe I'm too biased by svn and cvs but they don't need any cat to
> provide a diff, all they need is a src and dst.  Hmm, I think now I get
> what you're aiming for, to produce A+something you need A and A can be
> in the repository. 

Exactly. :-)

> However if A is in the repository, i.e. diff() allows
> to use repository paths, then the plugin should implement
> IRepositoryVersionControl too. So you'd get cat().

(You mean IBrowsingVersionControl, right? perforce can cat(), ls(), etc, 
but can't do pure-repo-side manipulations which is what IRepoVC provides.)

And... you can cat() anything you can diff(), which was why I think the 
two should be in the same interface. I guess it really doesn't matter 
(you wind up implementing cat() anyway internally, no biggie), but it'll 
be odd if we wind up with some plugin that can diff pure-repo paths that 
doesn't implement IBrowsing. :-)

>> 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.
> 
> No you misunderstand. I'm not against having stuff in IBasic that needs
> a connection, I'm against stuff in IBasic that takes a repository path,
> because currently the only exception is diff which is a very basic and
> often used vcs command. I don't think cat() is used that often and its
> certainly something that will never be run on a local file.

Understood, I'm just observing that diff() == cat().

>>>> (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.
> 
> Sorry, I'm not that maniacal about code reuse. Especially not if the
> gain is so little.

Um... it has all the same benefits of libraries. Write it once. Get 
updates for free. Consistency, easier translation. Of course, we've had 
this argument before. :-) I think we should sideline it for now and come 
back when we're actually writing plugins. (And anyway we can always 
merge down code later.)

> Last but not least:
> How can the generic implementation work if it can't subclass from
> QObject [snip]

#define MY_CLASS_NAME SvnPlugin
#include "generic-showdiff-impl.cpp"
:-)

> Ugly as hell - IMHO. Let the plugins implement diff the way they want
> it, you simply can't get everything under one hood.

See above. But, just to be clear, I'm talking about /showDiff()/, not 
diff().

>>>> 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?
> 
> In a GUI maybe, not via scripts, too much a border case - IMHO.

Maybe. I'm not arguing for it either way, I'd be fine making 'follow' 
the default and non-optional. Although I do wonder if log() should 
return the repo path at each revision, otherwise can you ask about an 
old version with a different name? (I know svn doesn't give file name by 
default, and frankly, it drives me nuts, because you don't know what's 
happening. IMO log() should tell you what repo path each entry is 
talking about. :-))

>>>> 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.
> 
> Maybe I misunderstood but IIRC you wanted to get the same information as
> log has just for a specific revision right?

Yes and no. You get the user, date, etc. You /also/ get a list of what 
files the change affected, and preferably in what manner (i.e. 
add/del/edit). The whole point is to find out 'what did XX revision 
change?'.

>>> and should go into our IRepositoryVersionControl.
>> ...but do repos with non-global versioning support it? [snip]
> 
> I think I really misunderstood something,  this doesn't make the
> slightest sense. Of course with cvs you can get the log message of a
> specific revision.
> 
> Or do you aim at getting the list of files modified in that revision? In
> that case thats not possible with CVS I think. I don't have a cvs
> project at hand to try what log returns.

Yes, that. :-) IMO this is a really important thing to be able to do 
(for VCS's that support it). Hmm... /does/ svn support it? I can't 
imagine not, but I can't figure out offhand how to do it, either (diff 
the whole repo, maybe?).

>> (Also, does this mean there is no way to ask about
>> /just/ a directory? Is that desirable?)
> 
> Not sure what you mean but I do get status information on just a
> directory inside a working copy:

I meant how would you do this with the interface. Behave differently 
based on the presence/absence of a trailing '/'? I guess that would 
works. :-)

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