On Feb 25, 2007, at 10:26 PM, mark benedetto king wrote:
> On Mon, Feb 19, 2007 at 12:23:02AM -0500, William Nagel wrote:
>> I'm attaching a patch to fix issue #2531. It adds a warning message
>> to 'svn switch --relocate' to let users know when a relocate doesn't
>> result in any actual rewriting of the working copy URLs.
>>
>> This is only my second patch submission for Subversion, so I'm very
>> open to any commentary.
>>
>> -Bill
>>
>
> It's slightly more difficult to review attached patches, which is
> why we prefer them in-line.
Duly noted.
>
> I think this patch would cause svn to warn on valid multi-target
> operations
> (if any of the targets aren't relocated).
Yes, you are right. I missed that. That brings up a question. With
the current interface, it would be impossible to return status
information about the separate relocates up to a point where a
decision could be made on whether to output a single warning if none
of the targets were relocated, unless there is some mechanism for
that in the Subversion architecture that I'm not aware of (and I do
consider myself a newbie on the SVN code).
On the other hand, it seems like it would be useful to warn the user
for each target that wasn't modified in a multi-target operation. So
for example, if file://foo existed in dir1 but not dir2 the following:
svn switch --relocate file://foo file://bar dir1 dir2
would output something like:
FROM url wasn't found in PATH dir2: no changes were made to dir2
Or, if file://foo didn't exist in either of them the output would be:
FROM url wasn't found in PATH dir1: no changes were made to dir1
FROM url wasn't found in PATH dir2: no changes were made to dir2
What do you think?
>
> Otherwise, a few nits:
>
>> @@ -35,13 +35,16 @@
>> * TO. ADM_ACCESS is the access baton for ENTRY. If DO_SYNC is
>> set then
>> * the new entry will be written to disk immediately, otherwise
>> only the
>> * entries cache will be affected. Calls VALIDATOR passing
>> VALIDATOR_BATON
>> - * to validate new URLs.
>> + * to validate new URLs. Takes a pointer COUNT that references
>> the number
>> + * of modifications made. If the entry is actually modified,
>> TOUCHED will
>> + * be set to true.
>> */
>
> Stray sentence about "pointer COUNT".
Oops, I'll take that out.
>
>> svn_error_t *
>> -svn_wc_relocate2(const char *path,
>> +perform_relocate(const char * path,
> svn_wc_adm_access_t *adm_access,
> const char *from,
>
> This function should be declared static.
Yes, thanks.
>
>> +svn_error_t *
>> +svn_wc_relocate2(const char *path,
>> + svn_wc_adm_access_t *adm_access,
>> + const char *from,
>> + const char *to,
>> + svn_boolean_t recurse,
>> + svn_wc_relocation_validator2_t validator,
>> + void *validator_baton,
>> + apr_pool_t *pool)
>> +{
>> + svn_boolean_t *touched = apr_pcalloc(pool, sizeof(svn_boolean_t));
>> + SVN_ERR(perform_relocate(path, adm_access, from, to, recurse,
>> touched,
>> + validator, validator_baton, pool));
>> + if (!(*touched))
>
> There's no need to allocate touched in a pool.
Yeah, I guess that's true. I generally don't like passing around
pointers to variables on the stack but in this case I guess it really
wouldn't cause any problems.
Thanks for the feedback. I'll update the patch once there's a
consensus on what to do with multi-target warnings.
-Bill
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Feb 28 01:29:59 2007