You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2005/10/10 12:58:56 UTC
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Sorry for the long delay in replying. I looked at this a couple of times but
still couldn't understand what is going on here.
Could anyone else take a look at this?
- Julian
S.Ramaswamy wrote:
> On 9/3/05, Julian Foad <ju...@btopenworld.com> wrote:
>
>>I'm not sure what it means to create a connection to a particular revision of
>>an object. In terms of generating the log, is it necessary to connect to a
>>particular revision, or will any revision work?
>>
>>If you connect to some particular revision such as the "start" or "end"
>>revision of the log, will that make the log operation more efficient than if
>>you had connected to "head" or a random revision? That's my guess of the day.
>>
>>
>>>I meant to get a ra_session to the youngest revision of the object, before
>>>using the operating revisions on that.
>>
>>Huh? The youngest revision of an object is always given by
>>svn_opt_revision_head. I suppose you meant the younger of the two specified
>>endpoints for the log, "start" and "end". Yes? But your code only does that
>>in certain cases (namely when both are specified as a number). Therefore
>>either your code is incomplete, or it is not necessary to connect to the
>>younger of the two revisions. If it is not necessary, then why do you do it in
>>some cases? Because it makes the log operation more efficient? If that's the
>>case, that's fine, but it's not obvious so a comment in the code would be
>>useful. If that's not the case then what is the reason for it?
>
>
> While it is generally true that passing start or end as the revision
> to svn_client__ra_session_from_path() doesn't change the result, there
> is one case in which it matters - if the revision range extends over
> renames, and both the revisions are numbers or dates, then the
> revision number/date passed to svn_client__ra_session_from_path()
> seems to matter.
>
> For example, if trunk/folder existed between r4 and r6 and was renamed
> to trunk/folder1 in r7, then, trying to get the log with 'svn log
> -r4:7 trunk/folder1' doesn't work if you are passing the start
> revision(4) to svn_client__ra_session_from_path(). But passing the end
> revision (7) to svn_client__ra_session_from_path() works. Here's the
> result with the actual revision # of the object(rev) and the final
> resulting url(url_p) from svn_client__ra_session_from_path() printed
> out - In this case the revision was set to start.
> -------------------------
> svn log -r4:7 trunk/folder1
> url_p:file:///home/ramaswamy/issues/2287/repos/trunk/folder
> rev:4
> subversion/libsvn_fs_fs/tree.c:315: (apr_err=160013)
> svn: File not found: revision 7, path '/trunk/folder'
> -------------------------
> Same case with dates. Setting the revision uniformly to
> svn_opt_revision_unspecified also does not work, with cases like "svn
> log -r4:6 trunk/folder1".
>
> Revised patch(v3) attached.
>
> Ramaswamy
>
> [[[
> Fix issue #2287 - add peg revision to svn_client_log2() and add
> peg revision support to the command line client.
>
> * subversion/include/svn_client.h:
> (svn_client_log3): New prototype.
> (svn_client_log2): Deprecate.
>
> * subversion/libsvn_client/log.c:
> (svn_client_log3): New function.
> (svn_client_log2): Re-implemented using new function
> svn_client_log3().
> (svn_client_log): Re-implemented using new function
> svn_client_log3().
>
> * subversion/clients/cmdline/log-cmd.c:
> (svn_cl__log): Call svn_client_log3().
>
> * subversion/tests/clients/cmdline/log_tests.py:
> (url_missing_in_head, log_through_copyfrom_history): Use peg
> revisions.
> ]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> takes a targets array instead, which means that the behavior of the
> peg revision needs w.r.t. multiple targets needs to be worked out.
> There are two ways of calling log, IIRC:
>
> $ svn log LOCAL_PATH1 [LOCAL_PATH2 ...]
Not exactly. Only one local path is allowed in the "svn" client.
$ svn log LOCAL_PATH
Why? Because svn_client_log2() doesn't work properly with more than one, and
it was difficult to fix so I didn't fix it. In those days I wasn't considering
the possibility of other clients using the API. Oops. :-(
When I say "doesn't work properly", I'm sure that was the case and don't
believe it has been fixed. I can't state the exact misbehaviour off the top of
my head, but I recall it being bad enough that I wouldn't want anyone to try to
use it, and that's why I disabled it (in the client).
> and
>
> $ svn log BASE_URL [SUB_PATH1 ...]
>
> In the first case, I think we already default to using the revision of
> the first path when no -r is specified. But what to do if multiple
> peg revs are passed? What about if some of the paths have peg revs
> and others don't?
>
> In the second case, I guess we can get away with just allowing a peg
> rev on BASE_URL and not on any of the sub-paths.
Yes, that sounds like the only sane behaviour for the URL case.
> Thoughts?
Uh... For our own client, a problem doesn't arise :-)
Seriously, I think the best medium-term solution is to disallow multiple local
targets in svn_client_log3(). They don't work properly in _log2() anyway so it
shouldn't cause many clients any hardship.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Posted by "S.Ramaswamy" <sr...@gmail.com>.
>
> Ramaswamy, I haven't had time to review your patch in detail (sorry!
> just have to give priority to 1.3.x review and voting right now), but
> I did notice one thing. You make the straightforward change of adding
> a 'peg_revision' parameter, thus creating svn_client_log3(), which
> is great. But the new doc string material says:
>
> + * given log message more than once). @a peg_revision indicates in which
> + * revision @a path_or_url is valid. If @a peg_revision is @c
> + * svn_opt_revision_unspecified, then it defaults to \
> @c svn_opt_revision_head
> + * for URLs or @c svn_opt_revision_working for WC targets.
>
> I think you must have cut-and-pasted from svn_client_blame2(),
> perhaps? There is no path_or_url parameter in svn_client_log3(). It
> takes a targets array instead, which means that the behavior of the
> peg revision needs w.r.t. multiple targets needs to be worked out.
oops, how about, "@peg revision indicates in which revision targets are
valid ..." ?
The patch does not attempt to change the current behaviour of 'svn log',
which does not allow multiple wc paths, or specify more than one url.
. Here is the section from clients/cmdline/log-cmd.c
------------------------------------------------------------
/* Verify that we pass at most one working copy path. */
if (! svn_path_is_url (target) )
{
if (targets->nelts > 1)
return svn_error_create (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
_("When specifying working copy paths, only "
"one target may be given"));
}
else
{
/* Check to make sure there are no other URLs. */
for (i = 1; i < targets->nelts; i++)
{
target = APR_ARRAY_IDX (targets, i, const char *);
if (svn_path_is_url (target))
return svn_error_create (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
_("Only relative paths can be specified "
"after a URL"));
}
------------------------------------------------------------
Thanks
Ramaswamy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Posted by "S.Ramaswamy" <sr...@gmail.com>.
>
> Ah, a red herring it may be, but that's the part I meant I "couldn't
> understand". That's where I got hung up on trying to review the patch. In my
> previous email or two in the thread I queried this, trying to discover why this
> particular part of the patch does what it does, and I couldn't understand
> Ramaswamy's answers.
>
In the mail that I sent with v3 of the patch, I was trying to explain
that (a)It doesn't matter whether you pass the start or the end
revision to svn_client__ra_session_from_path() (b)But there is a case
where it does seem to matter - When the revision range specified
includes a revision in which the file was renamed; Passing the lesser
revision and getting a ra_session from svn_ra_session_from_path()
makes svn_ra_get_log() fail(with that ra_session).
And the example that I gave in that mail was:
For example, if trunk/folder existed between r4 and r6 and was renamed
to trunk/folder1 in r7, then, trying to get the log with 'svn log
-r4:7 trunk/folder1' doesn't work if you are passing the start
revision(4) to svn_client__ra_session_from_path(). But passing the end
revision (7) to svn_client__ra_session_from_path() works. Here's the
result with the actual revision # of the object(rev) and the final
resulting url(url_p) from svn_client__ra_session_from_path() printed
out - In this case the revision was set to start.
-------------------------
svn log -r4:7 trunk/folder1
url_p:file:///home/ramaswamy/issues/2287/repos/trunk/folder
rev:4
subversion/libsvn_fs_fs/tree.c:315
<file:///home/ramaswamy/issues/2287/repos/trunk/folderrev:4subversion/libsvn_fs_fs/tree.c:315>:
(apr_err=160013)
svn: File not found: revision 7, path '/trunk/folder'
-------------------------
Thanks
Ramaswamy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>Sorry for the long delay in replying. I looked at this a couple of
>>times but still couldn't understand what is going on here.
>>
>>Could anyone else take a look at this?
Thanks for taking a look, Karl.
> I think it's a correctness problem.
[...]
> Subversion first goes to r100 [...] and *then* looks around for 'folder1',
> which of course doesn't exist -- and even if it did exist, it might be
> the wrong object.
Yes, absolutely, that's why this enhancement is needed. As usual, I didn't use
enough words to explain myself.
> The discussion below about whether to pass the start revision versus
> the end revision of the range to svn_client__ra_session_from_path() is
> a red herring, I think.
Ah, a red herring it may be, but that's the part I meant I "couldn't
understand". That's where I got hung up on trying to review the patch. In my
previous email or two in the thread I queried this, trying to discover why this
particular part of the patch does what it does, and I couldn't understand
Ramaswamy's answers.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> Sorry for the long delay in replying. I looked at this a couple of
> times but still couldn't understand what is going on here.
>
> Could anyone else take a look at this?
I think it's a correctness problem. The original issue says:
| The svn_client_log should take a peg revision. Then you can do
| something like this:
|
| svn log -r100:200 folder1
|
| even if folder1 is currently at revision 1000 and has been renamed
| in revision 700. Right now, this throws an error.
In other words, back in revisions 100:200, 'folder1' was named
something else -- call it 'oldfolder'. Even though 'folder1' in the
current working copy could have its history line identified by its URL
and a peg rev of r1000, svn_client_log() doesn't have any way of
taking that peg rev. So instead, Subversion first goes to r100 or
r200 (I don't know which), and *then* looks around for 'folder1',
which of course doesn't exist -- and even if it did exist, it might be
the wrong object. What we need to do is grab the object identity of
'folder1' at r1000, and trace it back so that in r100 - r200, we are
dealing with its previous incarnation as 'oldfolder'.
Note that we have svn_client_log2() these days, not svn_client_log(),
but svn_client_log2() doesn't take a peg revision either, so the
problem persists.
The discussion below about whether to pass the start revision versus
the end revision of the range to svn_client__ra_session_from_path() is
a red herring, I think. The real issue here is about being able to
pass a peg revision.
Ramaswamy, I haven't had time to review your patch in detail (sorry!
just have to give priority to 1.3.x review and voting right now), but
I did notice one thing. You make the straightforward change of adding
a 'peg_revision' parameter, thus creating svn_client_log3(), which
is great. But the new doc string material says:
+ * given log message more than once). @a peg_revision indicates in which
+ * revision @a path_or_url is valid. If @a peg_revision is @c
+ * svn_opt_revision_unspecified, then it defaults to \
@c svn_opt_revision_head
+ * for URLs or @c svn_opt_revision_working for WC targets.
I think you must have cut-and-pasted from svn_client_blame2(),
perhaps? There is no path_or_url parameter in svn_client_log3(). It
takes a targets array instead, which means that the behavior of the
peg revision needs w.r.t. multiple targets needs to be worked out.
There are two ways of calling log, IIRC:
$ svn log LOCAL_PATH1 [LOCAL_PATH2 ...]
and
$ svn log BASE_URL [SUB_PATH1 ...]
In the first case, I think we already default to using the revision of
the first path when no -r is specified. But what to do if multiple
peg revs are passed? What about if some of the paths have peg revs
and others don't?
In the second case, I guess we can get away with just allowing a peg
rev on BASE_URL and not on any of the sub-paths.
Thoughts?
-Karl
--
www.collab.net <> CollabNet | Distributed Development On Demand
> S.Ramaswamy wrote:
> > On 9/3/05, Julian Foad <ju...@btopenworld.com> wrote:
> >
> >>I'm not sure what it means to create a connection to a particular revision of
> >>an object. In terms of generating the log, is it necessary to connect to a
> >>particular revision, or will any revision work?
> >>
> >>If you connect to some particular revision such as the "start" or "end"
> >>revision of the log, will that make the log operation more efficient than if
> >>you had connected to "head" or a random revision? That's my guess of the day.
> >>
> >>
> >>>I meant to get a ra_session to the youngest revision of the object, before
> >>>using the operating revisions on that.
> >>
> >>Huh? The youngest revision of an object is always given by
> >>svn_opt_revision_head. I suppose you meant the younger of the two specified
> >>endpoints for the log, "start" and "end". Yes? But your code only does that
> >>in certain cases (namely when both are specified as a number). Therefore
> >>either your code is incomplete, or it is not necessary to connect to the
> >>younger of the two revisions. If it is not necessary, then why do you do it in
> >>some cases? Because it makes the log operation more efficient? If that's the
> >>case, that's fine, but it's not obvious so a comment in the code would be
> >>useful. If that's not the case then what is the reason for it?
> > While it is generally true that passing start or end as the revision
> > to svn_client__ra_session_from_path() doesn't change the result, there
> > is one case in which it matters - if the revision range extends over
> > renames, and both the revisions are numbers or dates, then the
> > revision number/date passed to svn_client__ra_session_from_path()
> > seems to matter.
> > For example, if trunk/folder existed between r4 and r6 and was
> > renamed
> > to trunk/folder1 in r7, then, trying to get the log with 'svn log
> > -r4:7 trunk/folder1' doesn't work if you are passing the start
> > revision(4) to svn_client__ra_session_from_path(). But passing the end
> > revision (7) to svn_client__ra_session_from_path() works. Here's the
> > result with the actual revision # of the object(rev) and the final
> > resulting url(url_p) from svn_client__ra_session_from_path() printed
> > out - In this case the revision was set to start.
> > -------------------------
> > svn log -r4:7 trunk/folder1
> > url_p:file:///home/ramaswamy/issues/2287/repos/trunk/folder
> > rev:4
> > subversion/libsvn_fs_fs/tree.c:315: (apr_err=160013)
> > svn: File not found: revision 7, path '/trunk/folder'
> > -------------------------
> > Same case with dates. Setting the revision uniformly to
> > svn_opt_revision_unspecified also does not work, with cases like "svn
> > log -r4:6 trunk/folder1".
> > Revised patch(v3) attached.
> > Ramaswamy
> > [[[
> > Fix issue #2287 - add peg revision to svn_client_log2() and add
> > peg revision support to the command line client.
> > * subversion/include/svn_client.h:
> > (svn_client_log3): New prototype.
> > (svn_client_log2): Deprecate.
> > * subversion/libsvn_client/log.c:
> > (svn_client_log3): New function.
> > (svn_client_log2): Re-implemented using new function
> > svn_client_log3().
> > (svn_client_log): Re-implemented using new function
> > svn_client_log3().
> > * subversion/clients/cmdline/log-cmd.c:
> > (svn_cl__log): Call svn_client_log3().
> > * subversion/tests/clients/cmdline/log_tests.py:
> > (url_missing_in_head, log_through_copyfrom_history): Use peg
> > revisions.
> > ]]]
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org