Ben Collins-Sussman wrote:
> On 10/13/07, lgo@tigris.org <lgo@tigris.org> wrote:
>> Author: lgo
>> Date: Sat Oct 13 03:49:30 2007
>> New Revision: 27170
>>
>> Log:
>> Fix fetching a file from the server during copy-on-update over ra_neon &
>> ra_serf.
>>
>> * subversion/libsvn_wc/update_editor.c
>> (add_file_with_history): copyfrom_path is absolute but fetch_func requires
>> a relative path, so skip the first '/'.
>>
>>
>> Modified:
>> trunk/subversion/libsvn_wc/update_editor.c
>>
>> Modified: trunk/subversion/libsvn_wc/update_editor.c
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/update_editor.c?pathrev=27170&r1=27169&r2=27170
>> ==============================================================================
>> --- trunk/subversion/libsvn_wc/update_editor.c (original)
>> +++ trunk/subversion/libsvn_wc/update_editor.c Sat Oct 13 03:49:30 2007
>> @@ -3061,7 +3061,9 @@
>> (APR_WRITE | APR_TRUNCATE | APR_CREATE),
>> pool));
>>
>> - SVN_ERR(eb->fetch_func(eb->fetch_baton, copyfrom_path, copyfrom_rev,
>> + /* copyfrom_path is a absolute path, fetch_func requires a path relative
>> + to the root of the repository so skip the first '/'. */
>> + SVN_ERR(eb->fetch_func(eb->fetch_baton, copyfrom_path + 1, copyfrom_rev,
>> svn_stream_from_aprfile(textbase_file, pool),
>> NULL, &base_props, pool));
>> }
>
>
> I'm trying to grok this change. Is it true that each RA layer sends a
> copyfrom_path to the update_editor which starts with '/'? If so,
> that's good.
>
> But it sounds like the bug here is that the four implementations of
> svn_ra_get_file() are inconsistent. It sounds like two of the
> implementations have no problem with svn_ra_get_file(session, "/foo")
> -- interpreting the file to fetch as "SessionRootURL/foo"... but the
> two http implementations interpret this invocation as just "/foo",
> ignoring the SessionRootURL?
>
> So while this change ensures that we always call
> svn_ra_get_file(session, "foo"), it seems to cover up the underlying
> problem of inconsistency. Can we fix the inconsistency? Is
> svn_ra_get_file(session, "/foo") a legal invocation? If not, then
> maybe libsvn_ra.so should reject it?
svn_ra_get_file(session, "/foo") is *not* a legal invocation. The paths
provided to the RA API are, as the various docstring state, relative to the
session URL. And that's how all four RA layers treat them. But some of the
those layers can pass the "relative" path through to the FS directly, while
others need to construct URLs out of them. And svn_path_join(session-url,
"/foo") == "/foo", not "session-url/foo".
Your use of the API was broken here, nothing more.
That said, we *do* have several similar inconsistencies throughout the
codebase, and -- worse -- we have lacking documentation. Do the paths in an
svn_log_entry_t's changed_paths hash have leading slashes? What about the
returns values of svn_client__path_relative_to_root and
svn_client__get_copy_source?
I *strongly* suggest we consistify, and that we consistify on *not* having
leading slashes on relative paths floating our our APIs (because the minute
you try to svn_path_join() them to some base, you get bogosity. It doesn't
matter in the *least* that the paths are absolute in terms of their location
in the backend filesystem -- that's an implementation detail, really, and if
this is a hangup for you, remember they are also relative to the root of
that filesystem.
--
C. Michael Pilato <cmpilato@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 13 16:20:37 2007