[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r40321 - trunk/subversion/libsvn_wc

From: Stefan Sperling <stsp_at_elego.de>
Date: Sat, 31 Oct 2009 15:16:39 +0100

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

This is an archived mail posted to the Subversion Dev mailing list.