You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2012/11/08 03:19:27 UTC
Re: svn commit: r1406870 - in /subversion/trunk/subversion: libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c tests/cmdline/special_tests.py
philip@apache.org writes:
> Author: philip
> Date: Wed Nov 7 23:49:06 2012
> New Revision: 1406870
>
> URL: http://svn.apache.org/viewvc?rev=1406870&view=rev
> Log:
> Fix issue 4091, symlink-ness change causes spurious tree-conflict.
>
> * subversion/libsvn_wc/update_editor.c
> (close_file): Only flag symlink-ness conflict if locally modified.
>
> * subversion/libsvn_wc/wc_db_wcroot.c
> (svn_wc__db_wcroot_parse_local_abspath): Only close db if really opened.
>
> * subversion/tests/cmdline/special_tests.py
> (replace_symlinks): Deepcopy the r2 status for later use, remove XFAIL.
I thought this was a simple fix but it turns out to be complicated.
Issue 4091 involves a file becoming a symlink, or a symlink becoming a
file, without a 'R'eplace, i.e. a text and property change but no
delete/add. If the file/symlink is not locally modified then update
should simply change the node from one to the other, and that's what the
code now does. But if the file/symlink is modified the code raises a
tree-conflict and I think that may be a problem.
Symlinks are text files in the repository so symlink conflicts are
really text-conflicts. If r3 has f -> g and r4 has f -> h and I have
local a modification f -> i then an update from r3 to r4 produces a
conflict (although the results are not pretty):
$ svn st wc
C wc/f
? wc/f.mine
? wc/f.r3
? wc/f.r4
Summary of conflicts:
Text conflicts: 1
$ ls -l wc
-rw-r--r-- 1 pm pm 13 Nov 8 02:13 f
lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.mine -> i
lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.r3 -> g
lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.r4 -> h
The handling of symlink conflicts could be improved, but fundamentally
they are text-conflicts.
So from an implementation point of view file-symlink conflicts are also
text-conflicts. Should we be marking such conflicts as tree-conflicts
or text-conflicts?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1406870 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
tests/cmdline/special_tests.py
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Thu, Nov 08, 2012 at 10:01:18 +0100:
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> > Sent: donderdag 8 november 2012 08:10
> > To: dev@subversion.apache.org
> > Subject: Re: svn commit: r1406870 - in /subversion/trunk/subversion:
> > libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
> > tests/cmdline/special_tests.py
>
> > BTW, it would be nice if the recipe for creating versioned symlinks on
> > Windows worked on Unixes too. For example, the 'svn add --with-revprop'
> > command could replace 'foo' --- which would be a normal file at that
> > point --- with an actual symlink.
>
> With 'revprop'?
>
> What?
>
> svn:special is not a revision property.
Doh. The point was to atomically ('svn add' and 'mark as
svn:special=symlink') --- obviously I got the --option name wrongly.
>
> Bert
>
RE: svn commit: r1406870 - in /subversion/trunk/subversion: libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c tests/cmdline/special_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: donderdag 8 november 2012 08:10
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1406870 - in /subversion/trunk/subversion:
> libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
> tests/cmdline/special_tests.py
> BTW, it would be nice if the recipe for creating versioned symlinks on
> Windows worked on Unixes too. For example, the 'svn add --with-revprop'
> command could replace 'foo' --- which would be a normal file at that
> point --- with an actual symlink.
With 'revprop'?
What?
svn:special is not a revision property.
Bert
Re: svn commit: r1406870 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
tests/cmdline/special_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Thu, Nov 08, 2012 at 09:07:07 +0200:
> Branko Čibej wrote on Thu, Nov 08, 2012 at 06:13:22 +0100:
> > On 08.11.2012 05:26, Daniel Shahaf wrote:
> > > Branko Čibej wrote on Thu, Nov 08, 2012 at 04:13:52 +0100:
> > >> I believe that the correct approach would be to always treat a changed
> > >> node kind (that's either the appearance/disappearance of the svn:symlink
> > >> property, or a change of the initial keyword in the special-file
> > >> contents) as if it were a replacement, for the purpose of conflict
> > >> detection and resolution, even though the node didn't actually get
> > >> replaced.
> > > Should we allow nodes to change their special-ness (namely: whether
> > > they have svn:special set, and if yes what's the initial keyword)
> > > without a replace?
> > >
> > > i.e., sure, current clients can do that --- "svn ps svn:special yes
> > > COMMITTERS" --- so we'll have to handle that in libsvn_wc. But maybe we
> > > shouldn't allow any more instances of that.
> >
> > Good point. It might be a good idea to simply forbid setting or deleting
> > svn:special explicitly, and also refusing to commit the file if its
> > contents were modified in a way that changes the special type.
> >
> > Effectively that means you could only create special files through
> > indirect means, e.g., by "svn add"ing a symlink.
> >
> > That wouldn't remove the work needed to fix the tree conflict bug, but
> > it would make the svn:special semantics clearer. I personally don't
> > think we have to worry about backward compatibility at that level; I'd
> > rather treat the fact that you can directly manipulate svn:special as a bug.
> >
>
> I would agree that being able to add/rm svn:special on a file node _that
> already exists in the repository_ is a bug. But being able to do that
> on a locally-added file is a feature --- it's what allows Windows users
> to create versioned symlinks:
>
> printf "link bar" > foo && svn add foo && svn ps svn:special yes foo && svn ci
>
> If we don't like changing the specialness of a local addition, we could
> deprecate (or break) that behaviour and have people run
> 'svn add --with-revprop svn:special=yes foo' instead.
BTW, it would be nice if the recipe for creating versioned symlinks on
Windows worked on Unixes too. For example, the 'svn add --with-revprop'
command could replace 'foo' --- which would be a normal file at that
point --- with an actual symlink.
Re: svn commit: r1406870 - in /subversion/trunk/subversion: libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c tests/cmdline/special_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> +1 on not allowing to change the symlinkness of existing files.
> And +1 on still allowing it on *new* files.
>
> In the update editor we could change the behavior of incoming symlink
> changes to be handled as a delete + add, introducing a tree conflict
> (and a local replacement of the incoming node to keep the original
> state) if there are local changes.
To my surprise we have a related problem with file-directory changes:
$ rm wc/file
$ mkdir wc/file
$ svn st wc
~ wc/file
$ svn up wc # attempts to modify text of wc/file
U wc/file
../src/subversion/svn/update-cmd.c:168: (apr_err=21)
../src/subversion/libsvn_client/update.c:639: (apr_err=21)
../src/subversion/libsvn_client/update.c:579: (apr_err=21)
../src/subversion/libsvn_client/update.c:440: (apr_err=21)
../src/subversion/libsvn_wc/adm_crawler.c:858: (apr_err=21)
../src/subversion/libsvn_repos/reporter.c:1541: (apr_err=21)
../src/subversion/libsvn_repos/reporter.c:1456: (apr_err=21)
../src/subversion/libsvn_wc/update_editor.c:2748: (apr_err=21)
../src/subversion/libsvn_wc/workqueue.c:1491: (apr_err=21)
../src/subversion/libsvn_wc/workqueue.c:1409: (apr_err=21)
../src/subversion/libsvn_wc/workqueue.c:628: (apr_err=21)
../src/subversion/libsvn_subr/io.c:3559: (apr_err=21)
svn: E000021: Can't move '/home/pm/sw/subversion/obj/wc/.svn/tmp/svn-KLx0k0' to '/home/pm/sw/subversion/obj/wc/file': Is a directory
I thought we addressed things like this in 1.7 but I see we didn't
finish it. The obstruction prevents us updating the working file but it
is entirely possible for the base tree to be updated. We probably don't
need either a tree- or text- conflict here. If we simpy skip updating
the file the obstruction will continue to be marked ~ in status.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
RE: svn commit: r1406870 - in /subversion/trunk/subversion: libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c tests/cmdline/special_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: donderdag 8 november 2012 13:31
> To: Bert Huijben
> Cc: 'Daniel Shahaf'; dev@subversion.apache.org
> Subject: Re: svn commit: r1406870 - in /subversion/trunk/subversion:
> libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
> tests/cmdline/special_tests.py
>
> "Bert Huijben" <be...@qqmail.nl> writes:
>
> > In the update editor we could change the behavior of incoming symlink
> > changes to be handled as a delete + add, introducing a tree conflict
> > (and a local replacement of the incoming node to keep the original
> > state) if there are local changes.
> >
> > I think I should be able to introduce this update editor behavior
> > easily, but it is hard for me to test the on-disk symlink behavior. So
> > I might break a few buildbot runs while working on this.
>
> We are currently adding a tree conflcit but we are not doing a good job:
>
> $ svn up wc --accept postpone
> Updating 'wc':
> C wc/f
> At revision 2.
> Summary of conflicts:
> Tree conflicts: 1
> $ svn st wc
> D C wc/f
> > local add, incoming add upon update
> Summary of conflicts:
> Tree conflicts: 1
>
> We have flagged an add-add conflict but it's more of an update-update
> conflict. Also the working state is 'D' and that is probably never what
> the user wants. To get to state 'R' the user must resolve the tree
> conflict before doing the extra add.
After r1407131 you would get something like:
<< s-reverse and s-type are the changes from file into symlink and the other
way >>
$ svn up svn-test-work\working_copies\special_tests-25
Updating 'svn-test-work\working_copies\special_tests-25':
U svn-test-work\working_copies\special_tests-25\s-in-place
C svn-test-work\working_copies\special_tests-25\s-replace
A svn-test-work\working_copies\special_tests-25\s-replace
C svn-test-work\working_copies\special_tests-25\s-reverse
C svn-test-work\working_copies\special_tests-25\s-type
Updated to revision 5.
Summary of conflicts:
Tree conflicts: 3
$ svn status -u svn-test-work\working_copies\special_tests-25
RM + C - 2 jrandom
svn-test-work\working_copies\special_tests-25\s-replace
> local edit, incoming replace upon update
RM + C - 2 jrandom
svn-test-work\working_copies\special_tests-25\s-reverse
> local edit, incoming replace upon update
RM + C - 2 jrandom
svn-test-work\working_copies\special_tests-25\s-type
> local edit, incoming replace upon update
M 5 3 jrandom
svn-test-work\working_copies\special_tests-25\s-in-place
When the files/symlinks were locally modified before the update. (If the
files weren't modified the update just handles the update, as with a normal
replacement)
As part of this work I enabled quite a few special tests on Windows, to at
least get into these code paths from the test suite.
Bert
>
> --
> Certified & Supported Apache Subversion Downloads:
> http://www.wandisco.com/subversion/download
Re: svn commit: r1406870 - in /subversion/trunk/subversion: libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c tests/cmdline/special_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> In the update editor we could change the behavior of incoming symlink
> changes to be handled as a delete + add, introducing a tree conflict
> (and a local replacement of the incoming node to keep the original
> state) if there are local changes.
>
> I think I should be able to introduce this update editor behavior
> easily, but it is hard for me to test the on-disk symlink behavior. So
> I might break a few buildbot runs while working on this.
We are currently adding a tree conflcit but we are not doing a good job:
$ svn up wc --accept postpone
Updating 'wc':
C wc/f
At revision 2.
Summary of conflicts:
Tree conflicts: 1
$ svn st wc
D C wc/f
> local add, incoming add upon update
Summary of conflicts:
Tree conflicts: 1
We have flagged an add-add conflict but it's more of an update-update
conflict. Also the working state is 'D' and that is probably never what
the user wants. To get to state 'R' the user must resolve the tree
conflict before doing the extra add.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
RE: svn commit: r1406870 - in /subversion/trunk/subversion: libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c tests/cmdline/special_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: donderdag 8 november 2012 08:07
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1406870 - in /subversion/trunk/subversion:
> libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
> tests/cmdline/special_tests.py
>
> Branko Čibej wrote on Thu, Nov 08, 2012 at 06:13:22 +0100:
> > On 08.11.2012 05:26, Daniel Shahaf wrote:
> > > Branko Čibej wrote on Thu, Nov 08, 2012 at 04:13:52 +0100:
> > >> I believe that the correct approach would be to always treat a changed
> > >> node kind (that's either the appearance/disappearance of the
> svn:symlink
> > >> property, or a change of the initial keyword in the special-file
> > >> contents) as if it were a replacement, for the purpose of conflict
> > >> detection and resolution, even though the node didn't actually get
> > >> replaced.
> > > Should we allow nodes to change their special-ness (namely: whether
> > > they have svn:special set, and if yes what's the initial keyword)
> > > without a replace?
> > >
> > > i.e., sure, current clients can do that --- "svn ps svn:special yes
> > > COMMITTERS" --- so we'll have to handle that in libsvn_wc. But maybe
> we
> > > shouldn't allow any more instances of that.
> >
> > Good point. It might be a good idea to simply forbid setting or deleting
> > svn:special explicitly, and also refusing to commit the file if its
> > contents were modified in a way that changes the special type.
> >
> > Effectively that means you could only create special files through
> > indirect means, e.g., by "svn add"ing a symlink.
> >
> > That wouldn't remove the work needed to fix the tree conflict bug, but
> > it would make the svn:special semantics clearer. I personally don't
> > think we have to worry about backward compatibility at that level; I'd
> > rather treat the fact that you can directly manipulate svn:special as a bug.
> >
>
> I would agree that being able to add/rm svn:special on a file node _that
> already exists in the repository_ is a bug. But being able to do that
> on a locally-added file is a feature --- it's what allows Windows users
> to create versioned symlinks:
+1 on not allowing to change the symlinkness of existing files.
And +1 on still allowing it on *new* files.
In the update editor we could change the behavior of incoming symlink changes to be handled as a delete + add, introducing a tree conflict (and a local replacement of the incoming node to keep the original state) if there are local changes.
I think I should be able to introduce this update editor behavior easily, but it is hard for me to test the on-disk symlink behavior. So I might break a few buildbot runs while working on this.
Bert
Re: svn commit: r1406870 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
tests/cmdline/special_tests.py
Posted by Peter Samuelson <pe...@p12n.org>.
[Daniel Shahaf]
> Did you mean:
> $ printf 'link bar' > foo
Yes I did mean that - thanks.
> > $ svn add --config-option config:miscellany:enable-auto-props=yes \
> > --config-option config:auto-props:foo=svn:special=1 foo
> > A foo
> >
> > $ ls -l foo
> > -rw-r--r-- 1 peters peters 4 Nov 8 10:57 foo
> >
> > $ svn ci -mm
> > svn: E145001: Commit failed (details follow):
> > svn: E145001: Entry '/tmp/foowc/foo' has unexpectedly changed special status
>
> Given that "bar\n" does not start with "link ", I think this commit
> should have succeeded. (Just like the "printf 'link bar'" variant
> succeeds on windows)
Or, alternately, it should have failed because svn does not recognise
the 'bar' special type.
Anyway, we discussed this a bit on IRC and further discovered:
- 'svn add' with auto-props fails on Unix but succeeds on Windows,
presumably because the subsequent 'commit' does not expect to find a
literal symlink on Windows.
- 'svn propset svn:special' behaves the same: it does not convert a
file into a symlink, therefore on Unix the subsequent commit fails.
- For a different case, svn:executable, both 'svn add' with auto-props
and 'svn propset' have the correct side effect of making the file
executable.
So to me it looks like 'svn add' with autoprops is a working subsitute
for 'svn propset'. Apparently both methods can be used to create a
symlink in Windows. Neither can be used to create a symlink in Unix.
The bug in which setting the 'svn:special' property does not transform
a file into the appropriate special type is a different issue, not
specific to 'svn add' with auto-props. That bug should not prevent us
from restricting 'propset' / 'propedit' / 'svn propdel' of svn:special,
if we so desire.
Final point: '--auto-props --config-option config:auto-props:*=a=b' is
certainly a bit cumbersome - we may want a new convenience option
'svn add --with-prop a=b'. (Nothing to do with svn:special per se,
this could be used for any file props, as a shortcut.) But for the
present thread, creating a symlink on Windows is today is already not
very intuitive for most users, I would guess. They probably already
have to look up the 'propset' line in a tutorial or cookbook.
Peter
Re: svn commit: r1406870 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
tests/cmdline/special_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Peter Samuelson wrote on Thu, Nov 08, 2012 at 11:01:37 -0600:
>
> [Daniel Shahaf]
> > it's what allows Windows users to create versioned symlinks:
> >
> > printf "link bar" > foo && svn add foo && svn ps svn:special yes foo && svn ci
> >
> > If we don't like changing the specialness of a local addition, we could
> > deprecate (or break) that behaviour and have people run
> > 'svn add --with-revprop svn:special=yes foo' instead.
>
> (Where we understand you didn't actually mean 'revprop'.)
> We have that option already ... unfortunately it doesn't actually work:
>
> $ echo bar > foo
>
Did you mean:
$ printf 'link bar' > foo
> $ svn add --config-option config:miscellany:enable-auto-props=yes \
> --config-option config:auto-props:foo=svn:special=1 foo
> A foo
>
> $ ls -l foo
> -rw-r--r-- 1 peters peters 4 Nov 8 10:57 foo
>
> $ svn ci -mm
> svn: E145001: Commit failed (details follow):
> svn: E145001: Entry '/tmp/foowc/foo' has unexpectedly changed special status
Given that "bar\n" does not start with "link ", I think this commit
should have succeeded. (Just like the "printf 'link bar'" variant
succeeds on windows)
>
> Peter
Re: svn commit: r1406870 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
tests/cmdline/special_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Peter Samuelson wrote on Thu, Nov 08, 2012 at 11:01:37 -0600:
>
> [Daniel Shahaf]
> > it's what allows Windows users to create versioned symlinks:
> >
> > printf "link bar" > foo && svn add foo && svn ps svn:special yes foo && svn ci
> >
> > If we don't like changing the specialness of a local addition, we could
> > deprecate (or break) that behaviour and have people run
> > 'svn add --with-revprop svn:special=yes foo' instead.
>
> (Where we understand you didn't actually mean 'revprop'.)
> We have that option already ... unfortunately it doesn't actually work:
>
Time to file a bug report, then!
(Good catch)
> $ echo bar > foo
>
> $ svn add --config-option config:miscellany:enable-auto-props=yes \
> --config-option config:auto-props:foo=svn:special=1 foo
> A foo
>
> $ ls -l foo
> -rw-r--r-- 1 peters peters 4 Nov 8 10:57 foo
>
> $ svn ci -mm
> svn: E145001: Commit failed (details follow):
> svn: E145001: Entry '/tmp/foowc/foo' has unexpectedly changed special status
>
> Peter
Re: svn commit: r1406870 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
tests/cmdline/special_tests.py
Posted by Peter Samuelson <pe...@p12n.org>.
[Daniel Shahaf]
> it's what allows Windows users to create versioned symlinks:
>
> printf "link bar" > foo && svn add foo && svn ps svn:special yes foo && svn ci
>
> If we don't like changing the specialness of a local addition, we could
> deprecate (or break) that behaviour and have people run
> 'svn add --with-revprop svn:special=yes foo' instead.
(Where we understand you didn't actually mean 'revprop'.)
We have that option already ... unfortunately it doesn't actually work:
$ echo bar > foo
$ svn add --config-option config:miscellany:enable-auto-props=yes \
--config-option config:auto-props:foo=svn:special=1 foo
A foo
$ ls -l foo
-rw-r--r-- 1 peters peters 4 Nov 8 10:57 foo
$ svn ci -mm
svn: E145001: Commit failed (details follow):
svn: E145001: Entry '/tmp/foowc/foo' has unexpectedly changed special status
Peter
Re: svn commit: r1406870 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
tests/cmdline/special_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Thu, Nov 08, 2012 at 06:13:22 +0100:
> On 08.11.2012 05:26, Daniel Shahaf wrote:
> > Branko Čibej wrote on Thu, Nov 08, 2012 at 04:13:52 +0100:
> >> I believe that the correct approach would be to always treat a changed
> >> node kind (that's either the appearance/disappearance of the svn:symlink
> >> property, or a change of the initial keyword in the special-file
> >> contents) as if it were a replacement, for the purpose of conflict
> >> detection and resolution, even though the node didn't actually get
> >> replaced.
> > Should we allow nodes to change their special-ness (namely: whether
> > they have svn:special set, and if yes what's the initial keyword)
> > without a replace?
> >
> > i.e., sure, current clients can do that --- "svn ps svn:special yes
> > COMMITTERS" --- so we'll have to handle that in libsvn_wc. But maybe we
> > shouldn't allow any more instances of that.
>
> Good point. It might be a good idea to simply forbid setting or deleting
> svn:special explicitly, and also refusing to commit the file if its
> contents were modified in a way that changes the special type.
>
> Effectively that means you could only create special files through
> indirect means, e.g., by "svn add"ing a symlink.
>
> That wouldn't remove the work needed to fix the tree conflict bug, but
> it would make the svn:special semantics clearer. I personally don't
> think we have to worry about backward compatibility at that level; I'd
> rather treat the fact that you can directly manipulate svn:special as a bug.
>
I would agree that being able to add/rm svn:special on a file node _that
already exists in the repository_ is a bug. But being able to do that
on a locally-added file is a feature --- it's what allows Windows users
to create versioned symlinks:
printf "link bar" > foo && svn add foo && svn ps svn:special yes foo && svn ci
If we don't like changing the specialness of a local addition, we could
deprecate (or break) that behaviour and have people run
'svn add --with-revprop svn:special=yes foo' instead.
(Note to jcorvel, I don't think 'svn add' takes that option yet. :) )
Daniel
> -- Brane
>
Re: svn commit: r1406870 - in /subversion/trunk/subversion: libsvn_wc/update_editor.c
libsvn_wc/wc_db_wcroot.c tests/cmdline/special_tests.py
Posted by Branko Čibej <br...@wandisco.com>.
On 08.11.2012 05:26, Daniel Shahaf wrote:
> Branko Čibej wrote on Thu, Nov 08, 2012 at 04:13:52 +0100:
>> I believe that the correct approach would be to always treat a changed
>> node kind (that's either the appearance/disappearance of the svn:symlink
>> property, or a change of the initial keyword in the special-file
>> contents) as if it were a replacement, for the purpose of conflict
>> detection and resolution, even though the node didn't actually get
>> replaced.
> Should we allow nodes to change their special-ness (namely: whether
> they have svn:special set, and if yes what's the initial keyword)
> without a replace?
>
> i.e., sure, current clients can do that --- "svn ps svn:special yes
> COMMITTERS" --- so we'll have to handle that in libsvn_wc. But maybe we
> shouldn't allow any more instances of that.
Good point. It might be a good idea to simply forbid setting or deleting
svn:special explicitly, and also refusing to commit the file if its
contents were modified in a way that changes the special type.
Effectively that means you could only create special files through
indirect means, e.g., by "svn add"ing a symlink.
That wouldn't remove the work needed to fix the tree conflict bug, but
it would make the svn:special semantics clearer. I personally don't
think we have to worry about backward compatibility at that level; I'd
rather treat the fact that you can directly manipulate svn:special as a bug.
-- Brane
Re: svn commit: r1406870 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c libsvn_wc/wc_db_wcroot.c
tests/cmdline/special_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Thu, Nov 08, 2012 at 04:13:52 +0100:
> On 08.11.2012 03:19, Philip Martin wrote:
> > philip@apache.org writes:
> >
> >> Author: philip
> >> Date: Wed Nov 7 23:49:06 2012
> >> New Revision: 1406870
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1406870&view=rev
> >> Log:
> >> Fix issue 4091, symlink-ness change causes spurious tree-conflict.
> >>
> >> * subversion/libsvn_wc/update_editor.c
> >> (close_file): Only flag symlink-ness conflict if locally modified.
> >>
> >> * subversion/libsvn_wc/wc_db_wcroot.c
> >> (svn_wc__db_wcroot_parse_local_abspath): Only close db if really opened.
> >>
> >> * subversion/tests/cmdline/special_tests.py
> >> (replace_symlinks): Deepcopy the r2 status for later use, remove XFAIL.
> > I thought this was a simple fix but it turns out to be complicated.
> > Issue 4091 involves a file becoming a symlink, or a symlink becoming a
> > file, without a 'R'eplace, i.e. a text and property change but no
> > delete/add. If the file/symlink is not locally modified then update
> > should simply change the node from one to the other, and that's what the
> > code now does. But if the file/symlink is modified the code raises a
> > tree-conflict and I think that may be a problem.
> >
> > Symlinks are text files in the repository so symlink conflicts are
> > really text-conflicts. If r3 has f -> g and r4 has f -> h and I have
> > local a modification f -> i then an update from r3 to r4 produces a
> > conflict (although the results are not pretty):
> >
> > $ svn st wc
> > C wc/f
> > ? wc/f.mine
> > ? wc/f.r3
> > ? wc/f.r4
> > Summary of conflicts:
> > Text conflicts: 1
> > $ ls -l wc
> > -rw-r--r-- 1 pm pm 13 Nov 8 02:13 f
> > lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.mine -> i
> > lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.r3 -> g
> > lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.r4 -> h
> >
> > The handling of symlink conflicts could be improved, but fundamentally
> > they are text-conflicts.
> >
> > So from an implementation point of view file-symlink conflicts are also
> > text-conflicts. Should we be marking such conflicts as tree-conflicts
> > or text-conflicts?
>
> My first response was going to be, "of course it's a tree conflict', but
> upon reflection it turns out that view is heavily skewed towards OS and
> file systems that natively support symlinks, so I'm going to try to
> rationalize that. :)
>
> On one hand, you make a good point that we're basically just looking at
> file contents and properties. On another hand, semantically, files with
> "svn:special" set are /not/ files but a different kind of node; what
> kind precisely depends on the "file contents" but that can be
> interpreted as an implementation detail.
>
> Your example with symlink-symlink conflicts is irrelevant; it's looking
> at the same node, the kind of the node did not change, therefore it
> cannot be a tree conflict.
>
> I believe that the correct approach would be to always treat a changed
> node kind (that's either the appearance/disappearance of the svn:symlink
> property, or a change of the initial keyword in the special-file
> contents) as if it were a replacement, for the purpose of conflict
> detection and resolution, even though the node didn't actually get
> replaced.
Should we allow nodes to change their special-ness (namely: whether
they have svn:special set, and if yes what's the initial keyword)
without a replace?
i.e., sure, current clients can do that --- "svn ps svn:special yes
COMMITTERS" --- so we'll have to handle that in libsvn_wc. But maybe we
shouldn't allow any more instances of that.
> I can't see any other way to get consistent behaviour when we
> introduce other special file types.
>
> -- Brane
Re: svn commit: r1406870 - in /subversion/trunk/subversion: libsvn_wc/update_editor.c
libsvn_wc/wc_db_wcroot.c tests/cmdline/special_tests.py
Posted by Branko Čibej <br...@wandisco.com>.
On 08.11.2012 03:19, Philip Martin wrote:
> philip@apache.org writes:
>
>> Author: philip
>> Date: Wed Nov 7 23:49:06 2012
>> New Revision: 1406870
>>
>> URL: http://svn.apache.org/viewvc?rev=1406870&view=rev
>> Log:
>> Fix issue 4091, symlink-ness change causes spurious tree-conflict.
>>
>> * subversion/libsvn_wc/update_editor.c
>> (close_file): Only flag symlink-ness conflict if locally modified.
>>
>> * subversion/libsvn_wc/wc_db_wcroot.c
>> (svn_wc__db_wcroot_parse_local_abspath): Only close db if really opened.
>>
>> * subversion/tests/cmdline/special_tests.py
>> (replace_symlinks): Deepcopy the r2 status for later use, remove XFAIL.
> I thought this was a simple fix but it turns out to be complicated.
> Issue 4091 involves a file becoming a symlink, or a symlink becoming a
> file, without a 'R'eplace, i.e. a text and property change but no
> delete/add. If the file/symlink is not locally modified then update
> should simply change the node from one to the other, and that's what the
> code now does. But if the file/symlink is modified the code raises a
> tree-conflict and I think that may be a problem.
>
> Symlinks are text files in the repository so symlink conflicts are
> really text-conflicts. If r3 has f -> g and r4 has f -> h and I have
> local a modification f -> i then an update from r3 to r4 produces a
> conflict (although the results are not pretty):
>
> $ svn st wc
> C wc/f
> ? wc/f.mine
> ? wc/f.r3
> ? wc/f.r4
> Summary of conflicts:
> Text conflicts: 1
> $ ls -l wc
> -rw-r--r-- 1 pm pm 13 Nov 8 02:13 f
> lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.mine -> i
> lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.r3 -> g
> lrwxrwxrwx 1 pm pm 1 Nov 8 02:13 f.r4 -> h
>
> The handling of symlink conflicts could be improved, but fundamentally
> they are text-conflicts.
>
> So from an implementation point of view file-symlink conflicts are also
> text-conflicts. Should we be marking such conflicts as tree-conflicts
> or text-conflicts?
My first response was going to be, "of course it's a tree conflict', but
upon reflection it turns out that view is heavily skewed towards OS and
file systems that natively support symlinks, so I'm going to try to
rationalize that. :)
On one hand, you make a good point that we're basically just looking at
file contents and properties. On another hand, semantically, files with
"svn:special" set are /not/ files but a different kind of node; what
kind precisely depends on the "file contents" but that can be
interpreted as an implementation detail.
Your example with symlink-symlink conflicts is irrelevant; it's looking
at the same node, the kind of the node did not change, therefore it
cannot be a tree conflict.
I believe that the correct approach would be to always treat a changed
node kind (that's either the appearance/disappearance of the svn:symlink
property, or a change of the initial keyword in the special-file
contents) as if it were a replacement, for the purpose of conflict
detection and resolution, even though the node didn't actually get
replaced. I can't see any other way to get consistent behaviour when we
introduce other special file types.
-- Brane