You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by William Nagel <bi...@stagelogic.com> on 2007/02/19 05:23:02 UTC
[PATCH] Issue #2531
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
Re: [PATCH] Issue #2531
Posted by William Nagel <bi...@stagelogic.com>.
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
Re: [PATCH] Issue #2531
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Hyrum K. Wright wrote:
> Ping...
>
> Has anybody had a chance to take a look at William's updated patch? If
> not, I'll put it in the issue tracker with issue 2531.
Done. (Sorry it took so long!)
-Hyrum
> William Nagel wrote:
>> Here's an updated patch for issue 2531, which adds support for a warning
>> message that gets displayed if a user tries to perform a relocate with a
>> source URL that doesn't exist in the working copy.
>>
>>> I think this patch would cause svn to warn on valid multi-target
>>> operations
>>> (if any of the targets aren't relocated).
>> I added some verbiage to the warning message to specify exactly which
>> target in a multi-target operation was left unchanged. If multiple
>> targets are unchanged, multiple warning messages will be displayed.
>> This seems to me to be the most viable way to make the relocate work
>> within the confines of the current API.
>>
>>> 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".
>>>
>>>> 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.
>>>
>>>> +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.
>> All nits fixed.
>>
>> -Bill
>>
>> [[[
>> Fix issue #2531: Added a warning message when 'svn switch --relocate'
>> doesn't
>> result in any changes.
>>
>> * subversion/libsvn_wc/relocate.c
>> (relocate_entry): Add a touched parameter. Add a check to set touched to
>> true if the entry is actually relocated.
>>
>> (svn_wc_relocate2): Extract out all of the contents of svn_wc_relocate2
>> and moved it into the new perform_relocate function. Add code to
>> initialize touched, call perform_relocate, and then throw a warning if
>> touched returns as false.
>>
>> (perform_relocate): New function, created from the former contents of
>> svn_wc_relocate2. Add a touched parameter, to be passed through to all
>> calls to relocate_entry.
>>
>> * subversion/tests/cmdline/switch_tests.py
>> (relocate_with_bad_url): New test case to exercise relocation without a
>> valid FROM url.
>> ]]]
>> Index: subversion/libsvn_wc/relocate.c
>> ===================================================================
>> --- subversion/libsvn_wc/relocate.c (revision 23560)
>> +++ subversion/libsvn_wc/relocate.c (working copy)
>> @@ -35,13 +35,15 @@
>> * 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. If the entry is actually modified, TOUCHED will
>> + * be set to true.
>> */
>> static svn_error_t *
>> relocate_entry(svn_wc_adm_access_t *adm_access,
>> const svn_wc_entry_t *entry,
>> const char *from,
>> const char *to,
>> + svn_boolean_t *touched,
>> svn_wc_relocation_validator2_t validator,
>> void *validator_baton,
>> svn_boolean_t do_sync,
>> @@ -104,17 +106,33 @@
>> }
>> if (flags)
>> - SVN_ERR(svn_wc__entry_modify(adm_access, entry->name,
>> - &entry2, flags, do_sync, pool));
>> + {
>> + SVN_ERR(svn_wc__entry_modify(adm_access, entry->name,
>> + &entry2, flags, do_sync, pool));
>> + (*touched) = TRUE;
>> + }
>> +
>> return SVN_NO_ERROR;
>> }
>> -svn_error_t *
>> -svn_wc_relocate2(const char *path,
>> +
>> +/* Change repository references at PATH that begin with FROM to begin
>> + * with TO instead. Perform necessary allocations in POOL. If RECURSE
>> + * is true, do so. VALIDATOR (and its baton, VALIDATOR_BATON), will
>> + * be called for each newly generated URL.
>> + *
>> + * ADM_ACCESS is an access baton for the directory containing PATH.
>> + *
>> + * TOUCHED should point to a boolean to be used for keeping track of
>> + * whether any repository references were actually changed.
>> + */
>> +static svn_error_t *
>> +perform_relocate(const char * path,
>> svn_wc_adm_access_t *adm_access,
>> const char *from,
>> const char *to,
>> svn_boolean_t recurse,
>> + svn_boolean_t *touched,
>> svn_wc_relocation_validator2_t validator,
>> void *validator_baton,
>> apr_pool_t *pool)
>> @@ -130,7 +148,7 @@
>> if (entry->kind == svn_node_file)
>> {
>> - SVN_ERR(relocate_entry(adm_access, entry, from, to,
>> + SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
>> validator, validator_baton, TRUE /* sync */,
>> pool));
>> return SVN_NO_ERROR;
>> @@ -143,7 +161,7 @@
>> validator has to do. ### Should svn_wc.h document the ordering? */
>> SVN_ERR(svn_wc_entries_read(&entries, adm_access, TRUE, pool));
>> entry = apr_hash_get(entries, SVN_WC_ENTRY_THIS_DIR,
>> APR_HASH_KEY_STRING);
>> - SVN_ERR(relocate_entry(adm_access, entry, from, to,
>> + SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
>> validator, validator_baton, FALSE, pool));
>> subpool = svn_pool_create(pool);
>> @@ -171,11 +189,11 @@
>> continue;
>> SVN_ERR(svn_wc_adm_retrieve(&subdir_access, adm_access,
>> subdir, subpool));
>> - SVN_ERR(svn_wc_relocate2(subdir, subdir_access, from, to,
>> - recurse, validator,
>> + SVN_ERR(perform_relocate(subdir, subdir_access, from, to,
>> + recurse, touched, validator,
>> validator_baton, subpool));
>> }
>> - SVN_ERR(relocate_entry(adm_access, entry, from, to,
>> + SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
>> validator, validator_baton, FALSE, subpool));
>> }
>> @@ -186,6 +204,30 @@
>> return SVN_NO_ERROR;
>> }
>> +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 = FALSE;
>> + SVN_ERR(perform_relocate(path, adm_access, from, to, recurse, &touched,
>> + validator, validator_baton, pool));
>> + if (!touched)
>> + return svn_error_create(SVN_WARNING, NULL,
>> + apr_psprintf(pool,
>> + "FROM url '%s' wasn't found in
>> '%s':"
>> + " no changes were made to '%s'",
>> + svn_path_local_style(from, pool),
>> + svn_path_local_style(path, pool),
>> + svn_path_local_style(path,
>> pool)));
>> + return SVN_NO_ERROR;
>> +}
>> +
>> /* Compatibility baton and wrapper. */
>> struct compat_baton {
>> svn_wc_relocation_validator_t validator;
>> Index: subversion/tests/cmdline/switch_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/switch_tests.py (revision 23560)
>> +++ subversion/tests/cmdline/switch_tests.py (working copy)
>> @@ -1402,7 +1402,25 @@
>> 'add', file_path)
>> svntest.actions.run_and_verify_svn(None, None, [],
>> 'switch', switch_url, file_path)
>> +
>> +#----------------------------------------------------------------------
>> +#Issue 2531
>> +def relocate_with_bad_url(sbox):
>> + "relocate with a non-existant starting url"
>> + sbox.build()
>> + wc_dir = sbox.wc_dir
>> + repo_url = sbox.repo_url
>> +
>> + # A relocate with a source URL that doesn't exist.
>> + # This should output a warning
>> + svntest.actions.run_and_verify_svn(None, None,
>> + ".*FROM url '%s' wasn't found in
>> '%s':"
>> + " no changes were made to '%s'.*" %
>> + ('file://foo', wc_dir, wc_dir),
>> + 'switch', '--relocate',
>> + 'file://foo', 'file://bar', wc_dir)
>> +
>> ########################################################################
>> # Run the tests
>> @@ -1430,6 +1448,7 @@
>> forced_switch,
>> forced_switch_failures,
>> switch_scheduled_add,
>> + relocate_with_bad_url,
>> ]
>> if __name__ == '__main__':
>>
>>
>
Re: [PATCH] Issue #2531
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Ping...
Has anybody had a chance to take a look at William's updated patch? If
not, I'll put it in the issue tracker with issue 2531.
-Hyrum
William Nagel wrote:
> Here's an updated patch for issue 2531, which adds support for a warning
> message that gets displayed if a user tries to perform a relocate with a
> source URL that doesn't exist in the working copy.
>
>> I think this patch would cause svn to warn on valid multi-target
>> operations
>> (if any of the targets aren't relocated).
>
> I added some verbiage to the warning message to specify exactly which
> target in a multi-target operation was left unchanged. If multiple
> targets are unchanged, multiple warning messages will be displayed.
> This seems to me to be the most viable way to make the relocate work
> within the confines of the current API.
>
>> 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".
>>
>>> 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.
>>
>>> +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.
>
> All nits fixed.
>
> -Bill
>
> [[[
> Fix issue #2531: Added a warning message when 'svn switch --relocate'
> doesn't
> result in any changes.
>
> * subversion/libsvn_wc/relocate.c
> (relocate_entry): Add a touched parameter. Add a check to set touched to
> true if the entry is actually relocated.
>
> (svn_wc_relocate2): Extract out all of the contents of svn_wc_relocate2
> and moved it into the new perform_relocate function. Add code to
> initialize touched, call perform_relocate, and then throw a warning if
> touched returns as false.
>
> (perform_relocate): New function, created from the former contents of
> svn_wc_relocate2. Add a touched parameter, to be passed through to all
> calls to relocate_entry.
>
> * subversion/tests/cmdline/switch_tests.py
> (relocate_with_bad_url): New test case to exercise relocation without a
> valid FROM url.
> ]]]
> Index: subversion/libsvn_wc/relocate.c
> ===================================================================
> --- subversion/libsvn_wc/relocate.c (revision 23560)
> +++ subversion/libsvn_wc/relocate.c (working copy)
> @@ -35,13 +35,15 @@
> * 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. If the entry is actually modified, TOUCHED will
> + * be set to true.
> */
> static svn_error_t *
> relocate_entry(svn_wc_adm_access_t *adm_access,
> const svn_wc_entry_t *entry,
> const char *from,
> const char *to,
> + svn_boolean_t *touched,
> svn_wc_relocation_validator2_t validator,
> void *validator_baton,
> svn_boolean_t do_sync,
> @@ -104,17 +106,33 @@
> }
> if (flags)
> - SVN_ERR(svn_wc__entry_modify(adm_access, entry->name,
> - &entry2, flags, do_sync, pool));
> + {
> + SVN_ERR(svn_wc__entry_modify(adm_access, entry->name,
> + &entry2, flags, do_sync, pool));
> + (*touched) = TRUE;
> + }
> +
> return SVN_NO_ERROR;
> }
> -svn_error_t *
> -svn_wc_relocate2(const char *path,
> +
> +/* Change repository references at PATH that begin with FROM to begin
> + * with TO instead. Perform necessary allocations in POOL. If RECURSE
> + * is true, do so. VALIDATOR (and its baton, VALIDATOR_BATON), will
> + * be called for each newly generated URL.
> + *
> + * ADM_ACCESS is an access baton for the directory containing PATH.
> + *
> + * TOUCHED should point to a boolean to be used for keeping track of
> + * whether any repository references were actually changed.
> + */
> +static svn_error_t *
> +perform_relocate(const char * path,
> svn_wc_adm_access_t *adm_access,
> const char *from,
> const char *to,
> svn_boolean_t recurse,
> + svn_boolean_t *touched,
> svn_wc_relocation_validator2_t validator,
> void *validator_baton,
> apr_pool_t *pool)
> @@ -130,7 +148,7 @@
> if (entry->kind == svn_node_file)
> {
> - SVN_ERR(relocate_entry(adm_access, entry, from, to,
> + SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
> validator, validator_baton, TRUE /* sync */,
> pool));
> return SVN_NO_ERROR;
> @@ -143,7 +161,7 @@
> validator has to do. ### Should svn_wc.h document the ordering? */
> SVN_ERR(svn_wc_entries_read(&entries, adm_access, TRUE, pool));
> entry = apr_hash_get(entries, SVN_WC_ENTRY_THIS_DIR,
> APR_HASH_KEY_STRING);
> - SVN_ERR(relocate_entry(adm_access, entry, from, to,
> + SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
> validator, validator_baton, FALSE, pool));
> subpool = svn_pool_create(pool);
> @@ -171,11 +189,11 @@
> continue;
> SVN_ERR(svn_wc_adm_retrieve(&subdir_access, adm_access,
> subdir, subpool));
> - SVN_ERR(svn_wc_relocate2(subdir, subdir_access, from, to,
> - recurse, validator,
> + SVN_ERR(perform_relocate(subdir, subdir_access, from, to,
> + recurse, touched, validator,
> validator_baton, subpool));
> }
> - SVN_ERR(relocate_entry(adm_access, entry, from, to,
> + SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
> validator, validator_baton, FALSE, subpool));
> }
> @@ -186,6 +204,30 @@
> return SVN_NO_ERROR;
> }
> +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 = FALSE;
> + SVN_ERR(perform_relocate(path, adm_access, from, to, recurse, &touched,
> + validator, validator_baton, pool));
> + if (!touched)
> + return svn_error_create(SVN_WARNING, NULL,
> + apr_psprintf(pool,
> + "FROM url '%s' wasn't found in
> '%s':"
> + " no changes were made to '%s'",
> + svn_path_local_style(from, pool),
> + svn_path_local_style(path, pool),
> + svn_path_local_style(path,
> pool)));
> + return SVN_NO_ERROR;
> +}
> +
> /* Compatibility baton and wrapper. */
> struct compat_baton {
> svn_wc_relocation_validator_t validator;
> Index: subversion/tests/cmdline/switch_tests.py
> ===================================================================
> --- subversion/tests/cmdline/switch_tests.py (revision 23560)
> +++ subversion/tests/cmdline/switch_tests.py (working copy)
> @@ -1402,7 +1402,25 @@
> 'add', file_path)
> svntest.actions.run_and_verify_svn(None, None, [],
> 'switch', switch_url, file_path)
> +
> +#----------------------------------------------------------------------
> +#Issue 2531
> +def relocate_with_bad_url(sbox):
> + "relocate with a non-existant starting url"
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + repo_url = sbox.repo_url
> +
> + # A relocate with a source URL that doesn't exist.
> + # This should output a warning
> + svntest.actions.run_and_verify_svn(None, None,
> + ".*FROM url '%s' wasn't found in
> '%s':"
> + " no changes were made to '%s'.*" %
> + ('file://foo', wc_dir, wc_dir),
> + 'switch', '--relocate',
> + 'file://foo', 'file://bar', wc_dir)
> +
> ########################################################################
> # Run the tests
> @@ -1430,6 +1448,7 @@
> forced_switch,
> forced_switch_failures,
> switch_scheduled_add,
> + relocate_with_bad_url,
> ]
> if __name__ == '__main__':
>
>
Re: [PATCH] Issue #2531
Posted by William Nagel <bi...@stagelogic.com>.
Here's an updated patch for issue 2531, which adds support for a
warning message that gets displayed if a user tries to perform a
relocate with a source URL that doesn't exist in the working copy.
> I think this patch would cause svn to warn on valid multi-target
> operations
> (if any of the targets aren't relocated).
I added some verbiage to the warning message to specify exactly which
target in a multi-target operation was left unchanged. If multiple
targets are unchanged, multiple warning messages will be displayed.
This seems to me to be the most viable way to make the relocate work
within the confines of the current API.
> 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".
>
>> 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.
>
>> +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.
All nits fixed.
-Bill
[[[
Fix issue #2531: Added a warning message when 'svn switch --relocate'
doesn't
result in any changes.
* subversion/libsvn_wc/relocate.c
(relocate_entry): Add a touched parameter. Add a check to set
touched to
true if the entry is actually relocated.
(svn_wc_relocate2): Extract out all of the contents of
svn_wc_relocate2
and moved it into the new perform_relocate function. Add code to
initialize touched, call perform_relocate, and then throw a
warning if
touched returns as false.
(perform_relocate): New function, created from the former contents of
svn_wc_relocate2. Add a touched parameter, to be passed through
to all
calls to relocate_entry.
* subversion/tests/cmdline/switch_tests.py
(relocate_with_bad_url): New test case to exercise relocation
without a
valid FROM url.
]]]
Index: subversion/libsvn_wc/relocate.c
===================================================================
--- subversion/libsvn_wc/relocate.c (revision 23560)
+++ subversion/libsvn_wc/relocate.c (working copy)
@@ -35,13 +35,15 @@
* 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. If the entry is actually modified, TOUCHED
will
+ * be set to true.
*/
static svn_error_t *
relocate_entry(svn_wc_adm_access_t *adm_access,
const svn_wc_entry_t *entry,
const char *from,
const char *to,
+ svn_boolean_t *touched,
svn_wc_relocation_validator2_t validator,
void *validator_baton,
svn_boolean_t do_sync,
@@ -104,17 +106,33 @@
}
if (flags)
- SVN_ERR(svn_wc__entry_modify(adm_access, entry->name,
- &entry2, flags, do_sync, pool));
+ {
+ SVN_ERR(svn_wc__entry_modify(adm_access, entry->name,
+ &entry2, flags, do_sync, pool));
+ (*touched) = TRUE;
+ }
+
return SVN_NO_ERROR;
}
-svn_error_t *
-svn_wc_relocate2(const char *path,
+
+/* Change repository references at PATH that begin with FROM to begin
+ * with TO instead. Perform necessary allocations in POOL. If RECURSE
+ * is true, do so. VALIDATOR (and its baton, VALIDATOR_BATON), will
+ * be called for each newly generated URL.
+ *
+ * ADM_ACCESS is an access baton for the directory containing PATH.
+ *
+ * TOUCHED should point to a boolean to be used for keeping track of
+ * whether any repository references were actually changed.
+ */
+static svn_error_t *
+perform_relocate(const char * path,
svn_wc_adm_access_t *adm_access,
const char *from,
const char *to,
svn_boolean_t recurse,
+ svn_boolean_t *touched,
svn_wc_relocation_validator2_t validator,
void *validator_baton,
apr_pool_t *pool)
@@ -130,7 +148,7 @@
if (entry->kind == svn_node_file)
{
- SVN_ERR(relocate_entry(adm_access, entry, from, to,
+ SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
validator, validator_baton, TRUE /*
sync */,
pool));
return SVN_NO_ERROR;
@@ -143,7 +161,7 @@
validator has to do. ### Should svn_wc.h document the
ordering? */
SVN_ERR(svn_wc_entries_read(&entries, adm_access, TRUE, pool));
entry = apr_hash_get(entries, SVN_WC_ENTRY_THIS_DIR,
APR_HASH_KEY_STRING);
- SVN_ERR(relocate_entry(adm_access, entry, from, to,
+ SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
validator, validator_baton, FALSE, pool));
subpool = svn_pool_create(pool);
@@ -171,11 +189,11 @@
continue;
SVN_ERR(svn_wc_adm_retrieve(&subdir_access, adm_access,
subdir, subpool));
- SVN_ERR(svn_wc_relocate2(subdir, subdir_access, from, to,
- recurse, validator,
+ SVN_ERR(perform_relocate(subdir, subdir_access, from, to,
+ recurse, touched, validator,
validator_baton, subpool));
}
- SVN_ERR(relocate_entry(adm_access, entry, from, to,
+ SVN_ERR(relocate_entry(adm_access, entry, from, to, touched,
validator, validator_baton, FALSE,
subpool));
}
@@ -186,6 +204,30 @@
return SVN_NO_ERROR;
}
+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 = FALSE;
+ SVN_ERR(perform_relocate(path, adm_access, from, to, recurse,
&touched,
+ validator, validator_baton, pool));
+ if (!touched)
+ return svn_error_create(SVN_WARNING, NULL,
+ apr_psprintf(pool,
+ "FROM url '%s' wasn't found
in '%s':"
+ " no changes were made to '%
s'",
+ svn_path_local_style(from,
pool),
+ svn_path_local_style(path,
pool),
+ svn_path_local_style(path,
pool)));
+ return SVN_NO_ERROR;
+}
+
/* Compatibility baton and wrapper. */
struct compat_baton {
svn_wc_relocation_validator_t validator;
Index: subversion/tests/cmdline/switch_tests.py
===================================================================
--- subversion/tests/cmdline/switch_tests.py (revision 23560)
+++ subversion/tests/cmdline/switch_tests.py (working copy)
@@ -1402,7 +1402,25 @@
'add', file_path)
svntest.actions.run_and_verify_svn(None, None, [],
'switch', switch_url, file_path)
+
+#----------------------------------------------------------------------
+#Issue 2531
+def relocate_with_bad_url(sbox):
+ "relocate with a non-existant starting url"
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ repo_url = sbox.repo_url
+
+ # A relocate with a source URL that doesn't exist.
+ # This should output a warning
+ svntest.actions.run_and_verify_svn(None, None,
+ ".*FROM url '%s' wasn't found
in '%s':"
+ " no changes were made to '%
s'.*" %
+ ('file://foo', wc_dir, wc_dir),
+ 'switch', '--relocate',
+ 'file://foo', 'file://bar',
wc_dir)
+
########################################################################
# Run the tests
@@ -1430,6 +1448,7 @@
forced_switch,
forced_switch_failures,
switch_scheduled_add,
+ relocate_with_bad_url,
]
if __name__ == '__main__':
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2531
Posted by mark benedetto king <mb...@lowlatency.com>.
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.
I think this patch would cause svn to warn on valid multi-target operations
(if any of the targets aren't relocated).
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".
> 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.
> +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.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org