[VCS] VcsRevision

[Andreas Pakulat]

On 21.05.07 11:39:38, Matthew Woehlke wrote:
> Andreas Pakulat wrote:
> > On 21.05.07 10:52:24, Matthew Woehlke wrote:
> >> Andreas Pakulat wrote:
> >>> On 21.05.07 09:26:24, Matthew Woehlke wrote:
> >>>> 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?
> > 
> > Yeap.
> 
> Done. I think we can also now remove RST::Valid and isValid()?

Yes I think so.

> >>>> 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()?
> > 
> > In log it would allow you to fetch exactly all changes between the two
> > revisions. This could - in theory - also be achieved by using the limit,
> > but then you'd have to know how many changes where between the
> > revisions... But as we agree on having overloads rather than ambigous
> > classes we will do that.
> 
> Ok, it's just that I suspect such a limit to be at least as "optional" 
> as the integer limit (perforce AFAICT does not support it)... but I 
> guess that's ok. Change checked in for log(), please check it out and 
> tell me if it's ok (<insert usual comment about how @param limit is 
> "just a name" :-)>). Right now showLog() uses an implicit limit, ok to 
> keep using that I guess?

I think keeping the implicit limit is fine.

> Maybe we should instead take both limits at once, with default 
> parameters for each (make the revision limit a pointer I guess, default 
> = NULL)?

No, also the VcsRevision is not a limit, its more like "from" and "to".
And having them in 2 separate methods makes it more clear that they mean
different things. One is for fetching the last N items in the history
starting at revision, the other one fetches all history items between
two revisions.
 
> >>> 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. :-)
> > 
> > Yes, everything I removed I agree with you.
> 
> log() should be ok now (see above). Is diff() ok as-is, or am I missing 
> a use case?

No, I think diff is ok too. We might need to document that dstPath
doesn't need to be given, if 2 revisions are given. Or do we force users
to provide the same path for both (path == repo or local path).

Andreas

-- 
Ships are safe in harbor, but they were never meant to stay there.