On Thu, Oct 29, 2009 at 04:59:24PM -0700, Paul T. Burba wrote:
> Author: pburba
> Date: Thu Oct 29 16:59:24 2009
> New Revision: 40306
>
> Log:
> Remove an svn_wc_entry_t.
>
> * subversion/libsvn_wc/copy.c
> (get_copyfrom_url_rev_via_parent): Remove.
> (determine_copyfrom_info): Use svn_wc__db_scan_addition() instead of
> get_copyfrom_url_rev_via_parent().
I was working on a patch to do just that, but you outraced me :)
Some remarks:
> @@ -338,8 +276,9 @@ determine_copyfrom_info(const char **cop
> const char *src_copyfrom_root_url;
> const char *src_copyfrom_relpath;
I want to drop the src_copyfrom prefix from all these variable names
to make them shorter.
> svn_revnum_t src_copyfrom_rev;
This variable is redundant, we can just use the existing 'rev' variable.
> + svn_wc__db_status_t status;
>
> - SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + SVN_ERR(svn_wc__db_read_info(&status, NULL, NULL, NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> NULL, &src_copyfrom_relpath,
> &src_copyfrom_root_url, NULL,
> @@ -355,13 +294,36 @@ determine_copyfrom_info(const char **cop
> src_copyfrom_relpath, result_pool);
The line above should say scratch_pool rather than result_pool (the
patch lacks the necessary context to see why).
> rev = src_copyfrom_rev;
> }
> - else
> + else if (status == svn_wc__db_status_added
> + || status == svn_wc__db_status_obstructed_add)
Ooops, I forgot to check for obstructed adds in my version of this change.
Thanks for doing this.
> {
> /* ...But if this file is merely the descendant of an explicitly
> copied/moved directory, we need to do a bit more work to
> determine copyfrom_url and copyfrom_rev. */
> - SVN_ERR(get_copyfrom_url_rev_via_parent(&url, &rev, db, src_abspath,
> - scratch_pool, scratch_pool));
> + const char *op_root_abspath;
> + const char *original_repos_relpath;
> + const char *original_root_url;
> + svn_revnum_t original_revision;
> + const char *op_root_rel_path;
Why not just re-use the existing variables?
> +
> + SVN_ERR(svn_wc__db_scan_addition(&status, &op_root_abspath,
The call the svn_wc__db_scan_addition only needs to be done if
status is 'copied' or 'moved here'. And then we don't need to look at
op_root_abspath at all.
> + NULL, NULL, NULL,
> + &original_repos_relpath,
> + &original_root_url, NULL,
> + &original_revision,
> + db, src_abspath,
> + scratch_pool, scratch_pool));
> + url = svn_uri_join(original_root_url, original_repos_relpath,
> + result_pool);
At the hackathon Greg said we should use svn_path_url_add_component2()
to join the root URL and the repository relpath, but I can't remember why.
> + op_root_rel_path = svn_dirent_is_child(op_root_abspath, src_abspath,
> + scratch_pool);
> + url = svn_uri_join(url, op_root_rel_path, scratch_pool);
My fire alarm goes off here, svn_dirent_is_child may return NULL !!!!!!
(In fact this makes svn_dirent_is_child() a fairly dangerous API,
maybe it should be made to return an empty string because that will do less
harm than crashing when this function's return value isn't validated?)
Fortunately we don't need to call it at all in the 'copied' or
'moved here' cases because these will always yield valid copyfrom
information and op_root_abspath is unrelated. So the above code can
just go away.
I've made all of these changes in r40321.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413220
Received on 2009-10-31 13:18:03 CET