Re: [PATCH] Issue #2531

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__':
>>
>>
>