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/08/01 00:18:13 UTC
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
S.Ramaswamy wrote:
>
> Fix issue #2287 - add peg revision to svn_client_log2() and add
> peg revision support to the command line client.
Apologies for the delay in reviewing this.
>
> * 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.
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 15298)
> +++ subversion/include/svn_client.h (working copy)
> @@ -915,7 +915,8 @@
> /**
> * Invoke @a receiver with @a receiver_baton on each log message from @a
> * start to @a end in turn, inclusive (but never invoke @a receiver on a
> - * given log message more than once).
> + * given log message more than once). @a peg_revision indicates in which
> + * revision @a targets are valid.
This needs to specify what svn_opt_revision_unspecified means as a peg revision ...
> *
> * @a targets contains either a URL followed by zero or more relative
> * paths, or a list of working copy paths, as <tt>const char *</tt>,
[...]
> /**
> + * Similar to svn_client_log3(), but with the @a peg_revision
> + * parameter always set to @c svn_opt_revision_unspecified.
... if svn_client_log2() is documented in terms of it. I wasn't easily able to
determine the meaning for myself.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> +svn_error_t *
> +svn_client_log2 (const apr_array_header_t *targets,
[...]
> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c (revision 15298)
> +++ subversion/libsvn_client/log.c (working copy)
[...]
> @@ -157,13 +161,17 @@
> targets = real_targets;
> }
>
> - /* Open a repository session to the BASE_URL. */
> - SVN_ERR (svn_path_condense_targets (&base_name, NULL, targets, TRUE, pool));
> - SVN_ERR (svn_client__open_ra_session_internal (&ra_session, base_url,
> - base_name, NULL, NULL,
> - (NULL != base_name), TRUE,
> - ctx, pool));
> + if (start->kind == svn_opt_revision_number &&
> + end->kind == svn_opt_revision_number)
> + revision = (start->value.number > end->value.number) ? *start : *end;
> + else
> + revision.kind = svn_opt_revision_unspecified;
>
> + SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
> + &url_p, path,
> + peg_revision, &revision, ctx,
> + pool));
> +
Please could you explain this change. I don't understand why something special
is happening if both the start and end revisions are specified as numbers
rather than, for example, dates. And I don't really understand _what_ special
behaviour is happening in that case.
I see from the implementation of svn_client__ra_session_from_path() that an
unspecified "revision" means "revision = peg_revision", so wouldn't it be
clearer to make your "else" clause say "revision = *peg_revision" (or an
equivalent pointer assignment) ?
The rest of the code looks fine. (I haven't studued the regression tests.)
- 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 Julian Foad <ju...@btopenworld.com>.
S.Ramaswamy wrote:
> Sorry for the delay.
Me too - I'm away till the end of August. I don't suppose anyone else can take
this up? If not, I'll get to it eventually.
- 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 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
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
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 "S.Ramaswamy" <sr...@gmail.com>.
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.
]]]
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
S.Ramaswamy wrote:
>>
>>>>>+ if (start->kind == svn_opt_revision_number &&
>>>>>+ end->kind == svn_opt_revision_number)
>>>>>+ revision = (start->value.number > end->value.number) ? *start : *end;
>>>>>+ else
>>>>>+ revision.kind = svn_opt_revision_unspecified;
>>>>>
>>>>>+ SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
>>>>>+ &url_p, path,
>>>>>+ peg_revision, &revision, ctx,
>>>>>+ pool));
>>>>>+
>>>>
>>>>Please could you explain this change. I don't understand why something special
>>>>is happening if both the start and end revisions are specified as numbers
>>>>rather than, for example, dates. And I don't really understand _what_ special
>>>>behaviour is happening in that case.
>>
>>I'd still like you to explain this part of the patch if you would, please.
>
>>From the doc string for svn_client__ra_session_from_path(), revision is,
> "a desired revision REVISION, create an RA connection to that object as it
> exists in that revision, following copy history if necessary."
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?
- 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 Julian Foad <ju...@btopenworld.com>.
S.Ramaswamy wrote:
>
>> If log2 depends on "unspecified" having a particular meaning in
>>log3, then either that meaning must be documented publically for log3 or it is
>>an implementation detail that shouldn't be mentioned publically for log2. In
>>the former case, it would be fine for log2 to be described in terms of what
>>value it passes to log3; in the latter case, log2 would have to be described in
>>terms of its effect rather than that value which would be an implementation detail.
>
> Ok, I could expand the doc string for svn_client_log3(), to include:
>
> * @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.
>
> and the doc string for svn_client_log() would remain the same:
OK.
> Note that svn_client_checkout2(), svn_client_checkout have similar doc
> strings w.r.t peg revision.
Oh yes. And svn_client_export3(). And the private function
svn_client__ra_session_from_path() which ultimately handles the peg revision in
that way for those two and many other higher-level functions.
I have updated the doc strings for all three of those in r16034.
- 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>.
Sorry for the delay.
>
> I meant that, but more strongly: I meant the pair of doc strings are
> incomplete. If log2 depends on "unspecified" having a particular meaning in
> log3, then either that meaning must be documented publically for log3 or it is
> an implementation detail that shouldn't be mentioned publically for log2. In
> the former case, it would be fine for log2 to be described in terms of what
> value it passes to log3; in the latter case, log2 would have to be described in
> terms of its effect rather than that value which would be an implementation detail.
>
Ok, I could expand the doc string for svn_client_log3(), to include:
* @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.
and the doc string for svn_client_log() would remain the same:
Note that svn_client_checkout2(), svn_client_checkout have similar doc
strings w.r.t peg revision.
> Now that I write and think about this clearly enough to present those two
> options, I wonder whether log3 needs to accept "unspecified" as part of its
> publically documented interface. It would be better if it didn't, unless there
> is some good public reason to do so. We have too many unnecessary defaults
> already in APIs like this; we shouldn't add more unless there is a clear
> benefit. It could still accept "unspecified" privately to facilitate the
> implementation of log2.
>
> (When I said, "I wasn't easily able to determine the meaning for myself," I
> meant that I wasn't able to determine the meaning of "unspecified", even by
> looking at the implementation. It certainly wasn't possible from the doc strings.)
>
>
> >>>+ if (start->kind == svn_opt_revision_number &&
> >>>+ end->kind == svn_opt_revision_number)
> >>>+ revision = (start->value.number > end->value.number) ? *start : *end;
> >>>+ else
> >>>+ revision.kind = svn_opt_revision_unspecified;
> >>>
> >>>+ SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
> >>>+ &url_p, path,
> >>>+ peg_revision, &revision, ctx,
> >>>+ pool));
> >>>+
> >>
> >>Please could you explain this change. I don't understand why something special
> >>is happening if both the start and end revisions are specified as numbers
> >>rather than, for example, dates. And I don't really understand _what_ special
> >>behaviour is happening in that case.
>
> I'd still like you to explain this part of the patch if you would, please.
>
Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
S.Ramaswamy wrote:
>>>Index: subversion/include/svn_client.h
>>>===================================================================
>>>--- subversion/include/svn_client.h (revision 15298)
>>>+++ subversion/include/svn_client.h (working copy)
>>>@@ -915,7 +915,8 @@
>>> /**
>>> * Invoke @a receiver with @a receiver_baton on each log message from @a
>>> * start to @a end in turn, inclusive (but never invoke @a receiver on a
>>>- * given log message more than once).
>>>+ * given log message more than once). @a peg_revision indicates in which
>>>+ * revision @a targets are valid.
>>
>>This needs to specify what svn_opt_revision_unspecified means as a peg revision ...
>>
>>> * @a targets contains either a URL followed by zero or more relative
>>> * paths, or a list of working copy paths, as <tt>const char *</tt>,
>>
>>[...]
>>
>>> /**
>>>+ * Similar to svn_client_log3(), but with the @a peg_revision
>>>+ * parameter always set to @c svn_opt_revision_unspecified.
>>
>>... if svn_client_log2() is documented in terms of it. I wasn't easily able to
>>determine the meaning for myself.
>
> I guess you are saying that, if an explanation of
> svn_opt_revision_unspecified in the svn_client_log3() doc string is
> included, then the svn_client_log2() doc string becomes easier to
> understand. Will do.
I meant that, but more strongly: I meant the pair of doc strings are
incomplete. If log2 depends on "unspecified" having a particular meaning in
log3, then either that meaning must be documented publically for log3 or it is
an implementation detail that shouldn't be mentioned publically for log2. In
the former case, it would be fine for log2 to be described in terms of what
value it passes to log3; in the latter case, log2 would have to be described in
terms of its effect rather than that value which would be an implementation detail.
Now that I write and think about this clearly enough to present those two
options, I wonder whether log3 needs to accept "unspecified" as part of its
publically documented interface. It would be better if it didn't, unless there
is some good public reason to do so. We have too many unnecessary defaults
already in APIs like this; we shouldn't add more unless there is a clear
benefit. It could still accept "unspecified" privately to facilitate the
implementation of log2.
(When I said, "I wasn't easily able to determine the meaning for myself," I
meant that I wasn't able to determine the meaning of "unspecified", even by
looking at the implementation. It certainly wasn't possible from the doc strings.)
>>>+ if (start->kind == svn_opt_revision_number &&
>>>+ end->kind == svn_opt_revision_number)
>>>+ revision = (start->value.number > end->value.number) ? *start : *end;
>>>+ else
>>>+ revision.kind = svn_opt_revision_unspecified;
>>>
>>>+ SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
>>>+ &url_p, path,
>>>+ peg_revision, &revision, ctx,
>>>+ pool));
>>>+
>>
>>Please could you explain this change. I don't understand why something special
>>is happening if both the start and end revisions are specified as numbers
>>rather than, for example, dates. And I don't really understand _what_ special
>>behaviour is happening in that case.
I'd still like you to explain this part of the patch if you would, please.
>>I see from the implementation of svn_client__ra_session_from_path() that an
>>unspecified "revision" means "revision = peg_revision", so wouldn't it be
>>clearer to make your "else" clause say "revision = *peg_revision" (or an
>>equivalent pointer assignment) ?
>
> I overlooked the fact that dates/keywords can also be specified as
> revisions. I can change that to,
>
> if (start->kind == svn_opt_revision_number) &&
> (end->kind == svn_opt_revision_number)
> revision = (start->value.number > end->value.number) ? *start : *end;
> else if (start->kind == svn_opt_revision_date) &&
> (end->kind == svn_opt_revision_date)
> revision = (start->value.date > end->value.date) ? *start : *end;
>
> Revision numbers can also be specified as a combination of dates and
> revision numbers, as well as a combination of keywords (PREV, HEAD, etc),
> numbers etc. In those cases the revision gets set to
> svn_opt_revision_unspecified in the current patch(v2).
Yes, but _why_ do you want it set to "unspecified" in such cases? :-) (Maybe
it's really obvious, but not to me. Maybe I just haven't studied it hard enough.)
- 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>.
>
> > Index: subversion/include/svn_client.h
> > ===================================================================
> > --- subversion/include/svn_client.h (revision 15298)
> > +++ subversion/include/svn_client.h (working copy)
> > @@ -915,7 +915,8 @@
> > /**
> > * Invoke @a receiver with @a receiver_baton on each log message from @a
> > * start to @a end in turn, inclusive (but never invoke @a receiver on a
> > - * given log message more than once).
> > + * given log message more than once). @a peg_revision indicates in which
> > + * revision @a targets are valid.
>
> This needs to specify what svn_opt_revision_unspecified means as a peg revision ...
>
> > *
> > * @a targets contains either a URL followed by zero or more relative
> > * paths, or a list of working copy paths, as <tt>const char *</tt>,
> [...]
> > /**
> > + * Similar to svn_client_log3(), but with the @a peg_revision
> > + * parameter always set to @c svn_opt_revision_unspecified.
>
> ... if svn_client_log2() is documented in terms of it. I wasn't easily able to
> determine the meaning for myself.
>
I guess you are saying that, if an explanation of
svn_opt_revision_unspecified in the svn_client_log3() doc string is
included, then the svn_client_log2() doc string becomes easier to
understand. Will do.
> > + if (start->kind == svn_opt_revision_number &&
> > + end->kind == svn_opt_revision_number)
> > + revision = (start->value.number > end->value.number) ? *start : *end;
> > + else
> > + revision.kind = svn_opt_revision_unspecified;
> >
> > + SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
> > + &url_p, path,
> > + peg_revision, &revision, ctx,
> > + pool));
> > +
>
> Please could you explain this change. I don't understand why something special
> is happening if both the start and end revisions are specified as numbers
> rather than, for example, dates. And I don't really understand _what_ special
> behaviour is happening in that case.
>
> I see from the implementation of svn_client__ra_session_from_path() that an
> unspecified "revision" means "revision = peg_revision", so wouldn't it be
> clearer to make your "else" clause say "revision = *peg_revision" (or an
> equivalent pointer assignment) ?
I overlooked the fact that dates/keywords can also be specified as
revisions. I can change that to,
if (start->kind == svn_opt_revision_number) &&
(end->kind == svn_opt_revision_number)
revision = (start->value.number > end->value.number) ? *start : *end;
else if (start->kind == svn_opt_revision_date) &&
(end->kind == svn_opt_revision_date)
revision = (start->value.date > end->value.date) ? *start : *end;
Revision numbers can also be specified as a combination of dates and
revision numbers, as well as a combination of keywords (PREV, HEAD, etc),
numbers etc. In those cases the revision gets set to
svn_opt_revision_unspecified in the current patch(v2).
>
> The rest of the code looks fine. (I haven't studued the regression tests.)
>
I haven't added any new regression tests. Just modified existing ones
to work with the new syntax.
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>.
Thanks for the review. I will try to answer these questions after a few days.
Ramaswamy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org