firemeteor_at_tigris.org writes:
> Log:
> In issue-2843-dev branch.
>
> Generate notifications in the cropping logic. And actually invoke the
> cropping logic in the update logic. Preliminary debug has made the
> cropping logic work to some degree. However, many tests fail
> unexpectedly. Further debugging is needed.
>
> * subversion/libsvn_wc/crop.c
> (svn_wc_crop_tree, crop_children): Generate notifications. Some
> adm_access related tunning to make it work. And some other bugs fixed.
>
> * subversion/libsvn_client/update.c
> (svn_client__update_internal): Lock enough depth and call
> svn_wc_crop_tree().
>
> [...]
>
> --- branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31568)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31569)
> @@ -28,11 +28,14 @@
>
> #define SVN_ERR_IGNORE_LOCAL_MOD(expr) \
> do { \
> - svn_error_t *svn_err__temp = (expr); \
> - if (svn_err__temp->apr_err == SVN_ERR_WC_LEFT_LOCAL_MOD) \
> - svn_error_clear(svn_err__temp); \
> - else if (svn_err__temp) \
> - return svn_err__temp; \
> + svn_error_t *__temp = (expr); \
> + if (__temp) \
> + { \
> + if (__temp->apr_err == SVN_ERR_WC_LEFT_LOCAL_MOD) \
> + svn_error_clear(__temp); \
> + else \
> + return __temp; \
> + } \
> } while (0)
Log message should mention that SVN_ERR_IGNORE_LOCAL_MOD was changed.
In general, a log message should mention every top-level symbol that was
affected.
Reminder: in a previous review (http://tinyurl.com/48r6u9) I gave
reasons why this macro should have a different name. Anyway, I fixed it
now, see r31731 (I also documented the macro).
> /* Helper function that crops the children of the TARGET, under the constraint
> @@ -43,6 +46,8 @@ svn_error_t *
> crop_children(svn_wc_adm_access_t *adm_access,
> const char *dir_path,
> svn_depth_t depth,
> + svn_wc_notify_func2_t notify_func,
> + void *notify_baton,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> apr_pool_t *pool)
I see that crop_children() has been documented more after this commit,
but currently (as of r31731), its documentation is still out-of-date
(for example, the current doc string mentions "ROOT" but there is no
'root' parameter).
> @@ -50,12 +55,9 @@ crop_children(svn_wc_adm_access_t *adm_a
> apr_hash_t *entries;
> apr_hash_index_t *hi;
> svn_wc_adm_access_t *dir_access;
> -// const char *full_path;
> svn_wc_entry_t *dot_entry;
> apr_pool_t *subpool = svn_pool_create(pool), *iterpool;
>
> -// full_path = svn_path_join(svn_wc_adm_access_path(adm_access),
> -// dir_path, subpool);
> SVN_ERR(svn_wc_adm_retrieve(&dir_access, adm_access, dir_path, subpool));
> SVN_ERR(svn_wc_entries_read(&entries, dir_access, TRUE, subpool));
> dot_entry = apr_hash_get(entries, SVN_WC_ENTRY_THIS_DIR,
> @@ -76,6 +78,7 @@ crop_children(svn_wc_adm_access_t *adm_a
> for (hi = apr_hash_first(subpool, entries); hi; hi = apr_hash_next(hi))
> {
> const void *key;
> + const char *this_path;
> void *val;
> apr_ssize_t klen;
> svn_wc_entry_t *current_entry;
> @@ -87,6 +90,7 @@ crop_children(svn_wc_adm_access_t *adm_a
> continue;
>
> current_entry = val;
> + this_path = svn_path_join(dir_path, current_entry->name, iterpool);
>
> if (current_entry->kind == svn_node_file && depth == svn_depth_empty)
> {
> @@ -99,31 +103,52 @@ crop_children(svn_wc_adm_access_t *adm_a
> cancel_baton,
> iterpool));
>
> + if (notify_func)
> + {
> + svn_wc_notify_t *notify;
> + notify = svn_wc_create_notify(this_path,
> + svn_wc_notify_delete,
> + iterpool);
> + (*notify_func)(notify_baton, notify, iterpool);
> + }
> }
> - /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> - svn_node_dir*/
> - else if (depth < svn_depth_immediates)
> + else if (current_entry->kind == svn_node_dir)
> {
> - svn_wc_adm_access_t *child_access;
> - SVN_ERR(svn_wc_adm_retrieve(&child_access, dir_access,
> - current_entry->name, iterpool));
> -
> - SVN_ERR_IGNORE_LOCAL_MOD
> - (svn_wc_remove_from_revision_control(child_access,
> - SVN_WC_ENTRY_THIS_DIR,
> - TRUE, /* destroy */
> - FALSE, /* instant error */
> - cancel_func,
> - cancel_baton,
> - iterpool));
> + if (depth < svn_depth_immediates)
> + {
> + svn_wc_adm_access_t *child_access;
> + SVN_ERR(svn_wc_adm_retrieve(&child_access, dir_access,
> + this_path, iterpool));
> +
> + SVN_ERR_IGNORE_LOCAL_MOD
> + (svn_wc_remove_from_revision_control(child_access,
> + SVN_WC_ENTRY_THIS_DIR,
> + TRUE, /* destroy */
> + FALSE, /* instant error */
> + cancel_func,
> + cancel_baton,
> + iterpool));
> + if (notify_func)
> + {
> + svn_wc_notify_t *notify;
> + notify = svn_wc_create_notify(this_path,
> + svn_wc_notify_delete,
> + iterpool);
> + (*notify_func)(notify_baton, notify, iterpool);
> + }
> + }
> + else
> + return crop_children(dir_access,
> + this_path,
> + svn_depth_empty,
> + notify_func,
> + notify_baton,
> + cancel_func,
> + cancel_baton,
> + iterpool);
> }
> - else
> - return crop_children(dir_access,
> - current_entry->name,
> - svn_depth_empty,
> - cancel_func,
> - cancel_baton,
> - iterpool);
> + /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> + svn_node_dir*/
> }
I still don't quite understand the last sentence in this comment (even
though the comment has moved :-) ). The code doesn't "assume" dir right
now -- it checks for dir explicitly.
Is it okay if I just remove the last sentence there, or is there
something important I'm not understanding?
> svn_pool_destroy(subpool);
> @@ -142,6 +167,8 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> apr_pool_t *pool)
> {
> const svn_wc_entry_t *entry;
> + const char *full_path;
> + svn_wc_adm_access_t *dir_access;
>
> /* Only make sense to when the depth is restrictive.
> Currently does not support svn_depth_exclude*/
> @@ -149,8 +176,9 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> return SVN_NO_ERROR;
>
> /* Only make sense to crop a dir target*/
> - SVN_ERR(svn_wc_entry(&entry, target, anchor, FALSE, pool));
> - if (entry->kind != svn_node_dir)
> + full_path = svn_path_join(svn_wc_adm_access_path(anchor), target, pool);
> + SVN_ERR(svn_wc_entry(&entry, full_path, anchor, FALSE, pool));
> + if (!entry || entry->kind != svn_node_dir)
> return SVN_NO_ERROR;
So if the entry doesn't exist, we will just return SVN_NO_ERROR. That's
okay, but then the public doc string for svn_wc_crop_tree() should say so.
Rest looks good. I had a few other concerns, but I see that they have
already been addressed in the current code, so I'll go look at your
later commits now.
By the way: is your tigris.org username something different from
"firemeteor"? We should try to make your tigris.org username and your
committer username be the same (it helps with mail moderation and other
things).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-13 19:44:21 CEST