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/07/05 16:55:25 UTC
Re: svn commit: r38328 - trunk/subversion/libsvn_client
On Fri, Jul 3, 2009 at 21:25, Hyrum K. Wright<hy...@hyrumwright.org> wrote:
>...
> +++ trunk/subversion/libsvn_client/copy.c Fri Jul 3 12:25:44 2009 (r38328)
>...
> @@ -1992,7 +1996,7 @@ svn_client_copy5(svn_commit_info_t **com
> svn_wc_context_t *wc_ctx;
>
> if (!ctx->wc_ctx)
> - SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, subpool,
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool,
> subpool));
This doesn't seem like a proper change. You need to destroy the
context at the end of the function, so the subpool seems the proper
choice. If you were going to stash it into the client context, then
*maybe* pool would be proper.
>...
> +++ trunk/subversion/libsvn_client/merge.c Fri Jul 3 12:25:44 2009 (r38328)
>...
> @@ -7622,6 +7672,12 @@ svn_client_merge3(const char *source1,
> _("'%s' has no URL"),
> svn_dirent_local_style(source2, pool));
>
> + /* Create the wc context. */
> + if (!ctx->wc_ctx)
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
> + else
> + wc_ctx = ctx->wc_ctx;
> +
> /* Open an admistrative session with the working copy. */
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, target_wcpath,
> ! dry_run, -1, ctx->cancel_func,
> @@ -7766,6 +7822,7 @@ svn_client_merge3(const char *source1,
> record_only, dry_run,
> merge_options,
> &use_sleep, ctx,
> + wc_ctx,
> pool);
> if (err)
> {
> @@ -7802,7 +7859,7 @@ svn_client_merge3(const char *source1,
> ancestral, related, same_repos,
> ignore_ancestry, force, dry_run,
> record_only, FALSE, depth, merge_options,
> - &use_sleep, ctx, pool);
> + &use_sleep, ctx, wc_ctx, pool);
>
> if (use_sleep)
> svn_io_sleep_for_timestamps(target_wcpath, pool);
> @@ -7810,6 +7867,8 @@ svn_client_merge3(const char *source1,
> if (err)
> return svn_error_return(err);
>
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
This could destroy ctx->wc_ctx, couldn't it? iow, Bad.
>...
> @@ -8527,6 +8586,13 @@ svn_client_merge_reintegrate(const char
> static const svn_wc_entry_callbacks2_t walk_callbacks =
> { get_subtree_mergeinfo_walk_cb, get_mergeinfo_error_handler };
> struct get_subtree_mergeinfo_walk_baton wb;
> + svn_wc_context_t *wc_ctx;
> +
> + /* Create the wc context. */
> + if (!ctx->wc_ctx)
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
> + else
> + wc_ctx = ctx->wc_ctx;
>
> /* Open an admistrative session with the working copy. */
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, target_wcpath,
> @@ -8676,7 +8742,7 @@ svn_client_merge_reintegrate(const char
> svn_depth_infinity,
> FALSE, FALSE, FALSE, dry_run,
> merge_options, &use_sleep,
> - ctx, pool);
> + ctx, wc_ctx, pool);
>
> if (use_sleep)
> svn_io_sleep_for_timestamps(target_wcpath, pool);
> @@ -8684,6 +8750,8 @@ svn_client_merge_reintegrate(const char
> if (err)
> return svn_error_return(err);
>
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
Same here.
>...
> @@ -8715,11 +8783,18 @@ svn_client_merge_peg3(const char *source
> svn_boolean_t use_sleep = FALSE;
> svn_error_t *err;
> svn_boolean_t same_repos;
> + svn_wc_context_t *wc_ctx;
>
> /* No ranges to merge? No problem. */
> if (ranges_to_merge->nelts == 0)
> return SVN_NO_ERROR;
>
> + /* Create the wc context. */
> + if (!ctx->wc_ctx)
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
> + else
> + wc_ctx = ctx->wc_ctx;
> +
> /* Open an admistrative session with the working copy. */
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, target_wcpath,
> (! dry_run), -1, ctx->cancel_func,
> @@ -8778,7 +8853,7 @@ svn_client_merge_peg3(const char *source
> err = do_merge(merge_sources, target_wcpath, entry, adm_access,
> TRUE, TRUE, same_repos, ignore_ancestry, force, dry_run,
> record_only, FALSE, depth, merge_options,
> - &use_sleep, ctx, pool);
> + &use_sleep, ctx, wc_ctx, pool);
>
> if (use_sleep)
> svn_io_sleep_for_timestamps(target_wcpath, pool);
> @@ -8786,6 +8861,8 @@ svn_client_merge_peg3(const char *source
> if (err)
> return svn_error_return(err);
>
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
And here.
>...
> +++ trunk/subversion/libsvn_client/mergeinfo.c Fri Jul 3 12:25:44 2009 (r38328)
>...
> @@ -113,17 +118,17 @@ svn_client__record_wc_mergeinfo(const ch
> /* Record the new mergeinfo in the WC. */
> /* ### Later, we'll want behavior more analogous to
> ### svn_client__get_prop_from_wc(). */
> - SVN_ERR(svn_wc_prop_set3(SVN_PROP_MERGEINFO, mergeinfo_str, wcpath,
> - adm_access, TRUE /* skip checks */, NULL, NULL,
> - pool));
> + SVN_ERR(svn_wc_prop_set4(wc_ctx, local_abspath, SVN_PROP_MERGEINFO,
> + mergeinfo_str, TRUE /* skip checks */, NULL, NULL,
> + scratch_pool));
>
> if (ctx->notify_func2)
> {
> ctx->notify_func2(ctx->notify_baton2,
> - svn_wc_create_notify(wcpath,
> + svn_wc_create_notify(local_abspath,
Is this a valid change? To change from relative to absolute paths?
ISTR that the notification callbacks do a lot of path munging to get
"good looking paths", and this could end up changing the "contract"
about the types of paths we feed into the notification system. Maybe a
different notification structure that explicitly contains abspaths?
>...
> @@ -1248,6 +1263,12 @@ svn_client_mergeinfo_log_merged(const ch
> svn_opt_revision_t *real_src_peg_revision;
> apr_hash_index_t *hi;
> svn_revnum_t youngest_rev = SVN_INVALID_REVNUM;
> + svn_wc_context_t *wc_ctx;
> +
> + if (!ctx->wc_ctx)
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
> + else
> + wc_ctx = ctx->wc_ctx;
>
> /* Step 1: Ensure that we have a merge source URL to work with. */
> SVN_ERR(location_from_path_and_rev(&merge_source_url, &real_src_peg_revision,
> @@ -1262,7 +1283,7 @@ svn_client_mergeinfo_log_merged(const ch
> do). */
> /* This get_mergeinfo() call doubles as a mergeinfo capabilities check. */
> SVN_ERR(get_mergeinfo(&tgt_mergeinfo, &repos_root, path_or_url,
> - peg_revision, ctx, pool));
> + peg_revision, ctx, wc_ctx, pool));
> if (! tgt_mergeinfo)
> return SVN_NO_ERROR;
> SVN_ERR(svn_client__get_history_as_mergeinfo(&source_history,
> @@ -1310,10 +1331,14 @@ svn_client_mergeinfo_log_merged(const ch
> using a receiver filter to only allow revisions to pass through
> that are in our rangelist. */
> log_target = svn_path_url_add_component2(repos_root, log_target + 1, pool);
> - return logs_for_mergeinfo_rangelist(log_target, rangelist,
> - discover_changed_paths, revprops,
> - log_receiver, log_receiver_baton,
> - ctx, pool);
> + SVN_ERR(logs_for_mergeinfo_rangelist(log_target, rangelist,
> + discover_changed_paths, revprops,
> + log_receiver, log_receiver_baton,
> + ctx, pool));
> +
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
Looks like another aggressive destruction here.
>...
> @@ -1327,9 +1352,15 @@ svn_client_mergeinfo_get_merged(apr_hash
> const char *repos_root;
> apr_hash_t *full_path_mergeinfo;
> svn_mergeinfo_t mergeinfo;
> + svn_wc_context_t *wc_ctx;
> +
> + if (!ctx->wc_ctx)
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
> + else
> + wc_ctx = ctx->wc_ctx;
>
> SVN_ERR(get_mergeinfo(&mergeinfo, &repos_root, path_or_url,
> - peg_revision, ctx, pool));
> + peg_revision, ctx, wc_ctx, pool));
>
> /* Copy the MERGEINFO hash items into another hash, but change
> the relative paths into full URLs. */
> @@ -1354,6 +1385,8 @@ svn_client_mergeinfo_get_merged(apr_hash
> *mergeinfo_p = full_path_mergeinfo;
> }
>
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
And here.
>...
> @@ -1379,6 +1412,12 @@ svn_client_mergeinfo_log_eligible(const
> svn_revnum_t youngest_rev = SVN_INVALID_REVNUM;
> apr_array_header_t *rangelist;
> const char *log_target = NULL;
> + svn_wc_context_t *wc_ctx;
> +
> + if (!ctx->wc_ctx)
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
> + else
> + wc_ctx = ctx->wc_ctx;
>
> /* Step 1: Ensure that we have a merge source URL to work with. */
> SVN_ERR(location_from_path_and_rev(&merge_source_url, &real_src_peg_revision,
>...
> @@ -1458,10 +1497,14 @@ svn_client_mergeinfo_log_eligible(const
> using a receiver filter to only allow revisions to pass through
> that are in our rangelist. */
> log_target = svn_path_url_add_component2(repos_root, log_target + 1, pool);
> - return logs_for_mergeinfo_rangelist(log_target, rangelist,
> - discover_changed_paths, revprops,
> - log_receiver, log_receiver_baton,
> - ctx, pool);
> + SVN_ERR(logs_for_mergeinfo_rangelist(log_target, rangelist,
> + discover_changed_paths, revprops,
> + log_receiver, log_receiver_baton,
> + ctx, pool));
> +
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
And here.
>...
> @@ -1478,6 +1521,12 @@ svn_client_suggest_merge_sources(apr_arr
> svn_revnum_t copyfrom_rev;
> svn_mergeinfo_t mergeinfo;
> apr_hash_index_t *hi;
> + svn_wc_context_t *wc_ctx;
> +
> + if (!ctx->wc_ctx)
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
> + else
> + wc_ctx = ctx->wc_ctx;
>
> list = apr_array_make(pool, 1, sizeof(const char *));
>
>...
> @@ -1524,6 +1573,8 @@ svn_client_suggest_merge_sources(apr_arr
> }
> }
>
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
Guess what? :-P
>...
> +++ trunk/subversion/libsvn_client/prop_commands.c Fri Jul 3 12:25:44 2009 (r38328)
>...
> @@ -856,6 +854,12 @@ svn_client_propget3(apr_hash_t **props,
> const svn_wc_entry_t *node;
> svn_boolean_t pristine;
> int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
> + svn_wc_context_t *wc_ctx;
> +
> + if (!ctx->wc_ctx)
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
> + else
> + wc_ctx = ctx->wc_ctx;
>
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, path_or_url,
> FALSE, adm_lock_level,
> @@ -873,8 +877,10 @@ svn_client_propget3(apr_hash_t **props,
>
> SVN_ERR(svn_client__get_prop_from_wc(*props, propname, path_or_url,
> pristine, node, adm_access,
> - depth, changelists, ctx, pool));
> + depth, changelists, ctx, wc_ctx,
> + pool));
>
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
Last one.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2368158
Re: svn commit: r38328 - trunk/subversion/libsvn_client
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Jul 5, 2009, at 11:55 AM, Greg Stein wrote:
> On Fri, Jul 3, 2009 at 21:25, Hyrum K. Wright<hy...@hyrumwright.org>
> wrote:
>> ...
>> +++ trunk/subversion/libsvn_client/copy.c Fri Jul 3 12:25:44
>> 2009 (r38328)
>> ...
>> @@ -1992,7 +1996,7 @@ svn_client_copy5(svn_commit_info_t **com
>> svn_wc_context_t *wc_ctx;
>>
>> if (!ctx->wc_ctx)
>> - SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */,
>> subpool,
>> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool,
>> subpool));
>
> This doesn't seem like a proper change. You need to destroy the
> context at the end of the function, so the subpool seems the proper
> choice. If you were going to stash it into the client context, then
> *maybe* pool would be proper.
This (and the other wc_ctx issues) should be addressed by r38345.
>> ...
>> +++ trunk/subversion/libsvn_client/mergeinfo.c Fri Jul 3 12:25:44
>> 2009 (r38328)
>> ...
>> @@ -113,17 +118,17 @@ svn_client__record_wc_mergeinfo(const ch
>> /* Record the new mergeinfo in the WC. */
>> /* ### Later, we'll want behavior more analogous to
>> ### svn_client__get_prop_from_wc(). */
>> - SVN_ERR(svn_wc_prop_set3(SVN_PROP_MERGEINFO, mergeinfo_str,
>> wcpath,
>> - adm_access, TRUE /* skip checks */,
>> NULL, NULL,
>> - pool));
>> + SVN_ERR(svn_wc_prop_set4(wc_ctx, local_abspath,
>> SVN_PROP_MERGEINFO,
>> + mergeinfo_str, TRUE /* skip checks */,
>> NULL, NULL,
>> + scratch_pool));
>>
>> if (ctx->notify_func2)
>> {
>> ctx->notify_func2(ctx->notify_baton2,
>> - svn_wc_create_notify(wcpath,
>> + svn_wc_create_notify(local_abspath,
>
> Is this a valid change? To change from relative to absolute paths?
> ISTR that the notification callbacks do a lot of path munging to get
> "good looking paths", and this could end up changing the "contract"
> about the types of paths we feed into the notification system. Maybe a
> different notification structure that explicitly contains abspaths?
IIRC, we don't include the type of path in the docs for our
notification structure, so either absolute or relative paths are
acceptable. Our current notification system does some munging of the
absolute path to create a relative path, if it can, so most
notification output won't change. I think the only case where this
doesn't happen is when we give parent relative paths on the command
line.
For example 'svn up ../..' will produce absolute paths in the output,
but 'svn up foo/bar' will produce paths relative to the cwd.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2368409