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.
>>
>> /* Only check if the file was modified when it wasn't
>> overwritten with a
>> special file */
>> @@ -2482,16 +2491,15 @@ svn_wc_remove_from_revision_control(svn_
>> }
>>
>> /* Remove the wcprops. */
>> - SVN_ERR(svn_wc__props_delete(full_path, svn_wc__props_wcprop,
>> - adm_access, pool));
>> + SVN_ERR(svn_wc__props_delete(db, local_abspath,
>> svn_wc__props_wcprop,
>> + pool));
>> /* Remove prop/NAME, prop-base/NAME.svn-base. */
>> - SVN_ERR(svn_wc__props_delete(full_path, svn_wc__props_working,
>> - adm_access, pool));
>> - SVN_ERR(svn_wc__props_delete(full_path, svn_wc__props_base,
>> - adm_access, pool));
>> + SVN_ERR(svn_wc__props_delete(db, local_abspath,
>> svn_wc__props_working,
>> + pool));
>> + SVN_ERR(svn_wc__props_delete(db, local_abspath,
>> svn_wc__props_base,
>> + pool));
>>
>> /* Remove NAME from PATH's entries file: */
>> - SVN_ERR(svn_path_get_absolute(&local_abspath, full_path,
>> pool));
>> SVN_ERR(svn_wc__entry_remove(db, local_abspath, pool));
>>
>> /* Remove text-base/NAME.svn-base */
>> @@ -2534,8 +2542,8 @@ svn_wc_remove_from_revision_control(svn_
>> /* Get rid of all the wcprops in this directory. This avoids
>> rewriting
>> the wcprops file over and over (meaning O(n^2) complexity)
>> below. */
>> - SVN_ERR(svn_wc__props_delete(full_path, svn_wc__props_wcprop,
>> - adm_access, pool));
>> + SVN_ERR(svn_wc__props_delete(db, local_abspath,
>> svn_wc__props_wcprop,
>> + pool));
>>
>> /* Walk over every entry. */
>> SVN_ERR(svn_wc_entries_read(&entries, adm_access, FALSE,
>> pool));
>> @@ -2591,9 +2599,6 @@ svn_wc_remove_from_revision_control(svn_
>> /* The directory is either missing or excluded,
>> so don't try to recurse, just delete the
>> entry in the parent directory. */
>> - svn_wc__db_t *db = svn_wc__adm_get_db(adm_access);
>> - const char *local_abspath;
>> -
>> SVN_ERR(svn_path_get_absolute(&local_abspath,
>> entrypath,
>> subpool));
>
> Is it safe to ovewrite local_abspath? (I don't see all the context; is
> it needed again?) Maybe call this entry_abspath instead?
It *is* safe to overwrite here, but I agree that it could be clearer.
Overall, I think svn_wc_remove_from_revision_control is starting to
turn into a power plant, and could perhaps benefit from a little
refactoring.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2072613
Received on 2009-05-05 22:53:44 CEST