You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2010/05/16 21:24:22 UTC
[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'.
]]]
Daniel
Re: [WIP] Use repos_root_url and repos_relpath in the status code.
Posted by Greg Stein <gs...@gmail.com>.
On Wed, May 19, 2010 at 05:37, Bert Huijben <be...@qqmail.nl> wrote:
>...
>> + 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).
That is the 'abs_path' grammar token from the URI specification.
>...
>> @@ -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)
This is in dup() ... you *have* to duplicate these values. ??
Cheers,
-g
RE: [WIP] Use repos_root_url and repos_relpath in the status code.
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Näslund [mailto:daniel@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
RE: [WIP] Use repos_root_url and repos_relpath in the status code.
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Näslund [mailto:daniel@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