On Thu, 24 Mar 2005, Daniel Berlin wrote:
> Sussman asked me to post the current work i have on streamy blame (done
> by blaming files from youngest revision to oldest, instead of the other
> way around) since it was hoping to be ready for 1.2.
>
I think this improvement is worthwhile, but I thought it was mostly about
streaminess, not speed. sussman gave another impression in his mail.
>
> Otherwise, i plan on testing and committing the server side and api
> changes(IE everything but the call to reverse_blame and the associate
> reverse blame functions on the *client* side), because it adds an
> argument to svn_ra_get_file_revs, which is new API for 1.2, and thus, if
> we don't do it now, we're going to have to add *another* function in
> 1.3.
>
I don't see a problem with revising another API. I don't like pushing in
an API change this late in the 1.2 cycle, though. And since this
functionality won't be used before 1.3 anyway (it isn't for a patch
release), I don't see the point in doing this.
> Unless someone has a better idea, or believes we should put it all off
> *anyway*,etc.
>
>
I think the idea is fine and I'm willing to work with you with it for 1.3.
But I htink we should punt for 1.2. (Blaming bing ChangeLog files isn't
that a common use case, is it?:-) > --Dan
I also have some API comments, which might make my arguments stronger...
> Index: subversion/include/svn_error_codes.h
> ===================================================================
> --- subversion/include/svn_error_codes.h (revision 13634)
> +++ subversion/include/svn_error_codes.h (working copy)
> @@ -884,6 +884,12 @@
> SVN_ERRDEF (SVN_ERR_CLIENT_MISSING_LOCK_TOKEN,
> SVN_ERR_CLIENT_CATEGORY_START + 13,
> "Path has no lock token")
> +
> + /* @since New in 1.2. */
> + SVN_ERRDEF (SVN_ERR_CLIENT_DONE_WITH_BLAME,
> + SVN_ERR_CLIENT_CATEGORY_START + 14,
> + "Client finished blame early")
> +
>
Earlier, we've rejected new error codes for such special cases. For
example, I have a memory of us rejecting the Java bindings wanting to ad
an error code for filtering exceptions.
Also, I don't like the implementation technique. This is the same
technique that svn log --limit used to exit early. The problem is that it
leaves the connection useless afterwards (mostly thinking of svnserve
here). We should consider designing the protocol in such a way that we can
exit gracefully, so that other users than libsvn_client could use it,
without having to punt on the RA session.
> /* misc errors */
>
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 13634)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -881,6 +881,18 @@
> svn_repos_file_rev_handler_t handler,
> void *handler_baton,
> apr_pool_t *pool);
> +/** Same as svn_repos_get_file_revs, except return the revisions in order from
> + * youngest to oldest. */
(This is missing a @since, but that's a detail.)
> +svn_error_t *svn_repos_get_file_revs_reverse (svn_repos_t *repos,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_repos_authz_func_t authz_read_func,
> + void *authz_read_baton,
> + svn_repos_file_rev_handler_t handler,
> + void *handler_baton,
> + apr_pool_t *pool);
> +
>
in svn_repos_get_log, the output order depends on which revision argument
comes first. I think we should do the same here, for consistency. I am not
sure if we need to "clarify the semantics" to include this. Else we could
just create an svn_repos_get_file_revs2 and deprecate the current.
>
> /* ---------------------------------------------------------------*/
> Index: subversion/include/svn_ra.h
> ===================================================================
> --- subversion/include/svn_ra.h (revision 13634)
> +++ subversion/include/svn_ra.h (working copy)
> @@ -918,6 +918,7 @@
> const char *path,
> svn_revnum_t start,
> svn_revnum_t end,
> + svn_boolean_t reverse,
> svn_ra_file_rev_handler_t handler,
> void *handler_baton,
> apr_pool_t *pool);
See above. This is messy. Here you introduced a flag. in libsvn_repos, you
added another API.
Since this is new in 1.2, we can prepare for 1.3 by clarifying that if the
revision order is newest-oldest, an not implemented error will be
returned, but that mihgt change. We have to think about the wrapper,
though.
> @@ -1242,7 +1243,8 @@
> * @since New in 1.1.
> *
> * Call @c svn_ra_get_file_revs with the session associated with
> - * @a session_baton and all other arguments.
> + * @a session_baton and all other arguments, and the reverse argument
> + * set to false.
> */
> svn_error_t *(*get_file_revs) (void *session_baton,
> const char *path,
> Index: subversion/libsvn_ra/ra_loader.h
> ===================================================================
> --- subversion/libsvn_ra/ra_loader.h (revision 13634)
> +++ subversion/libsvn_ra/ra_loader.h (working copy)
> @@ -198,6 +198,13 @@
> apr_hash_t **locks,
> const char *path,
> apr_pool_t *pool);
> + svn_error_t *(*get_file_revs_reverse) (svn_ra_session_t *session,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_ra_file_rev_handler_t handler,
> + void *handler_baton,
> + apr_pool_t *pool);
> } svn_ra__vtable_t;
>
Why diverge from the public API here? If an implementation needed two
functions, it could dispatch, but that's an implementation detail.
> /* The RA session object. */
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 29 12:15:10 2005