> -----Original Message-----
> From: Daniel Näslund [mailto:dan...@longitudo.com]
> Sent: zondag 16 mei 2010 23:24
> To: dev@subversion.apache.org
> Subject: [WIP] Use repos_root_url and repos_relpath in the status code.
>
> Hi!
>
> There's a lot of parameteter tracking in this patch. Basically it's all
> about passing down the url arguments to assemble_status().
>
> The goal is that we should be able to remove the entry and parent_entry
> fields in a follow-up and be able to use the parent_relpath when we want
> to detect switches but also for determining the toplevel op_root of a
> copy (we may have a copy below another copy with mixed revs).
>
> Sending it in to see if anyone has any objections. I don't feel
> comfortable committing it without someone more experienced giving it a
> look.
>
> [[[
> First step in the move to using repos_root_url and repos_relpaths for
> describing urls in the status code.
>
> The idea is to reuse the parents repos_relpath when detecting if a node is
> switched instead of doing an extra scan for each node. Since we're doing
a
> depth first traversal of the wc, the parent has already been visited.
> Hopefully we will save some read calls.
>
> We still depend on the url field but the plan is to remove it.
>
> * subversion/include/svn_wc.h
> (svn_wc_status3_t): Add new fields 'repos_root_url' and 'repos_relpath'.
>
> * subversion/libsvn_wc/status.c
> (assemble_status): Add new parameters. Go back to using the
> previous way of detecting a switched path; simply comparing the
> repos_relpath with the parent_repos_relpath + basename(path).
> (send_status_structure): Add parameter 'parent_repos_root_url' and
> 'parent_repos_relpath.
> (handle_dir_entry): Add parameter 'dir_repos_root_url' and
> 'dir_repos_relpath'.
> (internal_status): Fetch the parent_repos_root_url and
> parent_repos_relpath from the db. This function
> is called on the anchor paths.
> (get_dir_status): Add parameter 'parent_repos_root_url' and
> 'parent_repos_relpath.
> (svn_wc_dup_status3): Duplicate 'repos_root_url' and 'repos_relpath'.
> ]]]
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 944886)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -3647,6 +3647,13 @@ typedef struct svn_wc_status3_t
> /** Which changelist this item is part of, or NULL if not part of any.
*/
> const char *changelist;
>
> + /** The leading part of the url, not including the wc root and
subsequent
> + * paths. */
> + const char *repos_root_url;
> +
> + /** The path relative to the wc root. */
> + const char *repos_relpath;
> +
> /* NOTE! Please update svn_wc_dup_status3() when adding new fields
here. */
> } svn_wc_status3_t;
>
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 944886)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -273,6 +273,8 @@ assemble_status(svn_wc_status3_t **status,
> const char *local_abspath,
> const svn_wc_entry_t *entry,
> const svn_wc_entry_t *parent_entry,
> + const char *parent_repos_root_url,
> + const char *parent_repos_relpath,
> svn_node_kind_t path_kind,
> svn_boolean_t path_special,
> svn_boolean_t get_all,
> @@ -284,6 +286,8 @@ assemble_status(svn_wc_status3_t **status,
> svn_wc_status3_t *stat;
> svn_wc__db_status_t db_status;
> svn_wc__db_kind_t db_kind;
> + const char *repos_relpath;
> + const char *repos_root_url;
> const char *url;
> svn_boolean_t locked_p = FALSE;
> svn_boolean_t switched_p = FALSE;
> @@ -313,16 +317,20 @@ assemble_status(svn_wc_status3_t **status,
> SVN_ERR(svn_wc__db_op_read_tree_conflict(&tree_conflict, db,
local_abspath,
> scratch_pool, scratch_pool));
>
> - SVN_ERR(svn_wc__db_read_info(&db_status, &db_kind, &revision, NULL,
NULL,
> - NULL, &changed_rev, &changed_date,
> + SVN_ERR(svn_wc__db_read_info(&db_status, &db_kind, &revision,
> + &repos_relpath, &repos_root_url, NULL,
> + &changed_rev, &changed_date,
> &changed_author, NULL, NULL, NULL, NULL,
> NULL, &changelist, NULL, NULL, NULL, NULL,
> NULL, NULL, &base_shadowed, &conflicted,
> &lock, db, local_abspath, result_pool,
> scratch_pool));
>
> + /* ### Temporary until we've revved svn_wc_status3_t to use
> + * ### repos_{root_url,relpath} */
> SVN_ERR(svn_wc__internal_node_get_url(&url, db, local_abspath,
> result_pool, scratch_pool));
> +
> SVN_ERR(svn_wc__internal_is_file_external(&file_external_p, db,
> local_abspath,
scratch_pool));
>
> @@ -331,10 +339,23 @@ assemble_status(svn_wc_status3_t **status,
> an URL, at the very least. */
> if (! file_external_p)
> {
> - svn_boolean_t is_wc_root; /* Not used. */
> -
> - SVN_ERR(svn_wc__check_wc_root(&is_wc_root, NULL, &switched_p, db,
> - local_abspath, scratch_pool));
> + if (parent_repos_root_url && repos_root_url &&
> + (strcmp(parent_repos_root_url, repos_root_url) == 0))
> + {
> + /* ### Can we just join them like that? What about an added
node?
> + * ### It doesn't have an url yet!
> + * ### Is the join ok? A relpath is NOT uri-encoded so it
should
> + * ### be fine? */
> + if (! repos_relpath)
> + repos_relpath = svn_relpath_join(
> + parent_repos_relpath,
> +
svn_relpath_basename(local_abspath,
> +
result_pool),
Use svn_dirent_basename() to get the basename from an abspath
You can pass a NULL pool to svn_dirent_basename() to avoid coping the data.
(You get a pointer into the original path)
> + result_pool);
> + switched_p = (svn_relpath_is_child(
> + parent_repos_relpath,
> + repos_relpath, scratch_pool) ==
NULL);
This checks if the path is 'some' child of parent_repos_relpath. Not if it
is the unswitched child. (That would require checking the result against the
basename)
(You can use the same NULL pool trick here, to avoid the copy)
> + }
> }
>
> /* Examine whether our directory metadata is present, and compensate
> @@ -617,6 +638,8 @@ assemble_status(svn_wc_status3_t **status,
> stat->conflicted = conflicted;
> stat->versioned = TRUE;
> stat->changelist = changelist;
> + stat->repos_root_url = repos_root_url;
> + stat->repos_relpath = repos_relpath;
>
> *status = stat;
>
> @@ -692,6 +715,8 @@ send_status_structure(const struct walk_status_bat
> const char *local_abspath,
> const svn_wc_entry_t *entry,
> const svn_wc_entry_t *parent_entry,
> + const char *parent_repos_root_url,
> + const char *parent_repos_relpath,
> svn_node_kind_t path_kind,
> svn_boolean_t path_special,
> svn_boolean_t get_all,
> @@ -712,10 +737,16 @@ send_status_structure(const struct walk_status_bat
>
> if (entry->url)
> abs_path = entry->url + strlen(wb->repos_root);
> - else if (parent_entry && parent_entry->url)
> - abs_path = svn_uri_join(parent_entry->url +
strlen(wb->repos_root),
> - svn_dirent_basename(local_abspath, NULL),
> - pool);
> + else if (parent_repos_root_url && parent_repos_relpath)
> + {
> + /* ### Is this join ok? */
> + const char *parent_url = svn_uri_join(parent_repos_root_url,
> + parent_repos_relpath,
pool);
Answering this question: No, you need svn_path_url_add_component2() to
escape the relpath.
> +
> + abs_path = svn_uri_join(parent_url + strlen(wb->repos_root),
> + svn_dirent_basename(local_abspath,
NULL),
> + pool);
And this has the same issues with encoding. (And I don't know what you are
calculating here. It doesn't look like an absolute dirent. Please use a
different variable name).
Do you need the urls here?
(Calculating with repos_relpaths is much easier as you can forget all the
encoding rules)
> + }
> else
> abs_path = NULL;
>
> @@ -726,9 +757,9 @@ send_status_structure(const struct walk_status_bat
> }
>
> SVN_ERR(assemble_status(&statstruct, wb->db, local_abspath, entry,
> - parent_entry, path_kind, path_special, get_all,
> - is_ignored, repos_lock,
> - pool, pool));
> + parent_entry, parent_repos_root_url,
> + parent_repos_relpath, path_kind, path_special,
> + get_all, is_ignored, repos_lock, pool, pool));
>
> if (statstruct && status_func)
> return svn_error_return((*status_func)(status_baton, local_abspath,
> @@ -886,6 +917,8 @@ static svn_error_t *
> get_dir_status(const struct walk_status_baton *wb,
> const char *local_abspath,
> const svn_wc_entry_t *parent_entry,
> + const char *parent_repos_root_url,
> + const char *parent_repos_relpath,
> const char *selected,
> const apr_array_header_t *ignores,
> svn_depth_t depth,
> @@ -908,6 +941,8 @@ handle_dir_entry(const struct walk_status_baton *w
> const char *local_abspath,
> const svn_wc_entry_t *dir_entry,
> const svn_wc_entry_t *entry,
> + const char *dir_repos_root_url,
> + const char *dir_repos_relpath,
> svn_node_kind_t path_kind,
> svn_boolean_t path_special,
> const apr_array_header_t *ignores,
> @@ -935,18 +970,21 @@ handle_dir_entry(const struct walk_status_baton *w
> || depth == svn_depth_immediates
> || depth == svn_depth_infinity))
> {
> - SVN_ERR(get_dir_status(wb, local_abspath, dir_entry, NULL,
ignores,
> - depth, get_all, no_ignore, FALSE,
> - get_excluded, status_func, status_baton,
> - cancel_func, cancel_baton, pool));
> + SVN_ERR(get_dir_status(wb, local_abspath, dir_entry,
> + dir_repos_root_url, dir_repos_relpath,
> + NULL, ignores, depth, get_all,
no_ignore,
> + FALSE, get_excluded, status_func,
> + status_baton, cancel_func, cancel_baton,
> + pool));
> }
> else
> {
> /* ENTRY is a child entry (file or parent stub). Or we have a
> directory entry but DEPTH is limiting our recursion. */
> SVN_ERR(send_status_structure(wb, local_abspath, entry,
> - dir_entry,
> - svn_node_dir, FALSE /*
path_special */,
> + dir_entry, dir_repos_root_url,
> + dir_repos_relpath, svn_node_dir,
> + FALSE /* path_special */,
> get_all, FALSE /* is_ignored */,
> status_func, status_baton,
pool));
> }
> @@ -955,8 +993,10 @@ handle_dir_entry(const struct walk_status_baton *w
> {
> /* This is a file/symlink on-disk. */
> SVN_ERR(send_status_structure(wb, local_abspath, entry,
> - dir_entry, path_kind, path_special,
> - get_all, FALSE /* is_ignored */,
> + dir_entry, dir_repos_root_url,
> + dir_repos_relpath, path_kind,
> + path_special, get_all,
> + FALSE /* is_ignored */,
> status_func, status_baton, pool));
> }
>
> @@ -1035,6 +1075,8 @@ static svn_error_t *
> get_dir_status(const struct walk_status_baton *wb,
> const char *local_abspath,
> const svn_wc_entry_t *parent_entry,
> + const char *parent_repos_root_url,
> + const char *parent_repos_relpath,
> const char *selected,
> const apr_array_header_t *ignore_patterns,
> svn_depth_t depth,
> @@ -1050,6 +1092,8 @@ get_dir_status(const struct walk_status_baton *wb,
> {
> apr_hash_index_t *hi;
> const svn_wc_entry_t *dir_entry;
> + const char *dir_repos_root_url;
> + const char *dir_repos_relpath;
> apr_hash_t *dirents, *nodes, *conflicts, *all_children;
> apr_array_header_t *patterns = NULL;
> apr_pool_t *iterpool, *subpool = svn_pool_create(scratch_pool);
> @@ -1078,6 +1122,14 @@ get_dir_status(const struct walk_status_baton *wb,
> SVN_ERR(svn_wc__get_entry(&dir_entry, wb->db, local_abspath, FALSE,
> svn_node_dir, FALSE, subpool, iterpool));
>
> + SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, &dir_repos_relpath,
> + &dir_repos_root_url, NULL, NULL, NULL,
NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, wb->db, local_abspath, scratch_pool,
> + scratch_pool));
> +
> +
> if (selected == NULL)
> {
> const apr_array_header_t *victims;
> @@ -1126,7 +1178,9 @@ get_dir_status(const struct walk_status_baton *wb,
> if (! skip_this_dir)
> SVN_ERR(send_status_structure(wb, local_abspath,
> dir_entry, parent_entry,
> - svn_node_dir, FALSE /* path_special
*/,
> + parent_repos_root_url,
> + parent_repos_relpath, svn_node_dir,
> + FALSE /* path_special */,
> get_all, FALSE /* is_ignored */,
> status_func, status_baton,
> iterpool));
> @@ -1204,6 +1258,8 @@ get_dir_status(const struct walk_status_baton *wb,
> node_abspath,
> dir_entry,
> entry,
> + dir_repos_root_url,
> + dir_repos_relpath,
> dirent_p ? dirent_p->kind
> : svn_node_none,
> dirent_p ? dirent_p->special :
FALSE,
> @@ -1564,8 +1620,10 @@ make_dir_baton(void **dir_baton,
> const apr_array_header_t *ignores = eb->ignores;
>
> SVN_ERR(get_dir_status(&eb->wb, local_abspath,
> - status_in_parent->entry, NULL,
> - ignores, d->depth == svn_depth_files ?
> + status_in_parent->entry,
> + status_in_parent->repos_root_url,
> + status_in_parent->repos_relpath,
> + NULL, ignores, d->depth == svn_depth_files ?
> svn_depth_files : svn_depth_immediates,
> TRUE, TRUE, TRUE, FALSE, hash_stash,
d->statii,
> NULL, NULL, pool));
> @@ -1707,6 +1765,8 @@ mark_deleted(void *baton,
> static svn_error_t *
> handle_statii(struct edit_baton *eb,
> const svn_wc_entry_t *dir_entry,
> + const char *dir_repos_root_url,
> + const char *dir_repos_relpath,
> apr_hash_t *statii,
> svn_boolean_t dir_was_deleted,
> svn_depth_t depth,
> @@ -1750,10 +1810,10 @@ handle_statii(struct edit_baton *eb,
>
> SVN_ERR(get_dir_status(&eb->wb,
> local_abspath,
> - dir_entry, NULL,
> - ignores, depth, eb->get_all,
> - eb->no_ignore, TRUE, FALSE, status_func,
> - status_baton, eb->cancel_func,
> + dir_entry, dir_repos_root_url,
> + dir_repos_relpath, NULL, ignores, depth,
> + eb->get_all, eb->no_ignore, TRUE, FALSE,
> + status_func, status_baton,
eb->cancel_func,
> eb->cancel_baton, subpool));
> }
> if (dir_was_deleted)
> @@ -1989,6 +2049,8 @@ close_directory(void *dir_baton,
>
> /* Now do the status reporting. */
> SVN_ERR(handle_statii(eb, dir_status ? dir_status->entry : NULL,
> + dir_status ? dir_status->repos_root_url :
NULL,
> + dir_status ? dir_status->repos_relpath :
NULL,
> db->statii, was_deleted, db->depth, pool));
> if (dir_status && svn_wc__is_sendable_status(dir_status,
eb->no_ignore,
> eb->get_all))
> @@ -2012,7 +2074,7 @@ close_directory(void *dir_baton,
> && tgt_status->entry->kind == svn_node_dir)
> {
> SVN_ERR(get_dir_status(&eb->wb, eb->target_abspath,
> - tgt_status->entry, NULL,
> + tgt_status->entry, NULL, NULL,
NULL,
> eb->ignores, eb->default_depth,
> eb->get_all, eb->no_ignore,
TRUE,
> FALSE,
> @@ -2032,6 +2094,8 @@ close_directory(void *dir_baton,
> Note that our directory couldn't have been deleted,
> because it is the root of the edit drive. */
> SVN_ERR(handle_statii(eb, eb->anchor_status->entry,
> + eb->anchor_status->repos_root_url,
> + eb->anchor_status->repos_relpath,
> db->statii, FALSE, eb->default_depth,
pool));
> if (svn_wc__is_sendable_status(eb->anchor_status,
eb->no_ignore,
> eb->get_all))
> @@ -2358,11 +2422,15 @@ svn_wc_walk_status(svn_wc_context_t *wc_ctx,
> SVN_ERR(svn_wc_read_kind(&kind, wc_ctx, local_abspath, FALSE,
scratch_pool));
> SVN_ERR(svn_io_check_path(local_abspath, &local_kind, scratch_pool));
>
> + /* ### Why do we pass NULL for the url related parameters in all three
> + * calls to get_dir_status()? */
> if (kind == svn_node_file && local_kind == svn_node_file)
> {
> SVN_ERR(get_dir_status(&wb,
> svn_dirent_dirname(local_abspath,
scratch_pool),
> + NULL,
> NULL,
> + NULL,
> svn_dirent_basename(local_abspath, NULL),
> ignore_patterns,
> depth,
> @@ -2382,6 +2450,8 @@ svn_wc_walk_status(svn_wc_context_t *wc_ctx,
> local_abspath,
> NULL,
> NULL,
> + NULL,
> + NULL,
> ignore_patterns,
> depth,
> get_all,
> @@ -2399,6 +2469,8 @@ svn_wc_walk_status(svn_wc_context_t *wc_ctx,
> SVN_ERR(get_dir_status(&wb,
> svn_dirent_dirname(local_abspath,
scratch_pool),
> NULL,
> + NULL,
> + NULL,
> svn_dirent_basename(local_abspath, NULL),
> ignore_patterns,
> depth,
> @@ -2467,6 +2539,8 @@ internal_status(svn_wc_status3_t **status,
> svn_boolean_t path_special;
> const svn_wc_entry_t *entry;
> const svn_wc_entry_t *parent_entry = NULL;
> + const char *parent_repos_relpath;
> + const char *parent_repos_root_url;
> svn_error_t *err;
>
> SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
> @@ -2507,6 +2581,30 @@ internal_status(svn_wc_status3_t **status,
> const char *parent_abspath = svn_dirent_dirname(local_abspath,
> scratch_pool);
>
> + /* ### Do I need to check for base_shadowed here? */
> + err = svn_wc__db_read_info(NULL, NULL, NULL, &parent_repos_relpath,
> + &parent_repos_root_url, NULL, NULL,
NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL,
NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL,
NULL,
> + NULL, db, parent_abspath, result_pool,
> + scratch_pool);
I don't think status is interested in shadowed. Just look at the status.
> +
> + /* ### Does WC-NG throw _NODE_UNEXPECTED_KIND? I thought that would
be
> + * ### handled with svn_wc__db_status_{obstructed, added_obstruct,
> + * ### deleted_obstruct} */
No. You see unexpected kinds as svn_wc__db_status_obstructed*.
> + if (err && (err->apr_err == SVN_ERR_WC_MISSING
> + || err->apr_err == SVN_ERR_WC_NOT_WORKING_COPY
> + || err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND
> + || err->apr_err == SVN_ERR_NODE_UNEXPECTED_KIND))
> + {
> + svn_error_clear(err);
> + parent_entry = NULL;
> + parent_repos_root_url = NULL;
> + parent_repos_relpath = NULL;
> + }
> + else if (err)
> + return svn_error_return(err);
> +
> err = svn_wc__get_entry(&parent_entry, db, parent_abspath, TRUE,
> svn_node_dir, FALSE, scratch_pool,
scratch_pool);
> if (err && (err->apr_err == SVN_ERR_WC_MISSING
> @@ -2523,7 +2621,9 @@ internal_status(svn_wc_status3_t **status,
>
> return svn_error_return(assemble_status(status, db, local_abspath,
> entry, parent_entry,
> - path_kind, path_special,
> + parent_repos_root_url,
> + parent_repos_relpath,
path_kind,
> + path_special,
> TRUE /* get_all */,
> FALSE /* is_ignored */,
> NULL /* repos_lock */,
> @@ -2585,6 +2685,14 @@ svn_wc_dup_status3(const svn_wc_status3_t *orig_st
> new_stat->changelist
> = apr_pstrdup(pool, orig_stat->changelist);
>
> + if (orig_stat->repos_root_url)
> + new_stat->repos_root_url
> + = apr_pstrdup(pool, orig_stat->repos_root_url);
> +
> + if (orig_stat->repos_relpath)
> + new_stat->repos_relpath
> + = apr_pstrdup(pool, orig_stat->repos_relpath);
> +
Do you have to duplicate these values, or is the lifetime of these orig_stat
values long enough?
(I think you can just set the value from the parent, but I didn't check)
Bert
