You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Alexander Sinyushkin <Al...@svnkit.com> on 2008/06/25 12:02:24 UTC
[PATCH]: fix double occurrence of the same commit patch while 'svn
copy --parents url url' (was Re: Bug in 'svn copy --parents url url'?)
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
(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.
]]]
----
Alexander Sinyushkin,
TMate Software,
http://svnkit.com/ - Java [Sub]Versioning Library!
Karl Fogel wrote:
> Alexander Sinyushkin <Al...@svnkit.com> writes:
>> If I try to copy an existing directory to nonexistent one with
>> --parents, like this:
>>
>> svn copy --parents svn://localhost/repo/x svn://localhost/repo/y
>>
>> where x exists and y does not, an editor for commit message is
>> launched twice (if no -m or -F is provided). Commit succeeds,
>> i.e. afterwards I see /y/x in my repo but isn't it a bug that the
>> editor is launched twice?
>
> Yes, that's a bug.
>
>> Debugging this command I see that first call to setup_copy() fails
>> with the following error:
>>
>> "File already exists: filesystem '/home/alex/workspace/tmp/repo/db',
>> transaction '9-f', path '/y'"
>>
>> The problem here is in that commit paths array (which is passed to
>> svn_delta_path_driver()) contain two identical strings "y" what causes
>> the aforementioned error.
>>
>> But then the second call to setup_copy() is made in the following if
>> clause and commit ends well.
>>
>> if (copy_as_child && err && (sources->nelts == 1)
>> && (err->apr_err == SVN_ERR_ENTRY_EXISTS
>> || err->apr_err == SVN_ERR_FS_ALREADY_EXISTS))
>>
>> Anyway, I'm reporting this because you may decide that this behavior
>> should be changed. Thank you.
>
> I doubt anyone would disagree that it's bug... Since you're now familiar
> with the code path, do you have time to make a patch?
>
> Best,
> -Karl
>
>
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'?)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
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
Re: [PATCH]: fix double occurrence of the same commit patch while 'svn copy --parents url url'
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> Ping. This patch hasn't had any responses yet.
Thank you. It really should get reviewed, and it doesn't look like it
would be too hard. But Alexander, my apologies, I've just had a lot of
unexpected work to do on issue #2489, so my patch review committments
have gotten... um, delayed :-(.
If you could file a "[PATCH]" issue for this, that would be great. Or
if someone else has time to review it right now, that would be even
better.
-Karl
> Alexander Sinyushkin wrote on Wed, 25 Jun 2008 at 19:02 +0700:
>> 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
>> (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.
>> ]]]
>> ----
>> Alexander Sinyushkin,
>> TMate Software,
>> http://svnkit.com/ - Java [Sub]Versioning Library!
>>
>> Karl Fogel wrote:
>> > Alexander Sinyushkin <Al...@svnkit.com> writes:
>> > > If I try to copy an existing directory to nonexistent one with
>> > > --parents, like this:
>> > >
>> > > svn copy --parents svn://localhost/repo/x svn://localhost/repo/y
>> > >
>> > > where x exists and y does not, an editor for commit message is
>> > > launched twice (if no -m or -F is provided). Commit succeeds,
>> > > i.e. afterwards I see /y/x in my repo but isn't it a bug that the
>> > > editor is launched twice?
>> >
>> > Yes, that's a bug.
>> >
>> > > Debugging this command I see that first call to setup_copy() fails
>> > > with the following error:
>> > >
>> > > "File already exists: filesystem '/home/alex/workspace/tmp/repo/db',
>> > > transaction '9-f', path '/y'"
>> > >
>> > > The problem here is in that commit paths array (which is passed to
>> > > svn_delta_path_driver()) contain two identical strings "y" what causes
>> > > the aforementioned error.
>> > >
>> > > But then the second call to setup_copy() is made in the following if
>> > > clause and commit ends well.
>> > >
>> > > if (copy_as_child && err && (sources->nelts == 1)
>> > > && (err->apr_err == SVN_ERR_ENTRY_EXISTS
>> > > || err->apr_err == SVN_ERR_FS_ALREADY_EXISTS))
>> > >
>> > > Anyway, I'm reporting this because you may decide that this behavior
>> > > should be changed. Thank you.
>> >
>> > I doubt anyone would disagree that it's bug... Since you're now familiar
>> > with the code path, do you have time to make a patch?
>> >
>> > Best,
>> > -Karl
>> >
>> >
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
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'?)
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Ping. This patch hasn't had any responses yet.
Alexander Sinyushkin wrote on Wed, 25 Jun 2008 at 19:02 +0700:
> 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
> (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.
> ]]]
> ----
> Alexander Sinyushkin,
> TMate Software,
> http://svnkit.com/ - Java [Sub]Versioning Library!
>
> Karl Fogel wrote:
> > Alexander Sinyushkin <Al...@svnkit.com> writes:
> > > If I try to copy an existing directory to nonexistent one with
> > > --parents, like this:
> > >
> > > svn copy --parents svn://localhost/repo/x svn://localhost/repo/y
> > >
> > > where x exists and y does not, an editor for commit message is
> > > launched twice (if no -m or -F is provided). Commit succeeds,
> > > i.e. afterwards I see /y/x in my repo but isn't it a bug that the
> > > editor is launched twice?
> >
> > Yes, that's a bug.
> >
> > > Debugging this command I see that first call to setup_copy() fails
> > > with the following error:
> > >
> > > "File already exists: filesystem '/home/alex/workspace/tmp/repo/db',
> > > transaction '9-f', path '/y'"
> > >
> > > The problem here is in that commit paths array (which is passed to
> > > svn_delta_path_driver()) contain two identical strings "y" what causes
> > > the aforementioned error.
> > >
> > > But then the second call to setup_copy() is made in the following if
> > > clause and commit ends well.
> > >
> > > if (copy_as_child && err && (sources->nelts == 1)
> > > && (err->apr_err == SVN_ERR_ENTRY_EXISTS
> > > || err->apr_err == SVN_ERR_FS_ALREADY_EXISTS))
> > >
> > > Anyway, I'm reporting this because you may decide that this behavior
> > > should be changed. Thank you.
> >
> > I doubt anyone would disagree that it's bug... Since you're now familiar
> > with the code path, do you have time to make a patch?
> >
> > Best,
> > -Karl
> >
> >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org