Re: svn commit: r37590 - trunk/subversion/libsvn_wc

On May 6, 2009, at 3:43 AM, Bert Huijben wrote:

>> -----Original Message-----
>> From: Hyrum K. Wright [mailto:hyrum_at_hyrumwright.org]
>> Sent: dinsdag 5 mei 2009 22:53
>> To: Greg Stein
>> Cc: dev_at_subversion.tigris.org
>> Subject: Re: svn commit: r37590 - trunk/subversion/libsvn_wc
>>
>> On May 5, 2009, at 3:44 PM, Greg Stein wrote:
>>
>>> On Tue, May 5, 2009 at 22:19, Hyrum K. Wright
>>> <hyrum_at_hyrumwright.org> wrote:
>>>> ...
>>>> +++ trunk/subversion/libsvn_wc/adm_ops.c Tue May 5 13:19:56
>>>> 2009 (r37590)
>>>> ...
>>>> @@ -2445,8 +2451,12 @@ svn_wc_remove_from_revision_control(svn_
>>>> svn_error_t *err;
>>>> svn_boolean_t is_file;
>>>> svn_boolean_t left_something = FALSE;
>>>> + svn_wc__db_t *db = svn_wc__adm_get_db(adm_access);
>>>> const char *full_path = apr_pstrdup(pool,
>>>>
>>>> svn_wc_adm_access_path(adm_access));
>>>
>>> I know this isn't part of your change, but this seems dumb. By
>>> definition, the baton's path will last longer than this function. Is
>>> there a reason to make a copy of this string?
>>
>> I don't know / haven't looked at it too deeply.
>>
>>>> + const char *local_abspath;
>>>> +
>>>> + SVN_ERR(svn_path_get_absolute(&local_abspath, full_path, pool));
>>>>
>>>> /* Check cancellation here, so recursive calls get checked early.
>>>> */
>>>> if (cancel_func)
>>>> @@ -2460,10 +2470,9 @@ svn_wc_remove_from_revision_control(svn_
>>>> svn_node_kind_t kind;
>>>> svn_boolean_t wc_special, local_special;
>>>> svn_boolean_t text_modified_p;
>>>> - svn_wc__db_t *db = svn_wc__adm_get_db(adm_access);
>>>> - const char *local_abspath;
>>>>
>>>> full_path = svn_dirent_join(full_path, name, pool);
>>>> + SVN_ERR(svn_path_get_absolute(&local_abspath, full_path,
>>>> pool));
>>>
>>> You did this above.
>>
>> Sure, but the value for full_path has changed, so this will yield a
>> different result.
>
> But an entryname/basename joined to an absolute path will always
> return an
> absolute path, so this step is not necessary.

Understood, but I don't see any place that we guarantee full_path is
an absolute path to begin with. It may be an absolute path, or it may
be a path relative to the cwd or maybe to the working copy root.
That's one of the problems with the current working copy: we keep
tossing around these paths, but don't really say what kind they are.
(And in many places, we aren't too strict about what we accept.)

> The only case where joining a basename to an absolute path doesn't
> give you
> an absolute path is when the basename is '.' or '..', but these
> values are
> never valid as entrynames.

-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2080345
Received on 2009-05-06 14:37:11 CEST