You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2009/05/05 20:44:16 UTC
Re: svn commit: r37590 - trunk/subversion/libsvn_wc
On Tue, May 5, 2009 at 22:19, Hyrum K. Wright <hy...@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?
> + 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.
>
> /* 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?
>...
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2072542
Re: svn commit: r37590 - trunk/subversion/libsvn_wc
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On May 6, 2009, at 3:43 AM, Bert Huijben wrote:
>> -----Original Message-----
>> From: Hyrum K. Wright [mailto:hyrum@hyrumwright.org]
>> Sent: dinsdag 5 mei 2009 22:53
>> To: Greg Stein
>> Cc: dev@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
>>> <hy...@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
RE: svn commit: r37590 - trunk/subversion/libsvn_wc
Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: Hyrum K. Wright [mailto:hyrum@hyrumwright.org]
> Sent: dinsdag 5 mei 2009 22:53
> To: Greg Stein
> Cc: dev@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
> > <hy...@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.
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.
Bert
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2078486
Re: svn commit: r37590 - trunk/subversion/libsvn_wc
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On May 5, 2009, at 3:44 PM, Greg Stein wrote:
> On Tue, May 5, 2009 at 22:19, Hyrum K. Wright
> <hy...@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