Dave,
thanks for the patch. Some comments below.
On Fri, Sep 25, 2009 at 11:22:25AM -0700, Dave Brown wrote:
> The added/deleted accessors are not perfect, they don't go through
> all the logic that entries.c does to populate entry->schedule. I
> kept wanting to look at svn_wc__de_status_t values. But the accessors
> suffice for export.c use.
I think the "skip export" logic is too simplistic. E.g. it does not
consider schedule-replace nodes. However this problem has existed
before your patch, and can be fixed in another patch.
We should list all possible states a node can be in and define skipping
behaviour for each case, then update your skip_export_node() function.
> [[[
> Remove wc_entry_t usage from export.c. Make a couple of accessor api's
> in svn_wc_private.h to fetch from wc_db the equivalent of the old test
> entry->schedule == (schdule_add,schedule_delete).
s/schdule/schedule/
> * include/private/svn_wc_private.h:
> (svn_wc__node_is_delete, svn_wc__node_is_added): New functions
>
> * libsvn_wc/node.c:
> (svn_wc__node_is_delete, svn_wc__node_is_added): New functions
>
> * libsvn_client/export.c:
> (skip_export_node): add static helper function to decide if a node
> should be exported or not
You can indent additional lines in each (...) entry by one or two spaces.
> (copy_one_versioned_file,copy_versioned_files): Remove wc_entry_t
> usage. Use svn_wc_private.h api's to fetch specific information:
> url,depth,added or deleted status.
> ]]]
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2400350
> Index: subversion/include/private/svn_wc_private.h
> ===================================================================
> --- subversion/include/private/svn_wc_private.h (revision 39608)
> +++ subversion/include/private/svn_wc_private.h (working copy)
> @@ -434,6 +434,17 @@
> void *cancel_baton,
> apr_pool_t *scratch_pool);
>
> +/** Equivalent to the old notion of "entry->schedule == schedule_delete" */
> +svn_error_t *
> +svn_wc__node_is_delete(svn_boolean_t *deleted, svn_wc_context_t *wc_ctx,
> + const char *abspath, apr_pool_t *scratch_pool);
This should be called "svn_wc__node_is_deleted" (past tense).
There is a function with that name in entries.c already.
The existing function expects a wc_db rather than a wc_ctx.
and has a comment saying that it wants to be updated to wc_db.
Looks like your new function is almost a drop-in replacement.
If we cannot remove the existing function because it has too many users
internal to libsvn_wc relying on the wc_db parameter, we should update
the existing function with your code and provide a thin wrapper that takes
a wc_ctx for use by libsvn_client, with a different name (but more different
than just a single character).
> +
> +/** An imperfect equivalent to the old notion of "entry->schedule == schedule_add" */
> +svn_error_t *
> +svn_wc__node_is_added(svn_boolean_t *added, svn_wc_context_t *wc_ctx,
> + const char *abspath, apr_pool_t *scratch_pool);
> +
For consistency, the implementation of this function could then also live
in entries.c, with a wc_db argument, and should also have a thin wrapper
for libsvn_client. Or it can be kept as-is if we can delete the old
svn_wc__node_is_deleted() variant.
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/libsvn_wc/node.c
> ===================================================================
> --- subversion/libsvn_wc/node.c (revision 39608)
> +++ subversion/libsvn_wc/node.c (working copy)
> @@ -133,7 +133,49 @@
> return err;
> }
>
> +/* ### for is_added/is_delete() how to handle:
> + ### svn_wc__db_status_(absent|excluded|not_present|incomplete?
I don't think any of these states should be considered 'deleted'.
> +*/
> +/* Equivalent to the old notion of "entry->schedule == schedule_delete" */
> svn_error_t *
> +svn_wc__node_is_delete(svn_boolean_t *deleted, svn_wc_context_t *wc_ctx,
> + const char *abspath, apr_pool_t *scratch_pool)
> +{
> + svn_wc__db_status_t status;
> +
> + SVN_ERR(svn_wc__db_read_info(&status, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL,
> + wc_ctx->db, abspath, scratch_pool,
> + scratch_pool));
> +
> + *deleted = (status == svn_wc__db_status_deleted
> + || status == svn_wc__db_status_obstructed_delete);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Equivalent to the old notion of "entry->schedule == schedule_add" */
> +svn_error_t *
> +svn_wc__node_is_added(svn_boolean_t *added, svn_wc_context_t *wc_ctx,
> + const char *abspath, apr_pool_t *scratch_pool)
> +{
> + svn_wc__db_status_t status;
> +
> + SVN_ERR(svn_wc__db_read_info(&status, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL,
> + wc_ctx->db, abspath, scratch_pool,
> + scratch_pool));
> +
> + *added = (status == svn_wc__db_status_added
> + || status == svn_wc__db_status_obstructed_add);
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> svn_wc__node_get_kind(svn_node_kind_t *kind,
> svn_wc_context_t *wc_ctx,
> const char *abspath,
> Index: subversion/libsvn_client/export.c
> ===================================================================
> --- subversion/libsvn_client/export.c (revision 39608)
> +++ subversion/libsvn_client/export.c (working copy)
> @@ -95,7 +95,30 @@
> return SVN_NO_ERROR;
> }
>
> +/* Helper function to determine if a node should be exported at
> + a given revision.
> +
> + Only export 'added' files when the revision is WORKING.
> + Otherwise, skip the 'added' files, since they didn't exist
> + in the BASE revision and don't have an associated text-base.
> +
> + Don't export 'deleted' files and directories unless it's a
> + revision other than WORKING. These files and directories
> + don't really exist in WORKING. */
> static svn_error_t *
> +skip_export_node(svn_boolean_t *skip, const char *abspath,
> + svn_wc_context_t *wc_ctx, const svn_opt_revision_t *revision,
> + apr_pool_t *scratch_pool)
> +{
> + if (revision->kind != svn_opt_revision_working)
> + SVN_ERR(svn_wc__node_is_added(skip, wc_ctx, abspath, scratch_pool));
> + else
> + SVN_ERR(svn_wc__node_is_delete(skip, wc_ctx, abspath, scratch_pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> copy_one_versioned_file(const char *from_abspath,
> const char *to_abspath,
> svn_wc_context_t *wc_ctx,
> @@ -103,7 +126,6 @@
> const char *native_eol,
> apr_pool_t *scratch_pool)
> {
> - const svn_wc_entry_t *entry;
> apr_hash_t *kw = NULL;
> svn_subst_eol_style_t style;
> apr_hash_t *props;
> @@ -115,31 +137,12 @@
> svn_stream_t *dst_stream;
> const char *dst_tmp;
> svn_error_t *err;
> + svn_boolean_t skip_export;
>
> - err = svn_wc__get_entry_versioned(&entry, wc_ctx, from_abspath,
> - svn_node_file, FALSE, FALSE,
> - scratch_pool, scratch_pool);
> - if (err && err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
> - {
> - svn_error_clear(err);
> - entry = NULL;
> - }
> - else if (err)
> - return svn_error_return(err);
> -
> - /* Only export 'added' files when the revision is WORKING.
> - Otherwise, skip the 'added' files, since they didn't exist
> - in the BASE revision and don't have an associated text-base.
> -
> - Don't export 'deleted' files and directories unless it's a
> - revision other than WORKING. These files and directories
> - don't really exist in WORKING. */
> - if ((revision->kind != svn_opt_revision_working &&
> - entry->schedule == svn_wc_schedule_add) ||
> - (revision->kind == svn_opt_revision_working &&
> - entry->schedule == svn_wc_schedule_delete))
> + SVN_ERR(skip_export_node(&skip_export, from_abspath, wc_ctx, revision,
> + scratch_pool));
> + if (skip_export)
> return SVN_NO_ERROR;
> -
Please keep the above empty line.
> if (revision->kind != svn_opt_revision_working)
> {
> SVN_ERR(svn_wc_get_pristine_contents(&source, from_abspath,
> @@ -206,10 +209,13 @@
> svn_revnum_t changed_rev;
> const char *fmt;
> const char *author;
> + const char *url;
>
> SVN_ERR(svn_wc__node_get_changed_info(&changed_rev, NULL, &author,
> wc_ctx, from_abspath, scratch_pool,
> scratch_pool));
> + SVN_ERR(svn_wc__node_get_url(&url, wc_ctx, from_abspath, scratch_pool,
> + scratch_pool));
>
> if (local_mod)
> {
> @@ -228,7 +234,7 @@
> SVN_ERR(svn_subst_build_keywords2
> (&kw, keywords->data,
> apr_psprintf(scratch_pool, fmt, changed_rev),
> - entry->url, tm, author, scratch_pool));
> + url, tm, author, scratch_pool));
> }
>
> /* For atomicity, we translate to a tmp file and then rename the tmp file
> @@ -276,40 +282,28 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> - const svn_wc_entry_t *entry;
> svn_error_t *err;
> apr_pool_t *iterpool;
> const apr_array_header_t *children;
> const char *from_abspath;
> const char *to_abspath;
> svn_node_kind_t from_kind;
> + svn_boolean_t skip_export;
> int j;
>
> SVN_ERR(svn_dirent_get_absolute(&from_abspath, from, pool));
> SVN_ERR(svn_dirent_get_absolute(&to_abspath, to, pool));
>
> - SVN_ERR(svn_wc__get_entry_versioned(&entry, ctx->wc_ctx, from_abspath,
> - svn_node_unknown, FALSE, FALSE,
> - pool, pool));
> -
> - /* Only export 'added' files when the revision is WORKING.
> - Otherwise, skip the 'added' files, since they didn't exist
> - in the BASE revision and don't have an associated text-base.
> -
> - Don't export 'deleted' files and directories unless it's a
> - revision other than WORKING. These files and directories
> - don't really exist in WORKING. */
> - if ((revision->kind != svn_opt_revision_working &&
> - entry->schedule == svn_wc_schedule_add) ||
> - (revision->kind == svn_opt_revision_working &&
> - entry->schedule == svn_wc_schedule_delete))
> + SVN_ERR(skip_export_node(&skip_export, from_abspath, ctx->wc_ctx, revision,
> + pool));
Indentation is off here.
Rest looks fine.
Thanks,
Stefan
> + if (skip_export)
> return SVN_NO_ERROR;
>
> SVN_ERR(svn_wc__node_get_kind(&from_kind, ctx->wc_ctx, from_abspath,
> FALSE, pool));
> -
> if (from_kind == svn_node_dir)
> {
> + svn_depth_t from_depth;
> /* Try to make the new directory. If this fails because the
> directory already exists, check our FORCE flag to see if we
> care. */
> @@ -335,6 +329,8 @@
> else
> svn_error_clear(err);
> }
> + SVN_ERR(svn_wc__node_get_depth(&from_depth, ctx->wc_ctx, from_abspath,
> + pool));
>
> SVN_ERR(svn_wc__node_get_children(&children, ctx->wc_ctx, from_abspath,
> FALSE, pool, pool));
> @@ -397,7 +393,7 @@
>
> /* Handle externals. */
> if (! ignore_externals && depth == svn_depth_infinity
> - && entry->depth == svn_depth_infinity)
> + && from_depth == svn_depth_infinity)
> {
> apr_array_header_t *ext_items;
> const svn_string_t *prop_val;
--
printf("Eh???/n");
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2401607
Received on 2009-09-29 14:58:13 CEST