You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2009/10/31 12:54:01 UTC
Re: svn commit: r40321 - trunk/subversion/libsvn_wc
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.
After the scan_addition, you join stuff to derive the *root* of the
copyfrom. Paul's change computed corresponding source node.
add_component2() will escape the repos_relpath before appending to the URL.
Cheers,
-g
On Sat, Oct 31, 2009 at 08:08, Stefan Sperling <st...@elego.de> wrote:
> Author: stsp
> Date: Sat Oct 31 05:08:03 2009
> New Revision: 40321
>
> Log:
> Follow-up to r40306:
>
> * subversion/libsvn_wc/copy.c
> (determine_copyfrom_info): Remove redundant SRC_COPYFROM_REV variable
> and use REV directly. Rename SRC_COPYFROM_ROOT_URL and SRC_COPYFROM_RELPATH
> variables to ROOT_URL and REPOS_RELPATH, respectively.
> Only scan up the tree for copyfrom if the node's status indicates that
> it was copied or moved, and re-use existing variables when doing so instead
> of using new ones. Also, use svn_path_url_add_component2() instead of
> svn_uri_join() to join the repository root URL with the repository
> relpath. At the hackathon, gstein said it was better to use
> svn_path_url_add_component2() for this, but I cannot remember why.
>
> Modified:
> trunk/subversion/libsvn_wc/copy.c
>
> Modified: trunk/subversion/libsvn_wc/copy.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/copy.c?pathrev=40321&r1=40320&r2=40321
> ==============================================================================
> --- trunk/subversion/libsvn_wc/copy.c Fri Oct 30 18:56:23 2009 (r40320)
> +++ trunk/subversion/libsvn_wc/copy.c Sat Oct 31 05:08:03 2009 (r40321)
> @@ -273,26 +273,21 @@ determine_copyfrom_info(const char **cop
> {
> const char *url;
> svn_revnum_t rev;
> - const char *src_copyfrom_root_url;
> - const char *src_copyfrom_relpath;
> - svn_revnum_t src_copyfrom_rev;
> + const char *root_url;
> + const char *repos_relpath;
> svn_wc__db_status_t status;
>
> 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,
> - &src_copyfrom_rev, NULL, NULL, NULL, NULL,
> + NULL, &repos_relpath, &root_url, NULL,
> + &rev, NULL, NULL, NULL, NULL,
> NULL, db, src_abspath, result_pool,
> scratch_pool));
> -
> - if (src_copyfrom_root_url && src_copyfrom_relpath)
> + if (root_url && 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(src_copyfrom_root_url,
> - src_copyfrom_relpath, result_pool);
> - rev = src_copyfrom_rev;
> + url = svn_path_url_add_component2(root_url, repos_relpath, scratch_pool);
> }
> else if (status == svn_wc__db_status_added
> || status == svn_wc__db_status_obstructed_add)
> @@ -300,25 +295,16 @@ determine_copyfrom_info(const char **cop
> /* ...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. */
> - 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;
> -
> - 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,
> + SVN_ERR(svn_wc__db_scan_addition(&status, NULL, NULL, NULL, NULL,
> + &repos_relpath, &root_url, NULL,
> + &rev, db, src_abspath,
> scratch_pool, scratch_pool));
> - url = svn_uri_join(original_root_url, original_repos_relpath,
> - result_pool);
> - 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);
> - rev = original_revision;
> + 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);
> + }
> }
> else
> {
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=2413219
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413230
Re: svn commit: r40321 - trunk/subversion/libsvn_wc
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Oct 31, 2009 at 03:16:39PM +0100, Stefan Sperling wrote:
> 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.
And I think this version uses slightly clearer variable names during
source node URL computation.
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,44 @@ 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,
+ const char *src_parent_url;
+ const char *src_relpath;
+
+ src_parent_url = svn_path_url_add_component2(original_root_url,
+ original_repos_relpath,
+ scratch_pool);
+ src_relpath = svn_dirent_is_child(op_root_abspath, src_abspath,
scratch_pool);
+ if (src_relpath)
+ url = svn_path_url_add_component2(src_parent_url, src_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=2413249
Re: svn commit: r40321 - trunk/subversion/libsvn_wc
Posted by Stefan Sperling <st...@elego.de>.
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