Almost there. A few random thoughts.
On Sep 16, 2009, at 2:08 AM, Daniel Näslund wrote:
>
> Hyurum, here's my new patch. I've created the wc_ctx for the wrapper
> in
> deprecated.c from the anchor adm_access as you suggested in another
> mail.
>
>>> 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.)
>
> Good point! Since the wc_ctx is the same for all the callbacks I can
> create it in one place.
>
>>> Index: subversion/libsvn_wc/deprecated.c
>>> ===================================================================
>>> --- subversion/libsvn_wc/deprecated.c (revision 39314)
>>> +++ subversion/libsvn_wc/deprecated.c (arbetskopia)
>>> @@ -1291,11 +1292,21 @@
>>> void *diff_baton)
>>> {
>>> struct diff_callbacks3_wrapper_baton *b = diff_baton;
>>> + svn_wc_adm_access_t *adm_access;
>>>
>>> + 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()?
>
> I've created that local helper.
>
>> 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?
>
> Bert told me once that we usually create a recursive baton and since
> the
> file_changed() callback demands a write baton I used a write baton. If
> the docs says that it can be shallow then I'll make it shallow. The
> current code checks for a NULL baton and bails out if it finds one.
The 1.6 docs are actually quite nebulous, which is unfortunate.
>>> Index: subversion/libsvn_client/merge.c
>>> ===================================================================
>>> --- subversion/libsvn_client/merge.c (revision 39314)
>>> +++ subversion/libsvn_client/merge.c (arbetskopia)
>>> @@ -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.
>
> Changed. svn_dirent_split makes no promises for absolute paths so I
> call them relative.
Actually it does. From the docs:
Examples:
• "/foo/bar/baz" ==> "/foo/bar" and "baz"
>
>>> + svn_wc_adm_access_t *adm_access;
>> + 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?
>
> The current code checks for that further down.
>
> [[[
> As part of WC-NG work, remove adm_access batons in wc_diff_callback
> file_changed().
>
> * subversion/include/svn_wc.h
> (file_changed): Remove adm_access argument.
>
> * subversion/libsvn_wc_deprecated.c
> (diff_callbacks3_wrapper_baton): Add wc_ctx member.
> (make_dummy_adm_access): New.Local helper that incapsulates the
> creation of an adm_access to use in the wrapper functions.
> (wrap_4to3_file_changed): Remove adm_access argument. Internally get
> an adm_access baton by calling make_dummy_adm_access().
> (svn_wc_get_diff_editor5): Create a wc_ctx from anchor and save it in
> diff_callbacks3_wrapper_baton.
>
> * subversion/libsvn_wc/diff.c
> (file_diff, close_file): Remove adm_access argument when calling
> file_changed callback.
>
> * subversion/libsvn_client/repos_diff.c
> (close_file): Remove adm_access argument when calling file_changed
> callback.
>
> * subversion/libsvn_client/merge.c
> (merge_file_changed): Remove adm_access argument. Retrieve adm_access
> from wc_ctx.
> (do_file_merge): Remove adm_access argument when calling
> merge_file_changed().
>
> * subversion/libsvn_client/diff.c
> (diff_cmd_baton): Add wc_ctx member.
> (diff_file_changed): Remove adm_access argument. Call
> diff_props_changed() with a NULL baton. No need to fetch one
> since it's
> NULL in the caller and will be removed soon.
> (diff_file_added, diff_file_deleted_with_diff): Remove adm_access
> argument when calling diff_file_changed().
> (svn_client_diff4): Set wc_ctx member of diff_cmd_baton.
> ]]]
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 39343)
> +++ subversion/include/svn_wc.h (arbetskopia)
> @@ -1906,8 +1906,7 @@
> * property name.
> *
> */
> - svn_error_t *(*file_changed)(svn_wc_adm_access_t *adm_access,
> - svn_wc_notify_state_t *contentstate,
> + svn_error_t *(*file_changed)(svn_wc_notify_state_t *contentstate,
> svn_wc_notify_state_t *propstate,
> svn_boolean_t *tree_conflicted,
> const char *path,
In the spirit of this transition, this path should be documented and
guaranteed to be absolute. And probably renamed to local_abspath
(both here, and in the various implementors).
> Index: subversion/libsvn_wc/deprecated.c
> ===================================================================
> --- subversion/libsvn_wc/deprecated.c (revision 39343)
> +++ 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
> @@ -909,6 +910,21 @@
>
>
> /*** From diff.c ***/
> +
> +/* Local helper for the wc_diff_callback wrappers. */
> +static svn_error_t *make_dummy_adm_access(svn_wc_adm_access_t
> **adm_access,
Return type on separate line.
> + svn_wc_context_t *wc_ctx,
> + const char *path)
> +{
> + svn_error_return(svn_wc__adm_open_in_context(adm_access,
> wc_ctx, path,
> + FALSE, /*
> write_lock */
> + 0, /*
> levels_to_lock */
> + NULL, /*
> svn_cancel_func_t */
> + NULL, /*
> cancel_baton */
> + wc_ctx-
> >state_pool));
> +
> +}
> +
> /* Used to wrap svn_wc_diff_callbacks_t. */
> struct diff_callbacks_wrapper_baton {
> const svn_wc_diff_callbacks_t *callbacks;
> @@ -1269,14 +1285,14 @@
> /* Used to wrap svn_wc_diff_callbacks3_t. */
> struct diff_callbacks3_wrapper_baton {
> const svn_wc_diff_callbacks3_t *callbacks3;
> + svn_wc_context_t *wc_ctx;
> void *baton;
> };
>
> /* 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,
> - svn_wc_notify_state_t *contentstate,
> +wrap_4to3_file_changed(svn_wc_notify_state_t *contentstate,
> svn_wc_notify_state_t *propstate,
> svn_boolean_t *tree_conflicted,
> const char *path,
> @@ -1291,11 +1307,16 @@
> 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(make_dummy_adm_access(&adm_access, b->wc_ctx, path));
> +
> + 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, b->wc_ctx-
> >state_pool));
> }
>
> /* An svn_wc_diff_callbacks4_t function for wrapping
> @@ -1459,9 +1480,18 @@
> void **edit_baton,
> apr_pool_t *pool)
> {
> + svn_wc__db_t *wc_db;
> + svn_wc_context_t *wc_ctx;
Any need for these temporary variables? This is more just personal
preference here, but you could do
SVN_ERR(svn_wc__context_create_with_db(&b->wc_ctx, NULL /* config */,
svn_wc__adm_get_db(anchor),
pool));
> struct diff_callbacks3_wrapper_baton *b = apr_palloc(pool, sizeof
> (*b));
> b->callbacks3 = callbacks;
> b->baton = callback_baton;
> +
> + wc_db = svn_wc__adm_get_db(anchor);
> +
> + SVN_ERR(svn_wc__context_create_with_db(&wc_ctx, NULL /* config */,
> + wc_db, pool));
> + b->wc_ctx = wc_ctx;
instead of this whole dance.
> +
> return svn_wc_get_diff_editor6(anchor,
> target,
> &diff_callbacks3_wrapper,
> Index: subversion/libsvn_wc/diff.c
> ===================================================================
> --- subversion/libsvn_wc/diff.c (revision 39343)
> +++ subversion/libsvn_wc/diff.c (arbetskopia)
> @@ -665,7 +665,7 @@
> eb->db, pool, pool));
>
> SVN_ERR(dir_baton->edit_baton->callbacks->file_changed
> - (NULL, NULL, NULL, NULL,
> + (NULL, NULL, NULL,
> path,
When we guarantee absolute paths, this will need to be absolutized.
> modified ? textbase : NULL,
> translated,
> @@ -1580,7 +1580,7 @@
> reverse_propchanges(originalprops, b->propchanges, b->pool);
>
> SVN_ERR(b->edit_baton->callbacks->file_changed
> - (NULL, NULL, NULL, NULL,
> + (NULL, NULL, NULL,
> b->path,
Same.
> eb->reverse_order ? localfile : temp_file_path,
> eb->reverse_order ? temp_file_path : localfile,
> Index: subversion/libsvn_client/repos_diff.c
> ===================================================================
> --- subversion/libsvn_client/repos_diff.c (revision 39343)
> +++ subversion/libsvn_client/repos_diff.c (arbetskopia)
> @@ -839,7 +839,7 @@
> b->edit_baton->diff_cmd_baton));
> else
> SVN_ERR(eb->diff_callbacks->file_changed
> - (adm_access, &content_state, &prop_state, &b-
> >tree_conflicted,
> + (&content_state, &prop_state, &b->tree_conflicted,
> b->wcpath,
Same.
> b->path_end_revision ? b->path_start_revision :
> NULL,
> b->path_end_revision,
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 39343)
> +++ subversion/libsvn_client/merge.c (arbetskopia)
> @@ -1217,8 +1217,7 @@
>
> /* An svn_wc_diff_callbacks4_t function. */
> static svn_error_t *
> -merge_file_changed(svn_wc_adm_access_t *adm_access,
> - svn_wc_notify_state_t *content_state,
> +merge_file_changed(svn_wc_notify_state_t *content_state,
> svn_wc_notify_state_t *prop_state,
> svn_boolean_t *tree_conflicted,
> const char *mine,
> @@ -1237,9 +1236,18 @@
> svn_boolean_t merge_required = TRUE;
> enum svn_wc_merge_outcome_t merge_outcome;
> const char *mine_abspath;
> + const char *adm_relpath;
> + const char *file_relpath;
> + 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_relpath, &file_relpath,
> subpool);
svn_dirent_split() can handle absolute paths just fine, so adm_relpath
will actually be an absolute path (and should be renamed as such).
> +
> + SVN_ERR(svn_wc__adm_retrieve_from_context(&adm_access, merge_b-
> >ctx->wc_ctx,
> + adm_relpath, subpool));
Given the above, I don't know the ramifications of calling this
function with an absolute path. But given the passing tests, it
appears to work.
> +
> if (tree_conflicted)
> *tree_conflicted = FALSE;
>
> @@ -4568,8 +4576,9 @@
>
> /* 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,
> - merge_b, depth, merge_b-
> >dry_run,
> + SVN_ERR(svn_client__get_diff_editor(target_wcpath, adm_access,
> + callbacks, merge_b, depth,
> + merge_b->dry_run,
> merge_b->ra_session2,
> revision1,
> notification_receiver,
> notify_b,
> merge_b->ctx->cancel_func,
> @@ -6360,8 +6369,7 @@
> }
> else
> {
> - SVN_ERR(merge_file_changed(adm_access,
> - &text_state, &prop_state,
> + SVN_ERR(merge_file_changed(&text_state, &prop_state,
> &tree_conflicted,
> target_wcpath,
> tmpfile1,
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 39343)
> +++ subversion/libsvn_client/diff.c (arbetskopia)
> @@ -365,6 +365,8 @@
> /* The directory that diff target paths should be considered as
> relative to for output generation (see issue #2723). */
> const char *relative_to_dir;
> +
> + svn_wc_context_t *wc_ctx;
> };
>
> /* Generate a label for the diff output for file PATH at revision
> REVNUM.
> @@ -620,8 +622,7 @@
>
> /* An svn_wc_diff_callbacks4_t function. */
> static svn_error_t *
> -diff_file_changed(svn_wc_adm_access_t *adm_access,
> - svn_wc_notify_state_t *content_state,
> +diff_file_changed(svn_wc_notify_state_t *content_state,
> svn_wc_notify_state_t *prop_state,
> svn_boolean_t *tree_conflicted,
> const char *path,
> @@ -640,7 +641,7 @@
> tmpfile1, tmpfile2, rev1, rev2,
> mimetype1, mimetype2, diff_baton));
> if (prop_changes->nelts > 0)
> - SVN_ERR(diff_props_changed(adm_access, prop_state,
> tree_conflicted,
> + SVN_ERR(diff_props_changed(NULL, prop_state, tree_conflicted,
> path, prop_changes,
> original_props, diff_baton));
> if (content_state)
> @@ -684,7 +685,7 @@
> user see that *something* happened. */
> diff_cmd_baton->force_empty = TRUE;
>
> - SVN_ERR(diff_file_changed(adm_access, content_state, prop_state,
> + SVN_ERR(diff_file_changed(content_state, prop_state,
> tree_conflicted, path,
> tmpfile1, tmpfile2,
> rev1, rev2,
> @@ -710,15 +711,20 @@
> void *diff_baton)
> {
> struct diff_cmd_baton *diff_cmd_baton = diff_baton;
> + svn_wc_context_t *wc_ctx;
>
> /* 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_error_return(diff_file_changed(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));
> +
> + return SVN_NO_ERROR;
This should be 'return svn_error_return(...)'.
> }
>
> /* An svn_wc_diff_callbacks4_t function. */
> @@ -1715,6 +1721,7 @@
> diff_cmd_baton.force_empty = FALSE;
> diff_cmd_baton.force_binary = ignore_content_type;
> diff_cmd_baton.relative_to_dir = relative_to_dir;
> + diff_cmd_baton.wc_ctx = ctx->wc_ctx;
>
> return do_diff(&diff_params, &diff_callbacks, &diff_cmd_baton,
> ctx, pool);
> }
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2395667
Received on 2009-09-16 19:53:33 CEST