On Jun 18, 2009, at 1:27 PM, Philip Martin wrote:
> "Hyrum K. Wright" <hyrum_at_hyrumwright.org> writes:
>
>> Author: hwright
>> Date: Thu Jun 18 09:00:36 2009
>> New Revision: 38077
>
>> svn_wc_adm_access_t *adm_access;
>> struct validator_baton_t vb;
>> + svn_node_kind_t kind;
>> +
>> + /* We can't relocate an individual file, so don't even try. */
>> + SVN_ERR(svn_io_check_path(path, &kind, pool));
>> + if (kind != svn_node_dir)
>> + return svn_error_create
>> + (SVN_ERR_CLIENT_INVALID_RELOCATION, NULL,
>> + _("Cannot relocate a single file"));
>>
>> /* Get an access baton for PATH. */
>> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, path,
>
> That's an anti-pattern: it's usually wrong for the WC code to (try to)
> validate an operation before attempting it. One reason it's wrong is
> that it leaves a race between the filesystem stat and the adm open.
> Another reason is that when the path is valid there is an unnecessary
> stat call. It would be better to determine whether path is valid by
> interrogating the access baton and calling stat if necessary in the
> error path.
Good points, but I don't know that they are valid here. (Note that,
strictly speaking, after this change, the WC code isn't doing the
validation; this check occurs in libsvn_client.)
Firstly, if the underlying node does somehow change between the stat
call and the adm open, the open would fail. If it does, the user
could just rerun the command.
More importantly, use of svn_wc_entry_t and adm_access batons will be
deprecated in 1.7, which would be the typical method of "interrogating
the access baton". The above code is an attempt not to introduce yet
another call to svn_wc_entry(), which will just have to be audited
away eventually.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2363650
Received on 2009-06-19 21:34:12 CEST