You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2012/07/18 09:47:52 UTC
RE: svn commit: r1362739 - /subversion/trunk/subversion/tests/cmdline/basic_tests.py
> -----Original Message-----
> From: brane@apache.org [mailto:brane@apache.org]
> Sent: woensdag 18 juli 2012 03:46
> To: commits@subversion.apache.org
> Subject: svn commit: r1362739 -
> /subversion/trunk/subversion/tests/cmdline/basic_tests.py
>
> Author: brane
> Date: Wed Jul 18 01:46:05 2012
> New Revision: 1362739
>
> URL: http://svn.apache.org/viewvc?rev=1362739&view=rev
> Log:
> Add some XFAIL tests for issue #4193.
>
> * subversion/tests/cmdline/basic_tests.py
> (status_through_unversioned_symlink,
> status_through_versioned_symlink,
> add_through_unversioned_symlink, add_through_versioned_symlink):
> New tests.
>
> Modified:
> subversion/trunk/subversion/tests/cmdline/basic_tests.py
>
> Modified: subversion/trunk/subversion/tests/cmdline/basic_tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/
> basic_tests.py?rev=1362739&r1=1362738&r2=1362739&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/tests/cmdline/basic_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/basic_tests.py Wed Jul 18
> 01:46:05 2012
> @@ -2983,6 +2983,54 @@ def delete_conflicts_one_of_many(sbox):
> verify_file_deleted("failed to remove conflict file",
> sbox.ospath('A/D/G/rho.mine'))
>
> +@XFail()
> +@Issue(4193)
> +@SkipUnless(svntest.main.is_posix_os)
> +def status_through_unversioned_symlink(sbox):
> + """file status through unversioned symlink"""
> +
> + sbox.build(read_only = True)
> + state = svntest.actions.get_virginal_state(sbox.wc_dir, 1)
> + os.symlink('A', sbox.ospath('Z'))
> + svntest.actions.run_and_verify_status(sbox.ospath('Z/mu'), state)
> +
> +@XFail()
> +@Issue(4193)
> +@SkipUnless(svntest.main.is_posix_os)
> +def status_through_versioned_symlink(sbox):
> + """file status through versioned symlink"""
> +
> + sbox.build()
> + state = svntest.actions.get_virginal_state(sbox.wc_dir, 1)
> + os.symlink('A', sbox.ospath('Z'))
> + sbox.simple_add('Z')
> + state.add({'Z': Item(status='A ')})
> + svntest.actions.run_and_verify_status(sbox.ospath('Z/mu'), state)
> +
> +@XFail()
> +@Issue(4193)
> +@SkipUnless(svntest.main.is_posix_os)
> +def add_through_unversioned_symlink(sbox):
> + """add file through unversioned symlink"""
> +
> + sbox.build()
> + os.symlink('A', sbox.ospath('Z'))
> + sbox.simple_append('A/kappa', 'xyz', True)
> + sbox.simple_add('Z/kappa')
> +
> +@XFail()
> +@Issue(4193)
> +@SkipUnless(svntest.main.is_posix_os)
> +def add_through_versioned_symlink(sbox):
> + """add file through versioned symlink"""
> +
> + sbox.build()
> + os.symlink('A', sbox.ospath('Z'))
> + sbox.simple_add('Z')
> + sbox.simple_append('A/kappa', 'xyz', True)
> + sbox.simple_add('Z/kappa')
> +
> +
All these test, exercise exactly the same code in libsvn_wc, where we transform an absolute path into a wcroot, working copy relative path by looking it up in the database.
In 1.6 all these cases worked because we found a .svn directory with metadata in the immediate parent (=the symlinked directory).
I'm not sure if we really want to support all these corner cases (including those where this symlink is versioned itself). We never intentionally broke this feature.
I think we can only fix this problem by starting reading symlink targets and handle/parse them ourself. Which is something we never did in the generic case before and which will probably open a lot of new issues.
For this class of issues I would have used one of the less common *_tests.py files than basic_tests, as that is also used by many as the light version of running our entire test suite.
sbox has a few optional arguments that would make the tests cheaper to run, by not duplicating the repository etc. These would apply to all 4 tests.
Bert
Re: svn commit: r1362739 - /subversion/trunk/subversion/tests/cmdline/basic_tests.py
Posted by Branko Čibej <br...@wandisco.com>.
On 18.07.2012 09:47, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: brane@apache.org [mailto:brane@apache.org]
>> Sent: woensdag 18 juli 2012 03:46
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1362739 -
>> /subversion/trunk/subversion/tests/cmdline/basic_tests.py
>>
>> Author: brane
>> Date: Wed Jul 18 01:46:05 2012
>> New Revision: 1362739
>>
>> URL: http://svn.apache.org/viewvc?rev=1362739&view=rev
>> Log:
>> Add some XFAIL tests for issue #4193.
>>
>> * subversion/tests/cmdline/basic_tests.py
>> (status_through_unversioned_symlink,
>> status_through_versioned_symlink,
>> add_through_unversioned_symlink, add_through_versioned_symlink):
>> New tests.
>>
>> Modified:
>> subversion/trunk/subversion/tests/cmdline/basic_tests.py
>>
>> Modified: subversion/trunk/subversion/tests/cmdline/basic_tests.py
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/
>> basic_tests.py?rev=1362739&r1=1362738&r2=1362739&view=diff
>> ==========================================================
>> ====================
>> --- subversion/trunk/subversion/tests/cmdline/basic_tests.py (original)
>> +++ subversion/trunk/subversion/tests/cmdline/basic_tests.py Wed Jul 18
>> 01:46:05 2012
>> @@ -2983,6 +2983,54 @@ def delete_conflicts_one_of_many(sbox):
>> verify_file_deleted("failed to remove conflict file",
>> sbox.ospath('A/D/G/rho.mine'))
>>
>> +@XFail()
>> +@Issue(4193)
>> +@SkipUnless(svntest.main.is_posix_os)
>> +def status_through_unversioned_symlink(sbox):
>> + """file status through unversioned symlink"""
>> +
>> + sbox.build(read_only = True)
>> + state = svntest.actions.get_virginal_state(sbox.wc_dir, 1)
>> + os.symlink('A', sbox.ospath('Z'))
>> + svntest.actions.run_and_verify_status(sbox.ospath('Z/mu'), state)
>> +
>> +@XFail()
>> +@Issue(4193)
>> +@SkipUnless(svntest.main.is_posix_os)
>> +def status_through_versioned_symlink(sbox):
>> + """file status through versioned symlink"""
>> +
>> + sbox.build()
>> + state = svntest.actions.get_virginal_state(sbox.wc_dir, 1)
>> + os.symlink('A', sbox.ospath('Z'))
>> + sbox.simple_add('Z')
>> + state.add({'Z': Item(status='A ')})
>> + svntest.actions.run_and_verify_status(sbox.ospath('Z/mu'), state)
>> +
>> +@XFail()
>> +@Issue(4193)
>> +@SkipUnless(svntest.main.is_posix_os)
>> +def add_through_unversioned_symlink(sbox):
>> + """add file through unversioned symlink"""
>> +
>> + sbox.build()
>> + os.symlink('A', sbox.ospath('Z'))
>> + sbox.simple_append('A/kappa', 'xyz', True)
>> + sbox.simple_add('Z/kappa')
>> +
>> +@XFail()
>> +@Issue(4193)
>> +@SkipUnless(svntest.main.is_posix_os)
>> +def add_through_versioned_symlink(sbox):
>> + """add file through versioned symlink"""
>> +
>> + sbox.build()
>> + os.symlink('A', sbox.ospath('Z'))
>> + sbox.simple_add('Z')
>> + sbox.simple_append('A/kappa', 'xyz', True)
>> + sbox.simple_add('Z/kappa')
>> +
>> +
> All these test, exercise exactly the same code in libsvn_wc, where we transform an absolute path into a wcroot, working copy relative path by looking it up in the database.
I figured as much, but I wanted the tests in first. Though you'll notice
that the error messages are different for status, add and add with
versioned symlink.
> In 1.6 all these cases worked because we found a .svn directory with metadata in the immediate parent (=the symlinked directory).
Yup, that's obvious.
> I'm not sure if we really want to support all these corner cases (including those where this symlink is versioned itself). We never intentionally broke this feature.
>
>
> I think we can only fix this problem by starting reading symlink targets and handle/parse them ourself. Which is something we never did in the generic case before and which will probably open a lot of new issues.
I actually wanted to start this discussion. This is clearly a regression
from 1.6, and whilst we didn't intentionally break the feature and
didn't intentionally support it in 1.6, the fact remains that --
apparently -- people are using it.
The question now is, what to do? We probably don't want to always
normalize the targets, since the performance hit would be noticeable. On
the other hand, normalizing explicit targets when they cannot be
resolved will only affect users who actually do this.
> For this class of issues I would have used one of the less common *_tests.py files than basic_tests, as that is also used by many as the light version of running our entire test suite.
>
> sbox has a few optional arguments that would make the tests cheaper to run, by not duplicating the repository etc. These would apply to all 4 tests.
I'll look into that, thanks for the pointer. And I'll move these four
tests to a new file, wc_tests.py.
-- Brane
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1362739 - /subversion/trunk/subversion/tests/cmdline/basic_tests.py
Posted by Branko Čibej <br...@wandisco.com>.
On 18.07.2012 12:25, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Branko Čibej [mailto:brane@wandisco.com]
>> Sent: woensdag 18 juli 2012 11:50
>> To: dev@subversion.apache.org
>> Subject: Re: svn commit: r1362739 -
>> /subversion/trunk/subversion/tests/cmdline/basic_tests.py
>>
>> On 18.07.2012 10:51, Philip Martin wrote:
>>> "Bert Huijben" <be...@qqmail.nl> writes:
>>>
>>>> In 1.6 all these cases worked because we found a .svn directory with
>>>> metadata in the immediate parent (=the symlinked directory).
>>>>
>>>> I'm not sure if we really want to support all these corner cases
>>>> (including those where this symlink is versioned itself). We never
>>>> intentionally broke this feature.
>>> The 1.6 behaviour could be considered a bug.
>> That's a bit too strong, IMO. The fact is that, up to 1.6, every
>> directory in the working copy was effectively a working copy root, and
>> we even had code that specifically made it possible to acces a WC root
>> through a symbolic link (the main use case was wc-root-on-subst-drive on
>> Windows). So it's reasonable to interpret this as supported behaviour,
>> even if we never intended it.
>>
>>> Following unversioned
>>> symlinks to the root of a working copy is probably simple enough, but
>>> following unversioned symlinks inside a working copy looks far harder.
>>>
>>> svnadmin create repo
>>> svn import -mm repo/format file://`pwd`/repo/A/f
>>> svn co file://`pwd`/repo wc
>>> ln -s f wc/A/g1
>>> ln -s A wc/B1
>>> svn add wc/A/g1 wc/B1
>>> svn ci -mm wc
>>> ln -s f wc/A/g2
>>> ln -s A wc/B2
>>>
>>> wc/A/f is a versioned file; the content can be accessed through a
>>> variety of paths containing symlinks: wc/A/g1, wc/A/g2, wc/B1/f,
>>> wc/B2/f, wc/B1/g1, wc/B1/g2, wc/B2/g1 and wc/B2/g2. Some of the
>>> symlinks are versioned and some are unversioned.
>>>
>>> Now consider:
>>>
>>> echo new-f > wc/A/f
>>> svn st wc/A/g1
>>> svn ci -mm wc/A/g1
>>>
>>> wc/A/g1 is versioned so status shows the status of wc/A/g1 not wc/A/f
>>> and commit does nothing unless wc/A/g1 is changed. That behaviour looks
>>> correct to me.
>> Yes. This is the tricky part. IMO the only rational thing to do is to
>> never try to resolve symlinks if the original target resolves to a known
>> node. So in the commit case the user might be surprised by the different
>> behaviour if the symlink is versioned or not, but at least the
>> difference can be explained.
>>
>>> What about the unversioned wc/A/g2? The new 4193 tests seem to
>> assume
>>> that Subversion operations should follow this symlink through to wc/A/f.
>>> Is it sensible for operations on an unversioned symlink to affect the
>>> target while the same operations on a versioned symlink affect the
>>> source? That strikes me as very confusing.
>>>
>>> How should wc/B2/g1 behave? wc/B2 is unversioned but wc/B2/g1 points
>> to
>>> wc/A/g1 which is versioned. Do operations on wc/B2/g1 affect wc/A/f?
>> I think the only thing we could do is to resolve symlinks in explicit
>> targets iff the original path does not resolve to a known versioned
>> node. That would isolate the extra work in the error case and would not
>> change the behaviour of cases that currently work, nor would it change
>> the way recursive operations work.
>>
>> I also wouldn't worry about the paths in notification callbacks but
>> would just use the resolved paths everywhere; e.g., if "svn ci x/f" gets
>> translated to "svn ci a/f", I wouldn't try to show x/f in the commit
>> notification.
> That is nice behavior in theory, but doesn't really work in many use cases. Scripted users of the client library (or 'svn') expect to see the paths they passed in the results they get. E.g. If you perform a query and expect the result in a hashtable, you don't want the key you need to fetch the item to change.
>
> Another problem is that you can have one path in several formats in the target list of an operation, while we only filter duplicates (like in commit)
>
> libsvn_wc assumes that all paths are unique in their canonical absolute format. Converting from one absolute path to another in the lowest layer breaks all assumptions the higher layers make about the library behavior.
> Storing multiple wcroots (after resolving symlinks) doesn't break things, but when we have the (wcroot, local_relpath) pair things will break.
Good point, I hadn't considered that. So this is looking more and more
like a "wontfix".
> The other solution, trying to resolve when parsing the arguments in 'svn' won't work because we at that point don't know what the arguments are used for yet.
Yes, that's clear; we'd have to at least look at what the arguments
resolve to, and that would entail extra up-front work that I wouldn't
want to see in the code.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
RE: svn commit: r1362739 - /subversion/trunk/subversion/tests/cmdline/basic_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: woensdag 18 juli 2012 11:50
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1362739 -
> /subversion/trunk/subversion/tests/cmdline/basic_tests.py
>
> On 18.07.2012 10:51, Philip Martin wrote:
> > "Bert Huijben" <be...@qqmail.nl> writes:
> >
> >> In 1.6 all these cases worked because we found a .svn directory with
> >> metadata in the immediate parent (=the symlinked directory).
> >>
> >> I'm not sure if we really want to support all these corner cases
> >> (including those where this symlink is versioned itself). We never
> >> intentionally broke this feature.
> > The 1.6 behaviour could be considered a bug.
>
> That's a bit too strong, IMO. The fact is that, up to 1.6, every
> directory in the working copy was effectively a working copy root, and
> we even had code that specifically made it possible to acces a WC root
> through a symbolic link (the main use case was wc-root-on-subst-drive on
> Windows). So it's reasonable to interpret this as supported behaviour,
> even if we never intended it.
>
> > Following unversioned
> > symlinks to the root of a working copy is probably simple enough, but
> > following unversioned symlinks inside a working copy looks far harder.
> >
> > svnadmin create repo
> > svn import -mm repo/format file://`pwd`/repo/A/f
> > svn co file://`pwd`/repo wc
> > ln -s f wc/A/g1
> > ln -s A wc/B1
> > svn add wc/A/g1 wc/B1
> > svn ci -mm wc
> > ln -s f wc/A/g2
> > ln -s A wc/B2
> >
> > wc/A/f is a versioned file; the content can be accessed through a
> > variety of paths containing symlinks: wc/A/g1, wc/A/g2, wc/B1/f,
> > wc/B2/f, wc/B1/g1, wc/B1/g2, wc/B2/g1 and wc/B2/g2. Some of the
> > symlinks are versioned and some are unversioned.
> >
> > Now consider:
> >
> > echo new-f > wc/A/f
> > svn st wc/A/g1
> > svn ci -mm wc/A/g1
> >
> > wc/A/g1 is versioned so status shows the status of wc/A/g1 not wc/A/f
> > and commit does nothing unless wc/A/g1 is changed. That behaviour looks
> > correct to me.
>
> Yes. This is the tricky part. IMO the only rational thing to do is to
> never try to resolve symlinks if the original target resolves to a known
> node. So in the commit case the user might be surprised by the different
> behaviour if the symlink is versioned or not, but at least the
> difference can be explained.
>
> > What about the unversioned wc/A/g2? The new 4193 tests seem to
> assume
> > that Subversion operations should follow this symlink through to wc/A/f.
> > Is it sensible for operations on an unversioned symlink to affect the
> > target while the same operations on a versioned symlink affect the
> > source? That strikes me as very confusing.
> >
> > How should wc/B2/g1 behave? wc/B2 is unversioned but wc/B2/g1 points
> to
> > wc/A/g1 which is versioned. Do operations on wc/B2/g1 affect wc/A/f?
>
> I think the only thing we could do is to resolve symlinks in explicit
> targets iff the original path does not resolve to a known versioned
> node. That would isolate the extra work in the error case and would not
> change the behaviour of cases that currently work, nor would it change
> the way recursive operations work.
>
> I also wouldn't worry about the paths in notification callbacks but
> would just use the resolved paths everywhere; e.g., if "svn ci x/f" gets
> translated to "svn ci a/f", I wouldn't try to show x/f in the commit
> notification.
That is nice behavior in theory, but doesn't really work in many use cases. Scripted users of the client library (or 'svn') expect to see the paths they passed in the results they get. E.g. If you perform a query and expect the result in a hashtable, you don't want the key you need to fetch the item to change.
Another problem is that you can have one path in several formats in the target list of an operation, while we only filter duplicates (like in commit)
libsvn_wc assumes that all paths are unique in their canonical absolute format. Converting from one absolute path to another in the lowest layer breaks all assumptions the higher layers make about the library behavior.
Storing multiple wcroots (after resolving symlinks) doesn't break things, but when we have the (wcroot, local_relpath) pair things will break.
The other solution, trying to resolve when parsing the arguments in 'svn' won't work because we at that point don't know what the arguments are used for yet.
Bert
Re: svn commit: r1362739 - /subversion/trunk/subversion/tests/cmdline/basic_tests.py
Posted by Branko Čibej <br...@wandisco.com>.
On 18.07.2012 10:51, Philip Martin wrote:
> "Bert Huijben" <be...@qqmail.nl> writes:
>
>> In 1.6 all these cases worked because we found a .svn directory with
>> metadata in the immediate parent (=the symlinked directory).
>>
>> I'm not sure if we really want to support all these corner cases
>> (including those where this symlink is versioned itself). We never
>> intentionally broke this feature.
> The 1.6 behaviour could be considered a bug.
That's a bit too strong, IMO. The fact is that, up to 1.6, every
directory in the working copy was effectively a working copy root, and
we even had code that specifically made it possible to acces a WC root
through a symbolic link (the main use case was wc-root-on-subst-drive on
Windows). So it's reasonable to interpret this as supported behaviour,
even if we never intended it.
> Following unversioned
> symlinks to the root of a working copy is probably simple enough, but
> following unversioned symlinks inside a working copy looks far harder.
>
> svnadmin create repo
> svn import -mm repo/format file://`pwd`/repo/A/f
> svn co file://`pwd`/repo wc
> ln -s f wc/A/g1
> ln -s A wc/B1
> svn add wc/A/g1 wc/B1
> svn ci -mm wc
> ln -s f wc/A/g2
> ln -s A wc/B2
>
> wc/A/f is a versioned file; the content can be accessed through a
> variety of paths containing symlinks: wc/A/g1, wc/A/g2, wc/B1/f,
> wc/B2/f, wc/B1/g1, wc/B1/g2, wc/B2/g1 and wc/B2/g2. Some of the
> symlinks are versioned and some are unversioned.
>
> Now consider:
>
> echo new-f > wc/A/f
> svn st wc/A/g1
> svn ci -mm wc/A/g1
>
> wc/A/g1 is versioned so status shows the status of wc/A/g1 not wc/A/f
> and commit does nothing unless wc/A/g1 is changed. That behaviour looks
> correct to me.
Yes. This is the tricky part. IMO the only rational thing to do is to
never try to resolve symlinks if the original target resolves to a known
node. So in the commit case the user might be surprised by the different
behaviour if the symlink is versioned or not, but at least the
difference can be explained.
> What about the unversioned wc/A/g2? The new 4193 tests seem to assume
> that Subversion operations should follow this symlink through to wc/A/f.
> Is it sensible for operations on an unversioned symlink to affect the
> target while the same operations on a versioned symlink affect the
> source? That strikes me as very confusing.
>
> How should wc/B2/g1 behave? wc/B2 is unversioned but wc/B2/g1 points to
> wc/A/g1 which is versioned. Do operations on wc/B2/g1 affect wc/A/f?
I think the only thing we could do is to resolve symlinks in explicit
targets iff the original path does not resolve to a known versioned
node. That would isolate the extra work in the error case and would not
change the behaviour of cases that currently work, nor would it change
the way recursive operations work.
I also wouldn't worry about the paths in notification callbacks but
would just use the resolved paths everywhere; e.g., if "svn ci x/f" gets
translated to "svn ci a/f", I wouldn't try to show x/f in the commit
notification.
-- Brane
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1362739 - /subversion/trunk/subversion/tests/cmdline/basic_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> In 1.6 all these cases worked because we found a .svn directory with
> metadata in the immediate parent (=the symlinked directory).
>
> I'm not sure if we really want to support all these corner cases
> (including those where this symlink is versioned itself). We never
> intentionally broke this feature.
The 1.6 behaviour could be considered a bug. Following unversioned
symlinks to the root of a working copy is probably simple enough, but
following unversioned symlinks inside a working copy looks far harder.
svnadmin create repo
svn import -mm repo/format file://`pwd`/repo/A/f
svn co file://`pwd`/repo wc
ln -s f wc/A/g1
ln -s A wc/B1
svn add wc/A/g1 wc/B1
svn ci -mm wc
ln -s f wc/A/g2
ln -s A wc/B2
wc/A/f is a versioned file; the content can be accessed through a
variety of paths containing symlinks: wc/A/g1, wc/A/g2, wc/B1/f,
wc/B2/f, wc/B1/g1, wc/B1/g2, wc/B2/g1 and wc/B2/g2. Some of the
symlinks are versioned and some are unversioned.
Now consider:
echo new-f > wc/A/f
svn st wc/A/g1
svn ci -mm wc/A/g1
wc/A/g1 is versioned so status shows the status of wc/A/g1 not wc/A/f
and commit does nothing unless wc/A/g1 is changed. That behaviour looks
correct to me.
What about the unversioned wc/A/g2? The new 4193 tests seem to assume
that Subversion operations should follow this symlink through to wc/A/f.
Is it sensible for operations on an unversioned symlink to affect the
target while the same operations on a versioned symlink affect the
source? That strikes me as very confusing.
How should wc/B2/g1 behave? wc/B2 is unversioned but wc/B2/g1 points to
wc/A/g1 which is versioned. Do operations on wc/B2/g1 affect wc/A/f?
--
Cerified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download