On 05 Jul 2004 12:04:17 -0500, kfogel@collab.net <kfogel@collab.net> wrote:
> Garrett Rooney <rooneg@gmail.com> writes:
> > Still on the todo list:
> > More comments
> > Command line client help output
> > Reduce the code duplication in the various log functions
> > Implement ra_svn and ra_dav versions
> > Maybe make get_log_bounded functions take an array of paths, not just one.
> > Maybe add backwards compatability code so this can work with older servers?
>
> And log message, I hope :-).
Yes, of course ;-)
> Quick comments -- this was not as thorough a review as I'd like, but
> something's better than nothing I guess:
Thanks, I appreciate it.
> > Index: subversion/include/svn_repos.h
> > ===================================================================
> > --- subversion/include/svn_repos.h (revision 10134)
> > +++ subversion/include/svn_repos.h (working copy)
> > @@ -538,6 +538,25 @@
> > apr_pool_t *pool);
> >
> > /**
> > + * @since New in 1.1
> > + *
> > + * Call @a history_func (with @a history_baton) for each interesting
> > + * history location in the lifetime of @a path in @a fs, for @a num_back
> > + * revisions starting at @a start and going backwards. Only cross
> > + * filesystem copy history if @a cross_copies is @c TRUE. And do all
> > + * of this in @a pool.
> > + */
> > +svn_error_t *
> > +svn_repos_history_bounded (svn_fs_t *fs,
> > + const char *path,
> > + svn_repos_history_func_t history_func,
> > + void *history_baton,
> > + svn_revnum_t start,
> > + unsigned int num_back,
> > + svn_boolean_t cross_copies,
> > + apr_pool_t *pool);
> > +
> > +/**
>
> Why not change 'num_back' to a signed number 'distance', and reverse
> its default sense? That way the API can always go some distance
> behind *or* ahead of a given revision.
The issue with that (which is also why I'm not convinced on the new
syntax options people have proposed) is that I'm not entirely sure how
implementing 'FOO + 10' should work in the face of copies. Do we
follow all the paths forward and find the earliest 10? Plus, I don't
think we can easily do that now anyway within the limits of the
current filesystems, at least that was the impression I'd gotten from
other discussions on the lists...
> In terms of documenting this, just say it's "same as calling
> svn_repos_history() with end == start + distance" or something like
> that (with the right formatting codes, of course).
>
> > * @since New in 1.1.
> > *
> > * Set @a *locations to be a mapping of the revisions to the paths of
> > @@ -626,6 +645,18 @@
> > apr_pool_t *pool);
> >
> >
> > +svn_error_t *
> > +svn_repos_get_logs_bounded (svn_repos_t *repos,
> > + const char *path,
> > + svn_revnum_t start,
> > + unsigned int num_back,
> > + svn_boolean_t discover_changed_paths,
> > + svn_boolean_t strict_node_history,
> > + svn_log_message_receiver_t receiver,
> > + void *receiver_baton,
> > + apr_pool_t *pool);
> > +
>
> Same comments here.
>
> > /* ---------------------------------------------------------------*/
> >
> > /**
> > Index: subversion/include/svn_ra.h
> > ===================================================================
> > --- subversion/include/svn_ra.h (revision 10134)
> > +++ subversion/include/svn_ra.h (working copy)
> > @@ -733,6 +733,15 @@
> > apr_array_header_t *location_revisions,
> > apr_pool_t *pool);
> >
> > + svn_error_t *(*get_log_bounded) (void *session_baton,
> > + const char *path,
> > + svn_revnum_t start,
> > + unsigned int num_back,
> > + svn_boolean_t discover_changed_paths,
> > + svn_boolean_t strict_node_history,
> > + svn_log_message_receiver_t receiver,
> > + void *receiver_baton,
> > + apr_pool_t *pool);
> > } svn_ra_plugin_t;
>
> This needs to be documented like other RA vtable functions, of course.
Naturally.
> > Index: subversion/include/svn_opt.h
> > ===================================================================
> > --- subversion/include/svn_opt.h (revision 10134)
> > +++ subversion/include/svn_opt.h (working copy)
> > @@ -197,7 +197,10 @@
> > svn_opt_revision_working,
> >
> > /** repository youngest */
> > - svn_opt_revision_head
> > + svn_opt_revision_head,
> > +
> > + /** an offset relative to another revision */
> > + svn_opt_revision_offset
> > };
>
> I guess this whole bit might change now, with the suggestion of
> "-rHEAD:HEAD-10" syntax that others have made. (So I won't comment on
> any of the parsing code.)
I still need to formulate my thoughts on the syntax others have suggested...
> As far as backwards compatibilty: When the server is new enough to
> understand the new type of request, there should be no extra round
> trip. When the server is old, it can error back to the client, which
> the client can detect, and only *then* do an extra round-trip to
> get the offset revision by number so it can re-request.
Yeah, that makes sense.
> So, here's a wicked question: shouldn't an offset of "HEAD-10" on a
> file show you the last ten revisions *in which that file changed*, not
> simply the revisions numbered HEAD through HEAD-10? :-) (Or did your
> code already do that, and I just missed it?)
It does do that already ;-) The repos level functions use the path
you gave it to root the history, then go back in time using that, so
you start at the most recent rev in which that item changed and move
back through the list of revs where it changed. It pretty much works
the way I'd expect it to, and if I'm reading your question right I
think it works the way you'd expect it to ;-)
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 6 01:50:56 2004