[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: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2006-03-08 12:08:46 CET

kfogel@collab.net wrote:
>>
>>http://svn.haxx.se/dev/archive-2006-01/0350.shtml
>
> Paul, +1 to commit. Here's a new patch, exactly the same as yours
> except with a custom error message, [...]

-1. This isn't the right solution. It solves the immediate problem, avoiding
the assertion failure, but this explicit check should not be necessary: the
problem (of the user having specified a revision keyword that doesn't make
sense with a URL) should be caught later when the code tries to resolve the
revision specifier to a revision number.

Paul (earlier in the thread) compared the running of diff "BASE:1" with
"1:BASE" and found where the latter went wrong:

--------------------------------------------------------------------------

(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 to check_paths() results in
   the struct diff_paths *diff_paths: the struct 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
   represents the invalid request BASE, invalid yes, but the paths
   for the BASE rev of an URL. struct 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
--------------------------------------------------------------------------

Paul wrote about that analysis:
> 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.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 8 12:09:38 2006

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