[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] svn diff -r1:BASE URL asserts

From: Paul Burba <paulb_at_softlanding.com>
Date: 2006-01-17 15:51:17 CET

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

This is an archived mail posted to the Subversion Dev mailing list.