[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r38077 - in trunk/subversion: libsvn_client tests/cmdline

From: Hyrum K. Wright <hyrum_at_hyrumwright.org>
Date: Fri, 26 Jun 2009 13:01:07 -0500

On Jun 19, 2009, at 5:57 PM, Philip Martin wrote:

> "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?

As of now (r38212), this call has been removed and the check is
occurring within libsvn_wc.

-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2365802
Received on 2009-06-26 20:01:46 CEST

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.