On Sep 14, 2009, at 3:53 PM, Daniel Näslund wrote:
> Hi!
>
> I'm touching a lot of code here. The callbacks not only gets called
> from
> the outside but they call each other too. I had to make some temporary
> wc_ctx objects to be passed around. The fact that there are three(!)
> different diff-functions got me a bit confused. The fact that one of
> them is placed in libsvn_client though it uses wc_diff_callbacks got
> me
> even more confused.
>
> Passes make check with no failures.
>
> [[[
> As part of WC-NG, replace adm_access parameter with wc_ctx in
> wc_diff_callback file_changed().
>
> * subversion/include/svn_wc.h
> (file_changed): Replaced adm_access parameter with wc_ctx.
>
> * subversion/libsvn_wc_deprecated.c
> (wrap_4to3_file_changed): Replaced adm_access parameter with wc_ctx.
> Create an adm_access for calling older callbacks.
>
> * subversion/libsvn_client/repos_diff.c
> (edit_baton): Add wc_ctx field.
> (close_file): Call file_changed() callback with wc_ctx instead of
> adm_access.
> (svn_client__get_diff_editor): Add ctx parameter. Set eb->wc_ctx to
> ctx->wc_ctx.
>
> * subversion/libsvn_client/client.h
> (svn_client__get_diff_editor): Add ctx parameter.
>
> * subversion/libsvn_client/merge.c
> (merge_file_changed): Replaced adm_access parameter with wc_ctx.
> Retrieve adm_access from wc_ctx for calling other wc functions.
> (drive_merge_report_editor): Add ctx parameter when calling
> svn_client__get_diff_editor().
> (do_file_merge): Replace adm_access parameter with ctx->wc_ctx when
> calling merge_file_changed().
>
> * subversion/libsvn_client/diff.c
> (diff_file_changed): Replace adm_access parameter with wc_ctx. Add
> temporary adm_access for calling diff_props_changed(). Set to
> NULL since
> it was called with NULL.
> (diff_file_added): Create temporary wc_ctx for calling
> diff_file_changed(). Should be removed when diff_file_added() uses
> wc_ctx.
> (diff_file_deleted_with_diff): Create temporary wc_ctx for calling
> diff_file_changed(). Should be removed when
> diff_file_deleted_with_diff() uses wc_ctx.
> (diff_repos_repos): Add ctx parameter when calling
> svn_client__get_diff_editor().
> ]]]
Daniel,
Glad to see your continued work on this; comments below.
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 39314)
> +++ subversion/include/svn_wc.h (arbetskopia)
> @@ -1906,7 +1906,7 @@
> * property name.
> *
> */
> - svn_error_t *(*file_changed)(svn_wc_adm_access_t *adm_access,
> + svn_error_t *(*file_changed)(svn_wc_context_t *wc_ctx,
I wonder if we need to include the wc_ctx argument here at all, and
instead just let callers squirrel away a wc_ctx in their baton if they
are going to need it in the callback. (This would also mean you
wouldn't need to construct the temporary context below.)
> svn_wc_notify_state_t *contentstate,
> svn_wc_notify_state_t *propstate,
> svn_boolean_t *tree_conflicted,
> Index: subversion/libsvn_wc/deprecated.c
> ===================================================================
> --- subversion/libsvn_wc/deprecated.c (revision 39314)
> +++ subversion/libsvn_wc/deprecated.c (arbetskopia)
> @@ -39,6 +39,7 @@
> #include "props.h"
>
> #include "svn_private_config.h"
> +#include "private/svn_wc_private.h"
>
> /* baton for traversal_info_update */
> struct traversal_info_update_baton
> @@ -1275,7 +1276,7 @@
> /* An svn_wc_diff_callbacks4_t function for wrapping
> * svn_wc_diff_callbacks3_t. */
> static svn_error_t *
> -wrap_4to3_file_changed(svn_wc_adm_access_t *adm_access,
> +wrap_4to3_file_changed(svn_wc_context_t *wc_ctx,
> svn_wc_notify_state_t *contentstate,
> svn_wc_notify_state_t *propstate,
> svn_boolean_t *tree_conflicted,
> @@ -1291,11 +1292,21 @@
> void *diff_baton)
> {
> struct diff_callbacks3_wrapper_baton *b = diff_baton;
> + svn_wc_adm_access_t *adm_access;
>
> - return b->callbacks3->file_changed(adm_access, contentstate,
> propstate,
> - tree_conflicted, path,
> tmpfile1, tmpfile2,
> - rev1, rev2, mimetype1,
> mimetype2,
> - propchanges, originalprops, b-
> >baton);
> + SVN_ERR(svn_wc__adm_open_in_context(&adm_access, wc_ctx, path,
> + TRUE, /* write_lock */
> + -1, /* levels_to_lock */
> + NULL, /* svn_cancel_func_t */
> + NULL, /* cancel_baton */
> + wc_ctx->state_pool));
I don't yet know what the endgame is for svn_wc__adm_open_in_context()
and friends. I was kinda under the assumption that they would be
removed before 1.7, but maybe a need like this changes that assumption?
In any case, I'd like to see a local helper function would could
construct the dummy access baton for a given path, so that if/when how
that gets constructed changes, we only have to modify it one place in
the code, rather than search for a bunch of them. Maybe something
like make_dummy_adm_access()?
Also, any reason to grab the write lock and make the levels to lock
infinite? Did we make that a promise of our pre-1.7 APIs? Looking at
the 1.6 docs, we just say "@a adm_access will be an access baton for
the directory containing @a path, or @c NULL if the diff editor is not
using access batons." Does that mean we can return a shallow read
baton, or even no baton at all? Should our current code be checking
for a NULL baton?
> +
> + SVN_ERR(b->callbacks3->file_changed(adm_access, contentstate,
> propstate,
> + tree_conflicted, path,
> tmpfile1, tmpfile2,
> + rev1, rev2, mimetype1,
> mimetype2,
> + propchanges, originalprops, b-
> >baton));
> +
> + svn_error_return(svn_wc_adm_close2(adm_access, wc_ctx-
> >state_pool));
> }
>
> /* An svn_wc_diff_callbacks4_t function for wrapping
> Index: subversion/libsvn_client/repos_diff.c
> ===================================================================
> --- subversion/libsvn_client/repos_diff.c (revision 39314)
> +++ subversion/libsvn_client/repos_diff.c (arbetskopia)
> @@ -51,6 +51,9 @@
> /* ADM_ACCESS is an access baton that includes the TARGET
> directory */
> svn_wc_adm_access_t *adm_access;
>
> + /* WC_CTX references the data of the wc */
> + svn_wc_context_t *wc_ctx;
> +
> /* The callback and calback argument that implement the file
> comparison
> function */
> const svn_wc_diff_callbacks4_t *diff_callbacks;
> @@ -839,7 +842,7 @@
> b->edit_baton->diff_cmd_baton));
> else
> SVN_ERR(eb->diff_callbacks->file_changed
> - (adm_access, &content_state, &prop_state, &b-
> >tree_conflicted,
> + (eb->wc_ctx, &content_state, &prop_state, &b-
> >tree_conflicted,
> b->wcpath,
> b->path_end_revision ? b->path_start_revision :
> NULL,
> b->path_end_revision,
> @@ -1127,6 +1130,7 @@
> svn_error_t *
> svn_client__get_diff_editor(const char *target,
> svn_wc_adm_access_t *adm_access,
> + svn_client_ctx_t *ctx,
> const svn_wc_diff_callbacks4_t
> *diff_callbacks,
> void *diff_cmd_baton,
> svn_depth_t depth,
> @@ -1147,6 +1151,7 @@
>
> eb->target = target;
> eb->adm_access = adm_access;
> + eb->wc_ctx = ctx->wc_ctx;
> eb->diff_callbacks = diff_callbacks;
> eb->diff_cmd_baton = diff_cmd_baton;
> eb->dry_run = dry_run;
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 39314)
> +++ subversion/libsvn_client/client.h (arbetskopia)
> @@ -677,6 +677,7 @@
> svn_error_t *
> svn_client__get_diff_editor(const char *target,
> svn_wc_adm_access_t *adm_access,
> + svn_client_ctx_t *ctx,
> const svn_wc_diff_callbacks4_t *diff_cmd,
> void *diff_cmd_baton,
> svn_depth_t depth,
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 39314)
> +++ subversion/libsvn_client/merge.c (arbetskopia)
> @@ -1217,7 +1217,7 @@
>
> /* An svn_wc_diff_callbacks4_t function. */
> static svn_error_t *
> -merge_file_changed(svn_wc_adm_access_t *adm_access,
> +merge_file_changed(svn_wc_context_t *wc_ctx,
> svn_wc_notify_state_t *content_state,
> svn_wc_notify_state_t *prop_state,
> svn_boolean_t *tree_conflicted,
> @@ -1236,10 +1236,17 @@
> apr_pool_t *subpool = svn_pool_create(merge_b->pool);
> svn_boolean_t merge_required = TRUE;
> enum svn_wc_merge_outcome_t merge_outcome;
> - const char *mine_abspath;
> + const char *mine_abspath, *adm_dir, *file_path;
Prefer not to declare multiple variables on the same line. Also, is
ADM_DIR absolute or relative? Perhaps call it ADM_ABSPATH (a growing
convention) or ADM_RELPATH? Same with FILE_PATH.
> + svn_wc_adm_access_t *adm_access;
> + svn_error_t *err;
>
> - SVN_ERR(svn_dirent_get_absolute(&mine_abspath, mine, merge_b-
> >pool));
> + SVN_ERR(svn_dirent_get_absolute(&mine_abspath, mine, subpool));
>
> + svn_dirent_split(mine_abspath, &adm_dir, &file_path, subpool);
> +
> + SVN_ERR(svn_wc__adm_retrieve_from_context(&adm_access, wc_ctx,
> adm_dir,
> + subpool));
Is is possible that the adm_access returned here could be NULL? Do we
have to check for that?
> +
> if (tree_conflicted)
> *tree_conflicted = FALSE;
>
> @@ -4572,7 +4579,8 @@
>
> /* Get the diff editor and a reporter with which to, ultimately,
> drive it. */
> - SVN_ERR(svn_client__get_diff_editor(target_wcpath, adm_access,
> callbacks,
> + SVN_ERR(svn_client__get_diff_editor(target_wcpath, adm_access,
> + merge_b->ctx, callbacks,
> merge_b, depth, merge_b-
> >dry_run,
> merge_b->ra_session2,
> revision1,
> notification_receiver,
> notify_b,
> @@ -6364,7 +6372,7 @@
> }
> else
> {
> - SVN_ERR(merge_file_changed(adm_access,
> + SVN_ERR(merge_file_changed(ctx->wc_ctx,
> &text_state, &prop_state,
> &tree_conflicted,
> target_wcpath,
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 39314)
> +++ subversion/libsvn_client/diff.c (arbetskopia)
> @@ -619,7 +619,7 @@
>
> /* An svn_wc_diff_callbacks4_t function. */
> static svn_error_t *
> -diff_file_changed(svn_wc_adm_access_t *adm_access,
> +diff_file_changed(svn_wc_context_t *wc_ctx,
> svn_wc_notify_state_t *content_state,
> svn_wc_notify_state_t *prop_state,
> svn_boolean_t *tree_conflicted,
> @@ -634,6 +634,9 @@
> apr_hash_t *original_props,
> void *diff_baton)
> {
> + /* Used temporarily in WC_NG development */
> + svn_wc_adm_access_t *adm_access = NULL;
Why create this variable? Just pass NULL where it is used.
> +
> if (tmpfile1)
> SVN_ERR(diff_content_changed(path,
> tmpfile1, tmpfile2, rev1, rev2,
> @@ -675,6 +678,7 @@
> void *diff_baton)
> {
> struct diff_cmd_baton *diff_cmd_baton = diff_baton;
> + svn_wc_context_t *wc_ctx;
>
> /* We want diff_file_changed to unconditionally show diffs, even if
> the diff is empty (as would be the case if an empty file were
> @@ -683,7 +687,14 @@
> user see that *something* happened. */
> diff_cmd_baton->force_empty = TRUE;
>
> - SVN_ERR(diff_file_changed(adm_access, content_state, prop_state,
> + /* Create wc_ctx object to use in WC-NG work. TODO: Remove when we
> + * have a wc_ctx parameter in the file_added callback. */
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL, /* config */
> + diff_cmd_baton->pool,
> + diff_cmd_baton->pool));
Why create a new wc_ctx instead of reuse the existing global one?
> +
> +
> + SVN_ERR(diff_file_changed(wc_ctx, content_state, prop_state,
> tree_conflicted, path,
> tmpfile1, tmpfile2,
> rev1, rev2,
> @@ -692,6 +703,8 @@
>
> diff_cmd_baton->force_empty = FALSE;
>
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
> +
> return SVN_NO_ERROR;
> }
>
> @@ -709,15 +722,26 @@
> void *diff_baton)
> {
> struct diff_cmd_baton *diff_cmd_baton = diff_baton;
> + svn_wc_context_t *wc_ctx;
>
> + /* Create wc_ctx object to use in WC-NG work. TODO: Remove when we
> + * have a wc_ctx parameter in the file_deleted_with_diff
> callback. */
> + SVN_ERR(svn_wc_context_create(&wc_ctx, NULL, /* config */
> + diff_cmd_baton->pool,
> + diff_cmd_baton->pool));
Same.
> +
> /* We don't list all the deleted properties. */
> - return diff_file_changed(adm_access, state, NULL,
> tree_conflicted, path,
> - tmpfile1, tmpfile2,
> - diff_cmd_baton->revnum1, diff_cmd_baton-
> >revnum2,
> - mimetype1, mimetype2,
> - apr_array_make(diff_cmd_baton->pool, 1,
> - sizeof(svn_prop_t)),
> - apr_hash_make(diff_cmd_baton->pool),
> diff_baton);
> + SVN_ERR(diff_file_changed(wc_ctx, state, NULL, tree_conflicted,
> path,
> + tmpfile1, tmpfile2,
> + diff_cmd_baton->revnum1, diff_cmd_baton-
> >revnum2,
> + mimetype1, mimetype2,
> + apr_array_make(diff_cmd_baton->pool, 1,
> + sizeof(svn_prop_t)),
> + apr_hash_make(diff_cmd_baton->pool),
> diff_baton));
> +
> + SVN_ERR(svn_wc_context_destroy(wc_ctx));
> +
> + return SVN_NO_ERROR;
> }
>
> /* An svn_wc_diff_callbacks4_t function. */
> @@ -1268,7 +1292,7 @@
> Otherwise, we just use "". */
> SVN_ERR(svn_client__get_diff_editor
> (drr.base_path ? drr.base_path : "",
> - NULL, callbacks, callback_baton, diff_param->depth,
> + NULL, ctx, callbacks, callback_baton, diff_param->depth,
> FALSE /* doesn't matter for diff */, extra_ra_session,
> drr.rev1,
> NULL /* no notify_func */, NULL /* no notify_baton */,
> ctx->cancel_func, ctx->cancel_baton,
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2394792
Received on 2009-09-14 23:35:45 CEST