VcsRevision

Hi,

currently VcsRevision can have at least 5 types: date,
filenumber,
globalnumber, special or range. 

In the thread that started about merging iatomic there was
mentioning
about having repo path (and possibly also local file paths)
inside
VcsRevision. 

One question is wether a VcsRevision can be constructed by a
user of our
API's. If so there would be the need to set such paths, if
not the path
can be something that is completely internal to the revision
- or at
least only accessible to the plugins.

So, how are VcsRevisions constructed? Is always an interface
method
used? Can somebody see a use-case that needs constructing
VcsRevision
directly?

Andreas

-- 
You will be advanced socially, without any special effort on
your part.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

Andreas Pakulat wrote:
> currently VcsRevision can have at least 5 types: date,
filenumber,
> globalnumber, special or range. 
> 
> In the thread that started about merging iatomic there
was mentioning
> about having repo path (and possibly also local file
paths) inside
> VcsRevision. 
> 
> One question is wether a VcsRevision can be constructed
by a user of our
> API's. If so there would be the need to set such paths,
if not the path
> can be something that is completely internal to the
revision - or at
> least only accessible to the plugins.
> 
> So, how are VcsRevisions constructed? Is always an
interface method
> used? Can somebody see a use-case that needs
constructing VcsRevision
> directly?

Wow, you actually did an excellent job covering what I was
expecting to 
say in response already , so I'll
just throw out my own thoughts if 
anyone wants to disagree.

I assume one can create a date or special. Maybe a
GlobalNumber. 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. (An
easy answer is 
to construct it via '<repoPath>#<number>' passed
as a QString.) 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.

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

Actually, what is the use-case for a range, is it just
merge()? (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? )

-- 
Matthew
Excessive obscurity: -5


_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 21.05.07 09:26:24, Matthew Woehlke wrote:
> Andreas Pakulat wrote:
> > currently VcsRevision can have at least 5 types:
date, filenumber,
> > globalnumber, special or range. 
> > 
> > In the thread that started about merging iatomic
there was mentioning
> > about having repo path (and possibly also local
file paths) inside
> > VcsRevision. 
> > 
> > One question is wether a VcsRevision can be
constructed by a user of our
> > API's. If so there would be the need to set such
paths, if not the path
> > can be something that is completely internal to
the revision - or at
> > least only accessible to the plugins.
> > 
> > So, how are VcsRevisions constructed? Is always an
interface method
> > used? Can somebody see a use-case that needs
constructing VcsRevision
> > directly?
> 
> Wow, you actually did an excellent job covering what I
was expecting to 
> say in response already , so I'll
just throw out my own thoughts if 
> anyone wants to disagree.
> 
> 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?

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

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

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

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

Andreas

-- 
Someone is speaking well of you.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

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.

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

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

Andreas

-- 
Bank error in your favor.  Collect $200.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

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.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 24.05.07 10:47:33, Matthew Woehlke wrote:
> Andreas Pakulat wrote:
> > On 21.05.07 11:39:38, Matthew Woehlke wrote:
> >> 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.
> 
> My concern is just that if the VCS doesn't support
'from,to' in log(), 
> you are stuck waiting on it to fetch 'everything'.
Hmm... on second 
> thought, any objection to requiring it to be kind-of
like showLog()? Use 
> a reasonable limit internally if the VCS does not
support 'from,to' and 
> keep making limited calls until the 'to' revision is
reached, then truncate.

I don't care at the moment how the plugin achieves
"from,to", thats an
implementation detail. If that is slow because it fetches
the whole
history of a file, thats unfortunate but unavoidable. Also I
think
you're too concerned here, unless you've got a really crappy
VCS system
getting the full log of a certain file or dir shouldn't be
that bad. As
an example:


And I consider that a really huge repository log.

> >> 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).
> 
> I was assuming you always provided all parameters, even
if that means 
> passing the same arg twice. I guess we could also
document that empty 
> dstPath means 'use srcPath'.

Hmm, what about an overload that takes only 1 path but two
revisions?
Thats better than documenting a special case or having to
provide 2
paths that are the same, which is a special case that the
user needs to
know about...

Andreas

-- 
You will pay for your sins.  If you have already paid,
please disregard
this message.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 24.05.07 12:49:09, Matthew Woehlke wrote:
> Andreas Pakulat wrote:
> > On 24.05.07 10:47:33, Matthew Woehlke wrote:
> > Also I think
> > you're too concerned here, unless you've got a
really crappy VCS system
> > getting the full log of a certain file or dir
shouldn't be that bad. As
> > an example:
> > 
> > 
> > And I consider that a really huge repository log.
> 
> Your example seems to be missing . Anyway I
suppose I'm concerned 
> because someone previously made a case for the numeric
limit, so I'm 
> just following that.

Yes, it took longer and finally I forgot to wait for it
before sending
the mail, so here it is:
andreasmorpheus:~>time svn log https:
//svn.kde.org/home/kde/trunk/KDE/kdelibs >/dev/null

real    33m50.500s
user    0m4.342s
sys     0m0.474s

Which is actually longer than I thought, but as I said
before, 668025
revisions is a really huge number - IMHO.

Andreas

-- 
You will be run over by a bus.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel