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.
> > 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.
> > + 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.
]]]
/Daniel Näslund
Received on 2009-09-16 09:09:18 CEST