Josh, good work. Sorry I didn't review it earlier. It's a bit big, so I was
waiting for someone else to do it!
Josh Pieper wrote:
> 1. Is this desirable in the first place?
Yes.
> Adding tracing by default to
> sub-commands makes the most intuitive way to specify a specific
> revision, 'svn cat -r x URL', perform history tracing from HEAD.
> This could possibly take a long time, especially with the new authz
> security fixes. You would need to use 'svn cat URL@r' to get the
> same quick response as before.
Not necessarily. 'svn cat -r x URL' doesn't have to mean 'URL@HEAD'. We could
make it mean 'URL@x'. This would be backward-compatible. If fact, you could
argue that such compatibility is essential, even if it isn't the "nicest" meaning.
> --------------------------------------
>
> Add peg-revision options to blame, cat, ls, propget, and proplist. As
propget/list but not propset/edit/del? That sounds like a recipe for
confusion. Is there a technical reason for this or have you just not done the
others yet?
What happens if one of these commands is asked to search forward in history?
Especially if there are copies.
More comments below.
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 11562)
> +++ subversion/include/svn_client.h (working copy)
> @@ -792,9 +792,18 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> -/** Invoke @a receiver with @a receiver_baton on each line-blame item
> +/**
> + * @since New in 1.2.
> + *
> + * Invoke @a receiver with @a receiver_baton on each line-blame item
> * associated with revision @a end of @a path_or_url, using @a start
> - * as the default source of all blame.
> + * as the default source of all blame. @a peg_revision indicates
> + * which in which revision @a path_or_url is valid. The actual file
Too many "which"s. It must be Hallowe'en.
(Actually I make a distinction between the pronunciation of "which" and
"witch", but the pun still works ... lamely.)
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 11562)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -156,9 +156,9 @@
>
>
> /* Given PATH_OR_URL, which contains either a working copy path or an
> - absolute url and a revision REVISION, create an RA connection to
> - that object as it exists in that revision, following copy history
> - if necessary.
> + absolute url, a peg revision PEG_REVISON, and a desired revision
> + REVISION, create an RA connection to that object as it exists in
> + that revision, following copy history if necessary.
I think this needs to say what happens if it is asked to follow history
forwards, and so do all of the public functions that use it.
> Index: subversion/libsvn_client/ra.c
> ===================================================================
> --- subversion/libsvn_client/ra.c (revision 11562)
> +++ subversion/libsvn_client/ra.c (working copy)
> @@ -797,6 +797,7 @@
> svn_revnum_t *rev_p,
> const char **url_p,
> const char *path_or_url,
> + const svn_opt_revision_t *peg_revision_p,
> const svn_opt_revision_t *revision,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> @@ -816,38 +821,54 @@
> SVN_ERR (svn_ra_init_ra_libs (&ra_baton, pool));
> SVN_ERR (svn_ra_get_ra_library (&ra_lib, ra_baton, initial_url, pool));
>
> + /* If a peg revision was specified, but a desired revision was not,
> + assume it is the same as the peg revision. */
Is this necessary? If so, it should be documented.
It is generally better to start with a tight API (e.g. requiring that "desired"
be specified if "peg" is) as you can then loosen it later without breaking
compatibility, if such loosening of the interface is found to be really useful.
> + if (revision->kind == svn_opt_revision_unspecified &&
> + peg_revision_p->kind != svn_opt_revision_unspecified)
> + revision = peg_revision_p;
> +
> if (svn_path_is_url (path_or_url))
> {
> - /* If an explicit URL was passed in, just use it. */
> - good_rev = revision;
> - url = initial_url;
> + /* URLs get a default starting rev of HEAD. */
> + if (revision->kind == svn_opt_revision_unspecified)
> + start_rev.kind = svn_opt_revision_head;
> + else
> + start_rev = *revision;
> +
> + /* If an explicit URL was passed in, the default peg revision is
> + HEAD. */
> + if (peg_revision_p->kind == svn_opt_revision_unspecified)
> + peg_revision.kind = svn_opt_revision_head;
> + else
> + peg_revision = *peg_revision_p;
> }
> else
> {
> - /* For a working copy path, don't blindly use its initial_url
> - from the entries file. Run the history function to get the
> - object's (possibly different) url in REVISION. */
> - svn_opt_revision_t base_rev, dead_end_rev, start_rev;
> - svn_opt_revision_t *ignored_rev, *new_rev;
> - const char *ignored_url;
> -
> - dead_end_rev.kind = svn_opt_revision_unspecified;
> - base_rev.kind = svn_opt_revision_working;
> -
> + /* And a default starting rev of BASE. */
> if (revision->kind == svn_opt_revision_unspecified)
> start_rev.kind = svn_opt_revision_base;
> else
> start_rev = *revision;
> -
> - SVN_ERR (svn_client__repos_locations (&url, &new_rev,
> - &ignored_url, &ignored_rev,
> - /* peg coords are path@BASE: */
> - path_or_url, &base_rev,
> - /* search range: */
> - &start_rev, &dead_end_rev,
> - ra_lib, ctx, pool));
> - good_rev = new_rev;
> +
> + /* WC paths have a default peg revision of WORKING. */
> + if (peg_revision_p->kind == svn_opt_revision_unspecified)
> + peg_revision.kind = svn_opt_revision_working;
> + else
> + peg_revision = *peg_revision_p;
> }
> +
> + dead_end_rev.kind = svn_opt_revision_unspecified;
> +
> + /* Run the history function to get the object's (possibly
> + different) url in REVISION. */
> + SVN_ERR (svn_client__repos_locations (&url, &new_rev,
> + &ignored_url, &ignored_rev,
> + /* peg coords are path@BASE: */
> + path_or_url, &peg_revision,
Comment on previous line: not "@BASE" any more, is it?
> + /* search range: */
> + &start_rev, &dead_end_rev,
> + ra_lib, ctx, pool));
> + good_rev = new_rev;
>
> SVN_ERR (svn_client__open_ra_session (&session, ra_lib, url,
> NULL, NULL, NULL, FALSE, FALSE,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Nov 2 16:08:18 2004