Re: [PATCH]: fix double occurrence of the same commit patch while 'svn copy --parents url url' (was Re: Bug in 'svn copy --parents url url'?)

Alexander Sinyushkin wrote:
> Here is a patch to the problem in question to review .
>
> [[[
> Fix double occurrence of the same commit path
> in commit items array which takes place in
> 'svn cp --parent url/src url/dst' command
> where src exists and dst does not.
>
> * subversion/libsvn_client/copy.c
    ^
Should be in the first column

> (repos_to_repos_copy): when collecting new_dirs in case
> make_parents is true proceed only when dir (the result of
> svn_path_is_child(top_url, svn_path_dirname(pair->dst, pool),..))
> is not null. If dir is null do not bother collecting anything
> for new_dirs.

This part of the message looks a little bit *too* detailed. You've done a good
job outlining the approach with comments in the code, this can be a bit more
general (see what I ended up committing for an example).

> ]]]

Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c (revision 31880)
+++ subversion/libsvn_client/copy.c (working copy)
@@ -801,36 +801,32 @@
        dir = svn_path_is_child(top_url, svn_path_dirname(pair->dst, pool),
                                pool);

- /* Imagine a situation where the user passes --parents
- unnecessarily (that is, the destination directory already
- exists). Suppose they're copying a file when they do this.
- There are two syntaxes they might use:
-
- 1. svn copy --parents URL/src/foo.txt URL/dst/foo.txt
- 2. svn copy --parents URL/src/foo.txt URL/dst
-
- Assuming src/ and dst/ exist already, then both ways should
- create dst/foo.txt. However, the svn_path_dirname() call
+ /* Imagine a situation where the user tries to copy an existing source
+ directory to nonexistent directory with --parents options specified:
+
+ svn copy --parents URL/src URL/dst
+
+ where src exists and dst does not. the svn_path_dirname() call
           above will produce a string equivalent to top_url, which
- means the svn_path_is_child() will return NULL (quirkily,
- that's what it does when the two paths are the same). We
- must compensate for that behavior, while making sure not to
- add dst/ to the list of new_dirs to be created. */
-
- if (! dir)
- dir = svn_path_is_child(top_url, pair->dst, pool);
+ means the svn_path_is_child() will return NULL. In this case proceed
+ only if dir is not NULL. Do not try to add dst to the new_dirs list
+ since it will be added further in this function to commit items array
+ what can lead to double appearance of the same path in commit paths
array */

Please observe the 79-column limit.

- SVN_ERR(svn_ra_check_path(ra_session, dir, SVN_INVALID_REVNUM, &kind,
- iterpool));
-
- while (kind == svn_node_none)
+ if (dir)
          {
- svn_pool_clear(iterpool);
- APR_ARRAY_PUSH(new_dirs, const char *) = dir;
+ SVN_ERR(svn_ra_check_path(ra_session, dir, SVN_INVALID_REVNUM, &kind,
+ iterpool));

- svn_path_split(dir, &dir, NULL, pool);
- SVN_ERR(svn_ra_check_path(ra_session, dir, SVN_INVALID_REVNUM, &kind,
- iterpool));
+ while (kind == svn_node_none)
+ {
+ svn_pool_clear(iterpool);
+ APR_ARRAY_PUSH(new_dirs, const char *) = dir;
+
+ svn_path_split(dir, &dir, NULL, pool);
+ SVN_ERR(svn_ra_check_path(ra_session, dir, SVN_INVALID_REVNUM, &kind,

Same.

+ iterpool));

Extra parameters should be indented to the level of the other parameters to the
function.

+ }
          }
      }

Alexander,
The approach looks good, and I've committed the patch with tweak and a test in
r32203. I included the above feedback for next time. Also, is this bug present
in 1.5.0? Should this change be backported to the 1.5.x branch?

Thanks!

-Hyrum