VCS Interface classes

[Andreas Pakulat]

On 02.05.07 15:45:02, Matthew Woehlke wrote:
> Andreas Pakulat wrote:
> > On 01.05.07 19:02:17, Matthew Woehlke wrote:
> >> Andreas Pakulat wrote:
> >> 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?

Please do, I don't want to work on this tonight (rather get some other
things done).

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

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

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

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

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

> >> (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. And I can imagine its somewhat confusing: "Why does
this plugin not implement diff()? Aaaah, right its in its parent class
already". Also the plugin will declare to implement interfaces which are
not directly visible in the class definition (as the generic
implementation already is an interface subclass). Last but not least:
How can the generic implementation work if it can't subclass from
QObject - and it can't because the plugin needs to subclass from IPlugin
which is subclassing QObject - it needs to connect its job to its
finished-job to emit the return value.

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

Hmm, but then we'd have code that does something like this:

if rev1==working and rev2==head
   do_optimized_diff
else
   callParentClassdiff()

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

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

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

> > 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() :-).)

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.

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

Not sure what you mean but I do get status information on just a
directory inside a working copy:
andreas at morpheus:~/KDE-work/4.0/kdevelop>svn stat -N templates/
I      templates

Its just that svn doesn't give information on up-to-date dirs.

Andreas

-- 
Are you making all this up as you go along?