Re: [PATCH] Issue #2287 - Make svn_client_log() take a peg revision

Julian Foad <julianfoad@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 <julianfoad@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