RE: svn commit: r980784 - in /subversion/trunk/subversion/libsvn_wc: adm_files.c node.c wc_db.h

> -----Original Message-----
> From: philip_at_apache.org [mailto:philip_at_apache.org]
> Sent: vrijdag 30 juli 2010 15:29
> To: commits_at_subversion.apache.org
> Subject: svn commit: r980784 - in
> /subversion/trunk/subversion/libsvn_wc: adm_files.c node.c wc_db.h
>
> Author: philip
> Date: Fri Jul 30 13:28:39 2010
> New Revision: 980784
>
> URL: http://svn.apache.org/viewvc?rev=980784&view=rev
> Log:
> Remove an svn_wc_entry_t.

This breaks diff_tests.py 13 on Windows, although I don't know why yet. And the error code message looks like we might have a different problem somewhere else:
See http://ci.apache.org/builders/svn-slik-w2k3-x64-local/builds/975/steps/Test%20fsfs%2Blocal/logs/faillog

(some inline review below)

>
> * subversion/libsvn_wc/adm_files.c
> (svn_wc__internal_ensure_adm): Remove svn_wc_entry_t, use wc-ng
> calls.
>
> * subversion/libsvn_wc/wc_db.h
> (svn_wc__internal_get_copyfrom_info): New.
>
> * subversion/libsvn_wc/node.c
> (svn_wc__internal_get_copyfrom_info): Renamed from
> svn_wc__node_get_copyfrom_info, adjust recursive call.
> (svn_wc__node_get_copyfrom_info): Now just a wrapper.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/adm_files.c
> subversion/trunk/subversion/libsvn_wc/node.c
> subversion/trunk/subversion/libsvn_wc/wc_db.h
>
> Modified: subversion/trunk/subversion/libsvn_wc/adm_files.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm_
> files.c?rev=980784&r1=980783&r2=980784&view=diff
> =======================================================================
> =======
> --- subversion/trunk/subversion/libsvn_wc/adm_files.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/adm_files.c Fri Jul 30
> 13:28:39 2010
> @@ -597,9 +597,11 @@ svn_wc__internal_ensure_adm(svn_wc__db_t
> svn_depth_t depth,
> apr_pool_t *scratch_pool)
> {
> - const svn_wc_entry_t *entry;
> int format;
> const char *repos_relpath;
> + svn_wc__db_status_t status;
> + const char *db_repos_relpath, *db_repos_root_url, *db_repos_uuid;
> + svn_revnum_t db_revision;
>
> SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
> SVN_ERR_ASSERT(url != NULL);
> @@ -622,40 +624,59 @@ svn_wc__internal_ensure_adm(svn_wc__db_t
> repos_relpath, repos_root_url,
> repos_uuid,
> revision, depth, scratch_pool));
>
> - /* Now, get the existing url and repos for PATH. */
> - SVN_ERR(svn_wc__get_entry(&entry, db, local_abspath, FALSE,
> svn_node_unknown,
> - FALSE, scratch_pool, scratch_pool));
> + SVN_ERR(svn_wc__db_read_info(&status, NULL,
> + &db_revision, &db_repos_relpath,
> + &db_repos_root_url, &db_repos_uuid,
> + NULL, NULL, NULL, NULL, NULL, NULL,
> NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL,
> NULL,
> + NULL, NULL, NULL, NULL,
> + db, local_abspath, scratch_pool,
> scratch_pool));

This only sets db_revision, db_repos_relpath, etc. if the node doesn't have a working node... And then only when the node doesn't inherit the location from its parent. In all other cases the values will have their default values. (svn_wc__db_scan_addition() (status=added || !have_base) && svn_wc__db_base_scan_repos() (have_base)) and will handle the other cases). Looking at the rest of the function it might be useful to immediately check for original_* here as you need those for the copyfrom data.

The svn_wc__get_entry() call always retrieved the inherited values for you using these helpers, so this changes more than a few cases.
>
> - /* When the directory exists and is scheduled for deletion do not
> - * check the revision or the URL. The revision can be any
> + /* When the directory exists and is scheduled for deletion or is
> not-present
> + * do not check the revision or the URL. The revision can be any
> * arbitrary revision and the URL may differ if the add is
> * being driven from a merge which will have a different URL. */
> - if (entry->schedule != svn_wc_schedule_delete)
> + if (status != svn_wc__db_status_deleted
> + && status != svn_wc__db_status_obstructed_delete)
> {
> - if (entry->revision != revision)
> + /* ### Should we match copyfrom_revision? */
> + if (db_revision != revision)
> return
> svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> _("Revision %ld doesn't match existing "
> "revision %ld in '%s'"),
> - revision, entry->revision, local_abspath);
> + revision, db_revision, local_abspath);

This needs a svn_dirent_local_style() around local_abspath. (Not a new issue, but needs fixing anyway).

>
> /* The caller gives us a URL which should match the entry.
> However,
> some callers compensate for an old problem in entry->url and
> pass
> the copyfrom_url instead. See ^/notes/api-errata/wc002.txt.
> As
> a result, we allow the passed URL to match copyfrom_url if it
> - does match the entry's primary URL. */
> - /** ### comparing URLs, should they be canonicalized first? */
> - if (strcmp(entry->url, url) != 0
> - && (entry->copyfrom_url == NULL
> - || strcmp(entry->copyfrom_url, url) != 0)
> - && (!svn_uri_is_ancestor(repos_root_url, entry->url)
> - || strcmp(entry->uuid, repos_uuid) != 0))
> + does not match the entry's primary URL. */
> + /* ### comparing URLs, should they be canonicalized first? */
> + if (strcmp(db_repos_uuid, repos_uuid)
> + || strcmp(db_repos_root_url, repos_root_url)
> + || !svn_relpath_is_ancestor(db_repos_relpath,
> repos_relpath))
> {
> - return
> - svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> - _("URL '%s' doesn't match existing "
> - "URL '%s' in '%s'"),
> - url, entry->url, local_abspath);
> + const char *copyfrom_root_url, *copyfrom_repos_relpath;
> +
> +
> SVN_ERR(svn_wc__internal_get_copyfrom_info(&copyfrom_root_url,
> +
> &copyfrom_repos_relpath,
> + NULL, NULL, NULL,
> + db,
> local_abspath,
> + scratch_pool,
> + scratch_pool));
> +
> + if (copyfrom_root_url == NULL
> + || strcmp(copyfrom_root_url, repos_root_url)
> + || strcmp(copyfrom_repos_relpath, repos_relpath))
> + return
> + svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> + _("URL '%s' doesn't match existing "
> + "URL '%s' in '%s'"),
> + url,
> + svn_uri_join(db_repos_root_url,
> + db_repos_relpath,
> scratch_pool),
> + local_abspath);

You can't construct a url by just calling svn_uri_join(url, repos_relpath), as that doesn't handle escaping the relpath. Please use svn_path_url_add_component2() instead.

> }
> }
>
>
> Modified: subversion/trunk/subversion/libsvn_wc/node.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/node
> .c?rev=980784&r1=980783&r2=980784&view=diff
> =======================================================================
> =======
> --- subversion/trunk/subversion/libsvn_wc/node.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/node.c Fri Jul 30 13:28:39
> 2010
> @@ -467,17 +467,16 @@ svn_wc__node_get_repos_relpath(const cha
> }
>
> svn_error_t *
> -svn_wc__node_get_copyfrom_info(const char **copyfrom_root_url,
> - const char **copyfrom_repos_relpath,
> - const char **copyfrom_url,
> - svn_revnum_t *copyfrom_rev,
> - svn_boolean_t *is_copy_target,
> - svn_wc_context_t *wc_ctx,
> - const char *local_abspath,
> - apr_pool_t *result_pool,
> - apr_pool_t *scratch_pool)
> +svn_wc__internal_get_copyfrom_info(const char **copyfrom_root_url,
> + const char
> **copyfrom_repos_relpath,
> + const char **copyfrom_url,
> + svn_revnum_t *copyfrom_rev,
> + svn_boolean_t *is_copy_target,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> {
> - svn_wc__db_t *db = wc_ctx->db;
> const char *original_root_url;
> const char *original_repos_relpath;
> svn_revnum_t original_revision;
> @@ -544,11 +543,12 @@ svn_wc__node_get_copyfrom_info(const cha
>
> /* This is a copied node, so we should never fall off the
> top of a
> * working copy here. */
> - SVN_ERR(svn_wc__node_get_copyfrom_info(NULL, NULL,
> - &parent_copyfrom_url,
> - NULL, NULL,
> - wc_ctx,
> parent_abspath,
> - scratch_pool,
> scratch_pool));
> + SVN_ERR(svn_wc__internal_get_copyfrom_info(NULL, NULL,
> +
> &parent_copyfrom_url,
> + NULL, NULL,
> + db,
> parent_abspath,
> + scratch_pool,
> + scratch_pool));
>
> /* So, count this as a separate copy target only if the URLs
> * don't match up, or if the parent isn't copied at all. */
> @@ -607,6 +607,28 @@ svn_wc__node_get_copyfrom_info(const cha
> return SVN_NO_ERROR;
> }
>
> +svn_error_t *
> +svn_wc__node_get_copyfrom_info(const char **copyfrom_root_url,
> + const char **copyfrom_repos_relpath,
> + const char **copyfrom_url,
> + svn_revnum_t *copyfrom_rev,
> + svn_boolean_t *is_copy_target,
> + svn_wc_context_t *wc_ctx,
> + const char *local_abspath,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + return
> +
> svn_error_return(svn_wc__internal_get_copyfrom_info(copyfrom_root_url,
> +
> copyfrom_repos_relpath,
> + copyfrom_url,
> + copyfrom_rev,
> +
> is_copy_target,
> + wc_ctx->db,
> + local_abspath,
> + scratch_pool,
> +
> scratch_pool));
> +}
>
> /* A recursive node-walker, helper for svn_wc__node_walk_children().
> *
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_d
> b.h?rev=980784&r1=980783&r2=980784&view=diff
> =======================================================================
> =======
> --- subversion/trunk/subversion/libsvn_wc/wc_db.h (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.h Fri Jul 30 13:28:39
> 2010
> @@ -2536,6 +2536,17 @@ svn_wc__db_drop_root(svn_wc__db_t *db,
> const char *local_abspath,
> apr_pool_t *scratch_pool);
>
> +/* Internal version of svn_wc__node_get_copyfrom_info */
> +svn_error_t *
> +svn_wc__internal_get_copyfrom_info(const char **copyfrom_root_url,
> + const char
> **copyfrom_repos_relpath,
> + const char **copyfrom_url,
> + svn_revnum_t *copyfrom_rev,
> + svn_boolean_t *is_copy_target,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> /* @} */
>
>
>
Received on 2010-08-01 10:57:07 CEST