[VCS] VcsRevision

[Matthew Woehlke]

Andreas Pakulat wrote:
> On 21.05.07 09:26:24, Matthew Woehlke wrote:
>> I assume one can create a date or special
> 
> Hmm, yeah.
> 
>> Maybe a GlobalNumber.
> 
> That is probably not a problem either.
> 
>> I was assuming that one can't create a FileNumber but would always get
>> it from the plugin, but I guess that does seem inconvenient.
> 
> Use case please?

For creating one? Well, mostly I guess it would shortcut getting 'the 
version five revisions ago', but log() should be able to get you this. 
Since it's (obviously) only useful in VCS's that support FileNumber's, 
I'm also not convinced it is necessary.

>> Hmm, 
>> setRevisionValue probably should return a bool. I think the problem with 
>> building a number (either kind) is you have to know if "number" means 
>> 655326 as in svn/perforce or "3.5.6.107" as in (IIRC?) CVS.
> 
> Hmm, maybe we should disallow creating revisions from anything but a
> date or special. Seriously, I don't see the use-case in the public
> interface (no doubt you'll create them inside the gui sometimes).

...or at least state that no more support than that is required, and 
anything you get beyond that is potluck :-). Actually specials should 
only need the global statics, no need to create them, use e.g. 
VcsRevision::Head. We should probably make it a policy to try to parse 
user-typed strings (again, thinking about a GUI VCS browser that might 
use mainly its own widgets and not as much showXYZ methods), but I think 
you're right that user-typed strings is the main use-case for types 
other than date that would need to be constructed outside the plugin.

>> Creating a range is IMO not relevant, we already have setSourceRevision 
>> and setTargetRevision, so the answer is 'yes' as long as you have 
>> VcsRevisions to feed those. (Hmm, what happens though if you try to set 
>> one of those to a range? Should it return false, silently use an invalid 
>> revision instead, something else...?)
> 
> If you try to create a range of ranges you should get an invalid
> VcsRevisions and a false return value on the setSource and setTarget
> functions.

Later conversation aside, are we agreeing to s/void/bool/ on these returns?

>> Actually, what is the use-case for a range, is it just merge()?
> 
> No, its for diff() or log() (and maybe others I don't recall atm).

...but diff() already takes two revisions? What does it do in log()?

>> (Also, doesn't the target of a merge have to be a local path? And then
>> what does dstRevision mean? Did we have this conversation before and I
>> am forgetting? :-))
> 
> merge() takes two plain revisions, src and target, it won't work with
> either of the one being a range.

Er... right. I forgot merge() takes arbitrary files to form the patch, 
i.e. takes /three/ paths not two. Guess I should look at the signature 
first. :-)

I don't think perforce supports this natively (I see svn does, however), 
but I would prefer keeping the more flexible signature... so this is 
just a note that we may need a diff+patch library to implement it in 
perforce for some cases (i.e. srcPath != dstPath).

> Maybe we should rather have overloads
> for diff/log/<whatever can use a range of revisions> with 2 revision
> arguments instead of a range inside VcsRevision. That way we don't have
> to specially document when a range-revision can be used and when not.

Yes, that's exactly what I was leaning towards. It also seems to 
simplify things a lot not having a class that might have data or two 
other pointers to other instances of itself. :-)

-- 
Matthew
When in doubt, duct tape!