On Sat, Oct 31, 2009 at 08:54:01AM -0400, Greg Stein wrote:
> I think this is wrong on several counts, and that Paul's changes were
> proper. This revision seems a complete step backwards.
>
> First, "repos_relpath" is a horrible variable name for this.
> read_info() has an output variable named repos_relpath which is
> TOTALLY different from the original_repos_relpath that you're actually
> querying. Same goes for the other variables. Paul kept the semantics
> on the variable names, which keeps the code very readable and
> understandable.
Good point.
> After the scan_addition, you join stuff to derive the *root* of the
> copyfrom. Paul's change computed corresponding source node.
Oops, I didn't read the entire read_info docstring.
To my defense, the docstring says this:
* ancestor unshadowed BASE node. ORIGINAL_* will indicate the source
* of the copy.
And if you stop reading there, you'll miss this bit which comes much later:
* ORIGINAL_REPOS_RELPATH will refer to the *root* of the operation. It
* does *not* correspond to the node given by LOCAL_ABSPATH.
I'd like to propose to never just say "ORIGINAL_* will indicate the
source of the copy" in that docstring, since doing so does kind of imply
that ORIGINAL_* will contain proper copyfrom information.
> add_component2() will escape the repos_relpath before appending to the URL.
Thanks.
So what about the diff below? It changes variable names to match the output
parameter names of read_info(), and fixes computation of the source node URL.
BTW none of the tests detected that my patch was computing wrong
copyfrom info -- I'll see about adding a test for this.
Stefan
Index: subversion/libsvn_wc/copy.c
===================================================================
--- subversion/libsvn_wc/copy.c (revision 40323)
+++ subversion/libsvn_wc/copy.c (working copy)
@@ -272,25 +272,26 @@ determine_copyfrom_info(const char **copyfrom_url,
apr_pool_t *scratch_pool)
{
const char *url;
- svn_revnum_t rev;
- const char *root_url;
- const char *repos_relpath;
+ const char *original_root_url;
+ const char *original_repos_relpath;
+ svn_revnum_t original_revision;
svn_wc__db_status_t status;
url = NULL;
- rev = SVN_INVALID_REVNUM;
+ original_revision = SVN_INVALID_REVNUM;
SVN_ERR(svn_wc__db_read_info(&status, NULL, NULL, NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL, NULL, NULL,
- NULL, &repos_relpath, &root_url, NULL,
- &rev, NULL, NULL, NULL, NULL,
- NULL, db, src_abspath, scratch_pool,
- scratch_pool));
- if (root_url && repos_relpath)
+ NULL, &original_repos_relpath,
+ &original_root_url, NULL, &original_revision,
+ NULL, NULL, NULL, NULL, NULL, db, src_abspath,
+ scratch_pool, scratch_pool));
+ if (original_root_url && original_repos_relpath)
{
/* When copying/moving a file that was already explicitly
copied/moved then we know the URL it was copied from... */
- url = svn_path_url_add_component2(root_url, repos_relpath, scratch_pool);
+ url = svn_path_url_add_component2(original_root_url,
+ original_repos_relpath, scratch_pool);
}
else if (status == svn_wc__db_status_added
|| status == svn_wc__db_status_obstructed_add)
@@ -298,31 +299,46 @@ determine_copyfrom_info(const char **copyfrom_url,
/* ...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(svn_wc__db_scan_addition(&status, NULL, NULL, NULL, NULL,
- &repos_relpath, &root_url, NULL,
- &rev, db, src_abspath,
+ const char *op_root_abspath;
+
+ SVN_ERR(svn_wc__db_scan_addition(&status, &op_root_abspath, NULL, NULL,
+ NULL, &original_repos_relpath,
+ &original_root_url, NULL,
+ &original_revision, db, src_abspath,
scratch_pool, scratch_pool));
if (status == svn_wc__db_status_copied ||
status == svn_wc__db_status_moved_here)
{
- url = svn_path_url_add_component2(root_url, repos_relpath,
- scratch_pool);
+ const char *op_root_relpath;
+ const char *parent_url;
+
+ parent_url = svn_path_url_add_component2(original_root_url,
+ original_repos_relpath,
+ scratch_pool);
+ op_root_relpath = svn_dirent_is_child(op_root_abspath, src_abspath,
+ scratch_pool);
+ if (op_root_relpath)
+ {
+ url = svn_path_url_add_component2(parent_url, op_root_relpath,
+ scratch_pool);
+ }
}
}
- if (url && dst_url && strcmp(url, dst_url) == 0 && rev == dst_revision)
+ if (url && dst_url && strcmp(url, dst_url) == 0 &&
+ original_revision == dst_revision)
{
/* Suppress copyfrom info when the copy source is the same as
for the destination. */
url = NULL;
- rev = SVN_INVALID_REVNUM;
+ original_revision = SVN_INVALID_REVNUM;
}
if (url)
*copyfrom_url = apr_pstrdup(result_pool, url);
else
*copyfrom_url = NULL;
- *copyfrom_rev = rev;
+ *copyfrom_rev = original_revision;
return SVN_NO_ERROR;
}
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413247
Received on 2009-10-31 15:17:12 CET