"Hyrum K. Wright" <hyrum_wright_at_mail.utexas.edu> writes:
> 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.
If rerunning is acceptable, why was it necessary to add the check in
the first place?
>
> 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.
Introducing dodgy code on the grounds that you won't need to look at
it again doesn't seem like a good idea. Buggy code is exactly the
code that should be audited. When will the bug get fixed?
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2363708
Received on 2009-06-20 00:57:35 CEST