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