Re: [PATCH] Issue #2531

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