You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2006/01/25 22:40:12 UTC
r14401 and copy mydir mydir
Hi,
In r14401, an issue (2224) was fixed making commands like
svn cp wcdir wcdir
not get stuck in an infinite loop. This was fixed by allowing this
operation as a special case. This operation will result in a directory
wcdir begin created as a child of wcdir (as one could expect). But, if
you do
svn cp wcdir wcdir/wcdir
you'll get an error about not being able to copy a path into its own
child. This also holds if you try to copy a WC dir into a deeper
descendant.
I stumbled on this while working on making svn_client_copy always fail if
the target exists because the new code will fail even in the special case
mentioned above.
I wonder why we support this case at all. I think the right fix should
have been to disable this case (which was, presumably, supported by
accident because the child was added after the check).
Does anyone feel we have to maintain this special case, or is it OK to
just remove it and fix copy_test 29 to expect failure?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r14401 and copy mydir mydir
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
>
> In r14401, an issue (2224) was fixed making commands like
> svn cp wcdir wcdir
> not get stuck in an infinite loop. This was fixed by allowing this
> operation as a special case. This operation will result in a directory
> wcdir begin created as a child of wcdir (as one could expect).
[...]
> Does anyone feel we have to maintain this special case, or is it OK to
> just remove it and fix copy_test 29 to expect failure?
Half of me says "Just remove it; it's silly and shouldn't have been done."
The other half of me says: We keep on coming across situations like this, where
a silly behaviour has been in place and we'd like to remove it. Each
individual case seems fairly harmless, but if we keep on doing this we're going
to end up with a reputation for breaking our compatibility promise. We'll end
up having a Subversion v1.10 that can hardly run any of the scripts that were
written for v1.0, because of a multitude of tiny incompatibilities. Therefore
I think we need to be careful to preserve compatibility when we can.
When we do keep such a quirk for compatibility, I would like to see the main
code written cleanly, without the special behaviour, and a single, clearly
marked compatibility block that implements the special case. Also, ensure that
the documentation (e.g. "svn help", API doc-strings) clearly states that this
is a special case (probably deprecated).
In this case, the behaviour has only been in place since v1.2.0 and was only
announced as "fixed: 'svn copy dir dir' infinite recursion (issue #2224)". To
me, that doesn't imply that any particular behaviour is now specified, so maybe
we could just remove it. On the other hand we can't argue that we can remove
it because it is undocumented, because most of the behaviour of "svn copy" is
undocumented any yet we expect people to discover how it works and use it and
rely on it.
So... I don't know. I just needed to say all that. Either way will suit me in
this case.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r14401 and copy mydir mydir
Posted by John Szakmeister <jo...@szakmeister.net>.
On Thursday 26 January 2006 11:27, Julian Foad wrote:
> Peter N. Lundblad wrote:
> > On Thu, 26 Jan 2006, John Szakmeister wrote:
> >>>>Personally, I'd be fine with disabling the case, but I think we should
> >>>> be consistent and drop that ability from the RA interface as well.
> >>>
> >>>Why? In the URL case, you won't wind up with a cycle because you copy an
> >>>existing version. See also issue #1367. I think r14401 should be
> >>> reverted and the check in copy.c fixed (which it will be automatically
> >>> after my API change).
> >>
> >>Because I think it's important that we operate in very much the same way
> >>whether it's via RA or WC, where possible. Having a discrepancy like
> >> this means that you can't make these change atomically without tools
> >> like mucc--which isn't available on Windows. I don't have a strong
> >> opinion
>
> [...]
>
> > :-) But if we remove the posibility from the repository as well, you
> >
> > couldn't do this kind of things, even *with* mucc.
>
> Yes... I didn't follow that "discrepancy" sentence. The discrepancy we're
> talking about is the ability to copy into a child (or grandchild) directly
> in the repository but not in the WC, yes? Our "svn" command-line client
> can't do any but the most trivial rearrangements atomically in the
> repository anyway.
Sorry, I should've been more clear. I'd prefer that we allow the operation in
the WC rather than forbid it. I assumed that this ability was allowed for a
reason, but it may have just been an implementation detail of the RA layers
that allow this to occur there.
The command line client can most certainly do more complicated rearrangements.
I've had great success in rearranging entire source code trees, modifying the
build environment, and committing it all in one nice changeset.
> (What's mucc? A very quick Google revealed lots of University Cricket
> Clubs and the like.)
It's in the contrib area. It's a tool written by Philip Martin which allows
you to do multiple repository side copies, moves and deletes in a single
commit.
> > The problem for me is that I'll need to duplicate the logic in the
> > cmdline client in the compat wrapper, so I want to avoid making it more
> > complex than necessary. Also, to reply to Julian, I doubt that people
> > really depend on the ability to copy a WC root into itself becaus that's
> > the only special case of this that works.
>
> Right. I didn't realise that deeper copies didn't work. In that case,
> yes, it's an even sillier special case than I thought.
>
> > Removing the capability to copy an URL
> > into an descendant, however, seems more obstructive to me, so I don't
> > want to do that.
>
> I think that's a very odd capability to have. What's it for? I can't see
> any good reason to have it. I've got a feeling the reason we allow
> copy-into-itself might be just because the implementation of the repository
> made it happen naturally, and nobody thought of adding special-case code to
> forbid it.
>
> Still, I don't think that not having it on the WC is a reason to forbid it
> on the repository now.
My reasoning was consistency. If it doesn't make sense in the WC, how does it
make any more sense at the RA layer? But I guess that's not enough reason by
itself.
> > I am still for effectively reverting r14401 to avoid complexity for a
> > very special case.
>
> +1. If anyone wants to argue for adding in a full capability of WC
> copy-into-itself, that's a separate issue for later.
As I said before, I've been away from any real development for a while now.
You guys are certainly much closer to the issues. Please do as you see fit.
I was just voicing an opinion, and trying to answer Peter's original
question. :-)
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r14401 and copy mydir mydir
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Thu, 26 Jan 2006, John Szakmeister wrote:
>
>>>>Personally, I'd be fine with disabling the case, but I think we should be
>>>>consistent and drop that ability from the RA interface as well.
>>>
>>>Why? In the URL case, you won't wind up with a cycle because you copy an
>>>existing version. See also issue #1367. I think r14401 should be reverted
>>>and the check in copy.c fixed (which it will be automatically after my API
>>>change).
>>
>>Because I think it's important that we operate in very much the same way
>>whether it's via RA or WC, where possible. Having a discrepancy like this
>>means that you can't make these change atomically without tools like
>>mucc--which isn't available on Windows. I don't have a strong opinion
[...]
>
> :-) But if we remove the posibility from the repository as well, you
> couldn't do this kind of things, even *with* mucc.
Yes... I didn't follow that "discrepancy" sentence. The discrepancy we're
talking about is the ability to copy into a child (or grandchild) directly in
the repository but not in the WC, yes? Our "svn" command-line client can't do
any but the most trivial rearrangements atomically in the repository anyway.
(What's mucc? A very quick Google revealed lots of University Cricket Clubs
and the like.)
> The problem for me is that I'll need to duplicate the logic in the cmdline
> client in the compat wrapper, so I want to avoid making it more complex
> than necessary. Also, to reply to Julian, I doubt that people really
> depend on the ability to copy a WC root into itself becaus that's the only
> special case of this that works.
Right. I didn't realise that deeper copies didn't work. In that case, yes,
it's an even sillier special case than I thought.
> Removing the capability to copy an URL
> into an descendant, however, seems more obstructive to me, so I don't want
> to do that.
I think that's a very odd capability to have. What's it for? I can't see any
good reason to have it. I've got a feeling the reason we allow
copy-into-itself might be just because the implementation of the repository
made it happen naturally, and nobody thought of adding special-case code to
forbid it.
Still, I don't think that not having it on the WC is a reason to forbid it on
the repository now.
> I am still for effectively reverting r14401 to avoid complexity for a very
> special case.
+1. If anyone wants to argue for adding in a full capability of WC
copy-into-itself, that's a separate issue for later.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r14401 and copy mydir mydir
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 26 Jan 2006, John Szakmeister wrote:
> On Thursday 26 January 2006 05:02, Peter N. Lundblad wrote:
> > On Thu, 26 Jan 2006, John Szakmeister wrote:
> > > On Wednesday 25 January 2006 17:40, Peter N. Lundblad wrote:
> > > [snip]
> > >
> > > > I wonder why we support this case at all. I think the right fix should
> > > > have been to disable this case (which was, presumably, supported by
> > > > accident because the child was added after the check).
> > >
> > > The decision was made to allow it to work for the WC because 'svn cp
> > > url://path url://path' works for all 3 RA layers. It seemed like it was
> > > an intentional feature as a result, so we moved to support the WC case
> > > rather than removing the capability from the RA layers.
> >
> > But the RA layers do support the general case, not just this special case,
> > don't they?
>
> Do you mean they support 'svn cp url://path url://path/sibling'? Hm.
> I'm not sure I've really tried that out. Doing this with file:// leads
> to interesting results though, at least if you try that with the root
> directory:
> :: svnadmin create repo
> :: mkdir -p proj/a/b/c/d
> :: svn co file://`pwd`/repo proj
> :: svn add proj/*
> A proj/a
> A proj/a/b
> A proj/a/b/c
> A proj/a/b/c/d
> :: svn ci -m '' proj
> Adding proj/a
> Adding proj/a/b
> Adding proj/a/b/c
> Adding proj/a/b/c/d
>
> Committed revision 1.
> :: svn cp file://`pwd`/repo file://`pwd`/repo/a -m ''
>
> Committed revision 2.
> :: svn ls -R file://`pwd`/repo
> a/
> a/b/
> a/b/c/
> a/b/c/d/
> a/repo/
> a/repo/a/
> a/repo/a/b/
> a/repo/a/b/c/
> a/repo/a/b/c/d/
>
> So it seems like there might be a bug in the root case ('a/repo/' should not
> be in the listing... it should be 'a/a/' . :-) So the general case seems to
> work (for file://, at least).
It is consistent with the API in that if the target exists, it will use
the basename of the source to create a child in the target. You could
argue that this should use the basename of the URL of the source, but not
if the source is the root of the FS... Well, you can always specify the
full destination name. But this is really outside of the real question.
The repository still allows copying a directory into a descendant of
itself, and it works fine.
>
> > > Personally, I'd be fine with disabling the case, but I think we should be
> > > consistent and drop that ability from the RA interface as well.
> >
> > Why? In the URL case, you won't wind up with a cycle because you copy an
> > existing version. See also issue #1367. I think r14401 should be reverted
> > and the check in copy.c fixed (which it will be automatically after my API
> > change).
>
> Because I think it's important that we operate in very much the same way
> whether it's via RA or WC, where possible. Having a discrepancy like this
> means that you can't make these change atomically without tools like
> mucc--which isn't available on Windows. I don't have a strong opinion
> whether or not copy succeeds or fails in this case, but I'd prefer we be
> consistent. Seeing as how you're contributing much more to the project than
> I am these days, I'd say you're in a better position to make the judgment
> call. :-)
>
:-) But if we remove the posibility from the repository as well, you
couldn't do this kind of things, even *with* mucc.
The problem for me is that I'll need to duplicate the logic in the cmdline
client in the compat wrapper, so I want to avoid making it more complex
than necessary. Also, to reply to Julian, I doubt that people really
depend on the ability to copy a WC root into itself becaus that's the only
special case of this that works. Removing the capability to copy an URL
into an descendant, however, seems more obstructive to me, so I don't want
to do that.
I am still for effectively reverting r14401 to avoid complexity for a very
special case.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r14401 and copy mydir mydir
Posted by John Szakmeister <jo...@szakmeister.net>.
On Thursday 26 January 2006 05:02, Peter N. Lundblad wrote:
> On Thu, 26 Jan 2006, John Szakmeister wrote:
> > On Wednesday 25 January 2006 17:40, Peter N. Lundblad wrote:
> > [snip]
> >
> > > I wonder why we support this case at all. I think the right fix should
> > > have been to disable this case (which was, presumably, supported by
> > > accident because the child was added after the check).
> >
> > The decision was made to allow it to work for the WC because 'svn cp
> > url://path url://path' works for all 3 RA layers. It seemed like it was
> > an intentional feature as a result, so we moved to support the WC case
> > rather than removing the capability from the RA layers.
>
> But the RA layers do support the general case, not just this special case,
> don't they?
Do you mean they support 'svn cp url://path url://path/sibling'? Hm. I'm not
sure I've really tried that out. Doing this with file:// leads to
interesting results though, at least if you try that with the root directory:
:: svnadmin create repo
:: mkdir -p proj/a/b/c/d
:: svn co file://`pwd`/repo proj
:: svn add proj/*
A proj/a
A proj/a/b
A proj/a/b/c
A proj/a/b/c/d
:: svn ci -m '' proj
Adding proj/a
Adding proj/a/b
Adding proj/a/b/c
Adding proj/a/b/c/d
Committed revision 1.
:: svn cp file://`pwd`/repo file://`pwd`/repo/a -m ''
Committed revision 2.
:: svn ls -R file://`pwd`/repo
a/
a/b/
a/b/c/
a/b/c/d/
a/repo/
a/repo/a/
a/repo/a/b/
a/repo/a/b/c/
a/repo/a/b/c/d/
So it seems like there might be a bug in the root case ('a/repo/' should not
be in the listing... it should be 'a/a/' . :-) So the general case seems to
work (for file://, at least).
> > Personally, I'd be fine with disabling the case, but I think we should be
> > consistent and drop that ability from the RA interface as well.
>
> Why? In the URL case, you won't wind up with a cycle because you copy an
> existing version. See also issue #1367. I think r14401 should be reverted
> and the check in copy.c fixed (which it will be automatically after my API
> change).
Because I think it's important that we operate in very much the same way
whether it's via RA or WC, where possible. Having a discrepancy like this
means that you can't make these change atomically without tools like
mucc--which isn't available on Windows. I don't have a strong opinion
whether or not copy succeeds or fails in this case, but I'd prefer we be
consistent. Seeing as how you're contributing much more to the project than
I am these days, I'd say you're in a better position to make the judgment
call. :-)
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r14401 and copy mydir mydir
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 26 Jan 2006, John Szakmeister wrote:
> On Wednesday 25 January 2006 17:40, Peter N. Lundblad wrote:
> [snip]
> > I wonder why we support this case at all. I think the right fix should
> > have been to disable this case (which was, presumably, supported by
> > accident because the child was added after the check).
>
> The decision was made to allow it to work for the WC because 'svn cp
> url://path url://path' works for all 3 RA layers. It seemed like it was an
> intentional feature as a result, so we moved to support the WC case rather
> than removing the capability from the RA layers.
>
But the RA layers do support the general case, not just this special case,
don't they?
> Personally, I'd be fine with disabling the case, but I think we should be
> consistent and drop that ability from the RA interface as well.
>
Why? In the URL case, you won't wind up with a cycle because you copy an
existing version. See also issue #1367. I think r14401 should be reverted
and the check in copy.c fixed (which it will be automatically after my API
change).
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r14401 and copy mydir mydir
Posted by John Szakmeister <jo...@szakmeister.net>.
On Wednesday 25 January 2006 17:40, Peter N. Lundblad wrote:
[snip]
> I wonder why we support this case at all. I think the right fix should
> have been to disable this case (which was, presumably, supported by
> accident because the child was added after the check).
The decision was made to allow it to work for the WC because 'svn cp
url://path url://path' works for all 3 RA layers. It seemed like it was an
intentional feature as a result, so we moved to support the WC case rather
than removing the capability from the RA layers.
Personally, I'd be fine with disabling the case, but I think we should be
consistent and drop that ability from the RA interface as well.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org