Hi Neels. I'm not in a position to comment on anything other than
low-level issues, but I can do that.
Neels J Hofmeyr wrote:
> Index: subversion/libsvn_wc/externals.c
> ===================================================================
> --- subversion/libsvn_wc/externals.c (revision 1172371)
> +++ subversion/libsvn_wc/externals.c (working copy)
> [...]
Move doc string to header file.
> +/* If the URL for @a item is relative, then using the repository root
> + URL @a repos_root_url and the parent directory URL @parent_dir_url,
> + resolve it into an absolute URL and save it in @a item.
I know you only moved this function, but it looks like it does not 'save
it in @a item' but rather 'Set @a *resolved_url to ...'.
> + Regardless if the URL is absolute or not, if there are no errors,
> + the URL in @a item will be canonicalized.
Given the above, is that still true?
> + The following relative URL formats are supported:
> +
> + ../ relative to the parent directory of the external
> + ^/ relative to the repository root
> + // relative to the scheme
> + / relative to the server's hostname
> +
> + The ../ and ^/ relative URLs may use .. to remove path elements up
> + to the server root.
> +
> + The external URL should not be canonicalized before calling this function,
> + as otherwise the scheme relative URL '//host/some/path' would have been
> + canonicalized to '/host/some/path' and we would not be able to match on
> + the leading '//'. */
> +svn_error_t *
> +svn_wc__resolve_relative_external_url(const char **resolved_url,
> + const svn_wc_external_item2_t *item,
> + const char *repos_root_url,
> + const char *parent_dir_url,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> [...]
> +}
> Index: subversion/libsvn_wc/wc_db.c
> ===================================================================
> --- subversion/libsvn_wc/wc_db.c (revision 1172371)
> +++ subversion/libsvn_wc/wc_db.c (working copy)
> [...]
> @@ -10378,23 +10379,80 @@ svn_wc__db_upgrade_apply_props(svn_sqlit
> {
> int i;
> apr_array_header_t *ext;
> + const char *node_repos_relpath = "";
> + const char *node_repos_root_url = "";
> + const char *node_url = "";
> + apr_int64_t node_repos_id;
> + const char *local_abspath = svn_dirent_join(dir_abspath,
> + local_relpath,
> + scratch_pool);
A one-line comment here summarizing the next 22 lines would be lovely.
I'm not sure; is it trying to get 'node_repos_id' and
'node_repos_relpath' for the current node? It seems to be handling
those two variables in different ways, whereas they're intimately linked
so should be calculated together. Maybe clearer to initialize them in
this code block in both cases, and not initialize their declarations
above.
> + SVN_ERR(svn_sqlite__get_statement(&stmt, sdb,
> + STMT_SELECT_BASE_NODE));
> + SVN_ERR(svn_sqlite__bindf(stmt, "is", wc_id, local_relpath));
> + SVN_ERR(svn_sqlite__step(&have_row, stmt));
> + if (have_row)
> + {
> + svn_error_t *err;
> + const char *repos_relpath;
> + err = repos_location_from_columns(&node_repos_id, NULL, &repos_relpath,
> + stmt, 0, 4, 1, scratch_pool);
> + if (err)
> + svn_error_clear(err);
> + else
> + {
> + node_repos_relpath = repos_relpath;
> + SVN_ERR(fetch_repos_info(&node_repos_root_url, NULL, sdb,
> + node_repos_id, scratch_pool));
> + SVN_DBG(("local_relpath=%s url=%s / %s\n",
> + local_relpath, node_repos_root_url,
> + node_repos_relpath));
> + }
> + }
> + svn_sqlite__reset(stmt);
> +
>
> SVN_ERR(svn_sqlite__get_statement(&stmt, sdb,
> STMT_INSERT_EXTERNAL_UPGRADE));
>
> - SVN_ERR(svn_wc_parse_externals_description3(
> - &ext, svn_dirent_join(dir_abspath, local_relpath,
> - scratch_pool),
> - externals, FALSE, scratch_pool));
> + SVN_ERR(svn_wc_parse_externals_description3(&ext, local_abspath,
> + externals, FALSE,
> + scratch_pool));
> for (i = 0; i < ext->nelts; i++)
> {
> const svn_wc_external_item2_t *item;
> const char *item_relpath;
> + const char *item_resolved_url;
> + const char *item_repos_relpath;
>
> item = APR_ARRAY_IDX(ext, i, const svn_wc_external_item2_t *);
> item_relpath = svn_relpath_join(local_relpath, item->target_dir,
> scratch_pool);
A helper function to resolve to 'item_repos_id' and 'item_repos_relpath'
instead of the next 24 lines would be clearer. And/or a block comment.
> + node_url = svn_path_url_add_component2(node_repos_root_url,
> + node_repos_relpath,
> + scratch_pool);
> +
> + SVN_ERR(svn_wc__resolve_relative_external_url(&item_resolved_url,
> + item,
> + node_repos_root_url,
> + node_url,
> + scratch_pool,
> + scratch_pool));
> +
> + item_repos_relpath = svn_uri_skip_ancestor(node_repos_root_url,
> + item_resolved_url,
> + scratch_pool);
Please set an 'item_repos_id' variable here rather than relying on it
being the same as 'node_repos_id' which is true only because of the
following restriction.
> + if (item_repos_relpath == NULL)
> + {
> + /* It's from another repository.
> + * At this point we would have to contact the repository and
> + * find out its UUID and root. But the same information may
> + * already be lying around in a checked out external dir.
> + * ... currently, this is just wrong: */
item_repos_id = 1;
> + item_repos_relpath = "";
> + }
> +
> SVN_ERR(svn_sqlite__bindf(stmt, "issssis",
> wc_id,
> item_relpath,
> @@ -10402,8 +10460,8 @@ svn_wc__db_upgrade_apply_props(svn_sqlit
> scratch_pool),
> "normal",
> local_relpath,
> - (apr_int64_t)1, /* repos_id */
> - "" /* repos_relpath */));
> + node_repos_id,
item_repos_id,
> + item_repos_relpath));
>
> SVN_ERR(svn_sqlite__insert(NULL, stmt));
> }
Received on 2011-09-21 11:06:22 CEST