Julian Foad <julianfoad@btopenworld.com> wrote on 01/13/2006 09:06:24 PM:
> 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?
Hi Julian,
I don't have a definitive answer, but debugging a similar diff that
doesn't assert, side-by-side with the problem case, there are a couple
points where things seem a bit odd.
--------------------------------------------------------------------------------
(Working Scenario) (Asserting Scenario)
svn diff BASE:1 URL svn diff 1:BASE URL
--------------------------------------------------------------------------------
svn/diff-cmd.c svn/diff-cmd.c
svn__cl_diff() svn__cl_diff()
pegged_diff is determined only by The first potential problem,
looking at the start revision pegged_diff == TRUE, so we call:
kind. It is TRUE iff start
revision kind is not
svn_opt_revision_base or
svn_opt_revision_working.
So it's FALSE in this case which
results in call to:
--------------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
svn_client_diff3() svn_client_diff_peg3()
--------------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
do_diff() do_diff_peg()
Call check_paths() and get Our second potential problem, the
call
the struct diff_paths to check_paths() results in the
struct
*diff_paths: diff_paths* diff_paths:
diff_paths->is_local_rev1 1 diff_paths->is_local_rev1 0
diff_paths->is_local_rev2 0 diff_paths->is_local_rev2 1
diff_paths->is_repos_path1 1 diff_paths->is_repos_path1 1
diff_paths->is_repos_path2 1 diff_paths->is_repos_path2 0
Here the paths struct accurately But path 2 is a URL at revision
BASE,
represents the invalid request invalid yes, but the paths struct
for the BASE rev of an URL. doesn't reflect this and causes a
call
to:
--------------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
diff_repos_repos() diff_repos_wc()
And here we run into the assert:
assert (! svn_path_is_url
(path2));
--------------------------------------------------------------------------------
libsvn_client/diff.c
diff_prepare_repos_repos()
Here we call
svn_client__get_revision_number()
on a NULL path.
--------------------------------------------------------------------------------
libsvn_client/revisions.c
svn_client__get_revision_number()
Returns SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED
svn: A path under version control is needed for this operation
----------------------------------------------------------------
----------------
I'm afraid I don't have a thorough enough grasp of the implications of
changing svn__cl_diff(), do_diff_peg(), and/or check_paths() to say that
any of these could be "fixed" in a way that makes my patch unnecessary. I
can start digging into this some more, but I'm hoping that the above might
turn on the light bulb for someone who is more familiar with this code.
> >>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.
I'm not sure this matters; If the svn diff command is handling multiple
targets there are three scenarios:
1) Targets are all WCS
2) Targets are mixed WCS and URLS
3) Targets are all URLS
In the first case the check in my patch is never made because url_present
== FALSE.
In the second case the existing test of (url_present &&
working_copy_present) returns the "Target lists to diff may not contain
both working copy paths and URLs" error.
And in the last case *all* the target URLS are invalid and always will be,
since they are URLS :-)
...unless I am missing something.
Paul B.
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jan 17 19:06:52 2006