Paul Burba wrote:
> Julian Foad <julianfoad@btopenworld.com> wrote on 01/12/2006 07:39:00 PM:
>
>>My first reaction is: "What is special about BASE?" Why isn't handled the same
>>as PREV and COMMITTED which are the other two revision keywords that require a WC?
>
> What's special about it is that svn diff x:BASE asserts and the other
> cases are handled, albeit inconsistently. My thought was just to prevent
> the assert - that's my rather lame excuse anyway since I *did* include the
> svn diff BASE:x case which the current code doesn't assert on.
Yes, I understood that; sorry I wasn't clear, but what I meant was, "Why
doesn't our existing code behave consistently? Is there some problem with it
that we should fix, or is there a logical reason for the disparity?" In other
words, is this extra check necessary, or is there a broken/incomplete check in
place that needs to be corrected?
>>svn diff -r1:PREV file:///home/julianfoad/tmp/vcs/sandbox/trunk
>>svn: 'file:///home/julianfoad/tmp/vcs/sandbox' is not a working copy
> [[[
> Return consistent error on svn diff of special revisions BASE, COMMITTED,
> or PREV when operating on a URL.
>
> The original impetus for this patch was to stop svn diff 1:BASE URL from
> asserting and provide a more useful error message.
>
> * subversion/svn/diff-cmd.c
> (svn_cl__diff): If the starting or ending revision against a URL is
> BASE,
> COMMITTED, or PREV return an error.
> ]]]
>
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 18091)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -155,6 +155,19 @@
> return svn_error_createf (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> _("Target lists to diff may not contain
> "
> "both working copy paths and URLs"));
> +
> + /* Diffing special revisions BASE, COMMITTED, or PREV
> + against a URL is invalid. */
> + if (url_present
> + && (opt_state->start_revision.kind == svn_opt_revision_base
> + || opt_state->end_revision.kind == svn_opt_revision_base
> + || opt_state->start_revision.kind ==
> svn_opt_revision_committed
> + || opt_state->end_revision.kind ==
> svn_opt_revision_committed
> + || opt_state->start_revision.kind ==
> svn_opt_revision_previous
> + || opt_state->end_revision.kind ==
> svn_opt_revision_previous)
> +)
> + return svn_error_create
> + (SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED, NULL, NULL);
My only other comment is that it would be nice to include the path in the error
message, so that the cause is clear if this occurs in a command that was
handling multiple paths. That requires writing out most of the error message
long-hand, which is a bit ugly but worth it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jan 14 03:07:15 2006