You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stephen Butler <sb...@elego.de> on 2008/02/29 14:28:48 UTC
deleted-with-history [sic] after merge
Hi folks,
If an item has been modified or deleted in the repository since
the item was last updated in the working copy, Subversion should
disallow committing that item. This is a prerequisite for tree
conflict detection. The user should have to update in order to
integrate the repo changes, possibly revealing tree conflicts.
I had assumed that Subversion would block a commit that includes an
item that has been modified or deleted on the repo. But that's not
true in all cases:
WC State Repo State Blocked?
======== ========== ========
Modified Modified Yes
Modified Deleted Yes
Deleted Modified Yes
Deleted Deleted No
We'd like to plug that gap. I have a patch ready that does so,
including an extension to the relevant cmdline test (#28) in
commit_tests.py. Unfortunately, the patch makes merge_tests.py
fail at #33 ("merge a replacement of a directory", w/refs to
issues #2144 & #2607).
It seems that the merge code depends on the loophole in commit.
The attached shell script demonstrates this by replacing a directory
'X' that contains a file 'foo'.
If the dir replacement is done manually via 'svn del' and 'svn add',
the status is nominal:
D 1 1 steve /tmp/del-w-hist/wc/A/X/foo
A 0 ? ? /tmp/del-w-hist/wc/A/X/bar
R 1 1 steve /tmp/del-w-hist/wc/A/X
1 1 steve /tmp/del-w-hist/wc/A
Status against revision: 1
and during commit, foo is implicitly deleted with its parent dir:
Replacing tmp/del-w-hist/wc/A/X
Adding tmp/del-w-hist/wc/A/X/bar
Transmitting file data .
Committed revision 2.
If the dir replacement is done via 'svn merge', then the status looks
rather strange (foo appears to be "deleted-with-history"):
D + - 3 steve /tmp/del-w-hist/wc/B/X/foo
A + - 2 steve /tmp/del-w-hist/wc/B/X/bar
R + - 3 steve /tmp/del-w-hist/wc/B/X
M 3 3 steve /tmp/del-w-hist/wc/B
Status against revision: 3
and during commit, foo is deleted explicitly *after* its parent dir
has been deleted.
Sending tmp/del-w-hist/wc/B
Replacing tmp/del-w-hist/wc/B/X
Adding tmp/del-w-hist/wc/B/X/bar
Deleting tmp/del-w-hist/wc/B/X/foo
subversion/libsvn_client/commit.c:916: (apr_err=160028)
svn: Commit failed (details follow):
subversion/libsvn_repos/commit.c:125: (apr_err=160028)
svn: File or directory '/B/X/foo' is out of date
This causes an error in my patched Subversion, which forbids
committing a delete of an item that does not exist in the repository.
Comparing the working copies before the commits, I noticed something
odd in /B/X/.svn/entries: the file foo is marked as "copied". This
explains the 'D +' above. It contradicts the .entries format rules
in subversion/libsvn_wc/README:
copied:
A boolean: true if this entry was added with history; only allowed
when schedule is add. (### Why aren't the copyfrom attributes
enough for this?)
Certainly we should not mark schedule-delete items as "copied". Is
there a good reason for merge to cause commit to visit all items in a
tree that is already deleted?
I suspect the practical reason for the current behavior is the code
shared with diff, which certainly needs to visit everything. For more
on the diff/merge dependency, see my earlier post at
http://svn.haxx.se/dev/archive-2008-02/0776.shtml.
Regards,
Steve
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: disallowing commit of a deletion of a deleted file
Posted by Julian Foad <ju...@btopenworld.com>.
David Glasser wrote:
> Just for extra super duper fun, Subversion today isn't actually
> consistent about whether or not a double-delete should be a conflict
> or OK.
>
> Specifically, in the libsvn_repos commit editor, if a delete_entry is
> sent from the client on an entry that doesn't exist in the
> transactions' root (and note that the transaction root is always based
> off of the youngest rev when the commit is started, not anything found
> in the WC or otherwise client-specified), then it'll just ignore that
> call and not pass anything to the FS layer: the concurrent deletion is
> ignored. This is the usual case, where we commit deletions not
> simultaneously, but without an update in between.
>
> However! *If* two people are actually trying to delete the entry
> nearly simultaneously (specifically, if both transactions are created
> before either is committed), then the merge() code in
> libsvn_fs_{base,fs}/tree.c will actually notice the double-delete and
> throw a conflict error. (I managed to reproduce this a few months ago
> with careful placement of breakpoints on the server.)
Filed as issue #3156.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: disallowing commit of a deletion of a deleted file
Posted by David Glasser <gl...@davidglasser.net>.
On Wed, Apr 2, 2008 at 4:02 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "David Glasser" <gl...@davidglasser.net> writes:
> > Just for extra super duper fun, Subversion today isn't actually
> > consistent about whether or not a double-delete should be a conflict
> > or OK.
> >
> > Specifically, in the libsvn_repos commit editor, if a delete_entry is
> > sent from the client on an entry that doesn't exist in the
> > transactions' root (and note that the transaction root is always based
> > off of the youngest rev when the commit is started, not anything found
> > in the WC or otherwise client-specified), then it'll just ignore that
> > call and not pass anything to the FS layer: the concurrent deletion is
> > ignored. This is the usual case, where we commit deletions not
> > simultaneously, but without an update in between.
> >
> > However! *If* two people are actually trying to delete the entry
> > nearly simultaneously (specifically, if both transactions are created
> > before either is committed), then the merge() code in
> > libsvn_fs_{base,fs}/tree.c will actually notice the double-delete and
> > throw a conflict error. (I managed to reproduce this a few months ago
> > with careful placement of breakpoints on the server.)
>
> How odd. That means that simply re-attempting the failed commit would
> succeed, even though no update had happened in between, right?
I believe so, yes.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: disallowing commit of a deletion of a deleted file
Posted by Karl Fogel <kf...@red-bean.com>.
"David Glasser" <gl...@davidglasser.net> writes:
> Just for extra super duper fun, Subversion today isn't actually
> consistent about whether or not a double-delete should be a conflict
> or OK.
>
> Specifically, in the libsvn_repos commit editor, if a delete_entry is
> sent from the client on an entry that doesn't exist in the
> transactions' root (and note that the transaction root is always based
> off of the youngest rev when the commit is started, not anything found
> in the WC or otherwise client-specified), then it'll just ignore that
> call and not pass anything to the FS layer: the concurrent deletion is
> ignored. This is the usual case, where we commit deletions not
> simultaneously, but without an update in between.
>
> However! *If* two people are actually trying to delete the entry
> nearly simultaneously (specifically, if both transactions are created
> before either is committed), then the merge() code in
> libsvn_fs_{base,fs}/tree.c will actually notice the double-delete and
> throw a conflict error. (I managed to reproduce this a few months ago
> with careful placement of breakpoints on the server.)
How odd. That means that simply re-attempting the failed commit would
succeed, even though no update had happened in between, right?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: disallowing commit of a deletion of a deleted file (was: Re: deleted-with-history [sic] after merge)
Posted by David Glasser <gl...@davidglasser.net>.
On Wed, Mar 19, 2008 at 2:42 PM, Stefan Sperling <st...@elego.de> wrote:
>
> [Now that I have my tree-conflicts hat back on, I'd like to revive
> this thread, which didn't come to a conclusion last time around.
>
> Stephen's original mail in this thread talks about two separate issues,
> actually. One of them, the commit of a delete of an already deleted
> file, will be addressed below. The other one, namely that of the diff
> editor visiting all files in deleted directories during merge, should
> be discussed separately. In fact, Stephen discovered the latter issue
> while investigating the former.]
>
>
> On Fri, Feb 29, 2008 at 08:50:56AM -0600, Ben Collins-Sussman wrote:
> > 2008/2/29 Stephen Butler <sb...@elego.de>:
> > > Hi folks,
> > >
> > > If an item has been modified or deleted in the repository since
> > > the item was last updated in the working copy, Subversion should
> > > disallow committing that item. This is a prerequisite for tree
> > > conflict detection. The user should have to update in order to
> > > integrate the repo changes, possibly revealing tree conflicts.
> > >
> > > I had assumed that Subversion would block a commit that includes an
> > > item that has been modified or deleted on the repo. But that's not
> > > true in all cases:
> > >
> > > WC State Repo State Blocked?
> > > ======== ========== ========
> > > Modified Modified Yes
> > > Modified Deleted Yes
> > > Deleted Modified Yes
> > > Deleted Deleted No
> > >
> > > We'd like to plug that gap.
> >
> > Let's back up a moment. Why is that a 'gap'? That's a very
> > deliberate design decision, IIRC. The 'auto merging' loop that's part
> > of commit finalization considers some changes to be mergeable, and
> > some not. If two people are in a commit race and change the same
> > file, that's not auto-mergeable: somebody wins the race, the other
> > person's commit outright fails. But if the only 'conflict' is that
> > they both happened to delete the same file, that's a perfectly safe
> > auto-mergeable change: the overlapping changes are in agreement.
> > Nobody will be destroying or overwriting somebody else's change by
> > allowing the changes to be merged.
> >
> > In theory, if the two commits made absolutely identical textual
> > changes to the same file, that would also be an example of a 'safe'
> > auto-mergeable thing; we just never made the commit-finalization loop
> > sophisticated enough to notice that case.
> >
> > In any case, I certainly don't consider this a loophole or oversight;
> > it's a deliberate feature. Is this behavior presenting a real problem
> > for your model of tree-conflict resolution?
>
>
> Ben, I've talked to Stephen again today about this issue. I think
> I understand now why the current behaviour of accepting a delete
> of a deleted file as a valid commit operation does indeed present
> a real problem for tree-conflict detection (not resolution -- automatic
> tree-conflict resolution isn't covered by the current implementation).
Just for extra super duper fun, Subversion today isn't actually
consistent about whether or not a double-delete should be a conflict
or OK.
Specifically, in the libsvn_repos commit editor, if a delete_entry is
sent from the client on an entry that doesn't exist in the
transactions' root (and note that the transaction root is always based
off of the youngest rev when the commit is started, not anything found
in the WC or otherwise client-specified), then it'll just ignore that
call and not pass anything to the FS layer: the concurrent deletion is
ignored. This is the usual case, where we commit deletions not
simultaneously, but without an update in between.
However! *If* two people are actually trying to delete the entry
nearly simultaneously (specifically, if both transactions are created
before either is committed), then the merge() code in
libsvn_fs_{base,fs}/tree.c will actually notice the double-delete and
throw a conflict error. (I managed to reproduce this a few months ago
with careful placement of breakpoints on the server.)
just muddying the waters,
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: disallowing commit of a deletion of a deleted file (was: Re:
deleted-with-history [sic] after merge)
Posted by Stephen Butler <sb...@elego.de>.
Quoting Stefan Sperling <st...@elego.de>:
> Instead of the commit succeeding (leaving the developer with the impression
> that the file was deleted, even though it was renamed), the developer
> should be forced to update (the deleted file being out-of-date).
> The developer will then receive the delete of the already locally deleted
> file, which our current implementation will flag as a potential
> tree-conflict.
> The developer should then take this a warning sign, check the resulting tree
> for sanity, take appropriate action if need be and mark the conflict as
> resolved. Now the commit can take place without risk of causing trouble
> further down the road.
>
> I admit that this sounds a bit unwieldy, since we could essentially
> already flag a tree-conflict during the commit phase, upon discovering
> that the file is missing in the repository, without requiring the extra
> update.
A cautionary note about flagging tree conflicts during commit:
The current tree-conflicts plan has a simple division of labor. The
update/switch/merge commands detect tree conflicts and flag them in
the working copy. The commit command stops with an error if it
finds a flagged conflict.
The user then removes the conflict flags using the resolved or revert
commands, and tries to commit again.
Suppose we extend commit to detect tree conflicts and flag them in
the working copy. The user would have no chance to resolve the
conflicts. We would need a special list of "tree conflicts detected
by commit" and would have to track which of them were already
resolved.
Being a little more strict about requiring an up-to-date working
copy is much simpler.
Steve
> But in any case, the commit needs to be blocked if we want to have
> a chance at informing the developer whether a delete about to be committed
> may actually end up being a rename.
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
disallowing commit of a deletion of a deleted file (was: Re:
deleted-with-history [sic] after merge)
Posted by Stefan Sperling <st...@elego.de>.
[Now that I have my tree-conflicts hat back on, I'd like to revive
this thread, which didn't come to a conclusion last time around.
Stephen's original mail in this thread talks about two separate issues,
actually. One of them, the commit of a delete of an already deleted
file, will be addressed below. The other one, namely that of the diff
editor visiting all files in deleted directories during merge, should
be discussed separately. In fact, Stephen discovered the latter issue
while investigating the former.]
On Fri, Feb 29, 2008 at 08:50:56AM -0600, Ben Collins-Sussman wrote:
> 2008/2/29 Stephen Butler <sb...@elego.de>:
> > Hi folks,
> >
> > If an item has been modified or deleted in the repository since
> > the item was last updated in the working copy, Subversion should
> > disallow committing that item. This is a prerequisite for tree
> > conflict detection. The user should have to update in order to
> > integrate the repo changes, possibly revealing tree conflicts.
> >
> > I had assumed that Subversion would block a commit that includes an
> > item that has been modified or deleted on the repo. But that's not
> > true in all cases:
> >
> > WC State Repo State Blocked?
> > ======== ========== ========
> > Modified Modified Yes
> > Modified Deleted Yes
> > Deleted Modified Yes
> > Deleted Deleted No
> >
> > We'd like to plug that gap.
>
> Let's back up a moment. Why is that a 'gap'? That's a very
> deliberate design decision, IIRC. The 'auto merging' loop that's part
> of commit finalization considers some changes to be mergeable, and
> some not. If two people are in a commit race and change the same
> file, that's not auto-mergeable: somebody wins the race, the other
> person's commit outright fails. But if the only 'conflict' is that
> they both happened to delete the same file, that's a perfectly safe
> auto-mergeable change: the overlapping changes are in agreement.
> Nobody will be destroying or overwriting somebody else's change by
> allowing the changes to be merged.
>
> In theory, if the two commits made absolutely identical textual
> changes to the same file, that would also be an example of a 'safe'
> auto-mergeable thing; we just never made the commit-finalization loop
> sophisticated enough to notice that case.
>
> In any case, I certainly don't consider this a loophole or oversight;
> it's a deliberate feature. Is this behavior presenting a real problem
> for your model of tree-conflict resolution?
Ben, I've talked to Stephen again today about this issue. I think
I understand now why the current behaviour of accepting a delete
of a deleted file as a valid commit operation does indeed present
a real problem for tree-conflict detection (not resolution -- automatic
tree-conflict resolution isn't covered by the current implementation).
If you are talking only about file deletions, the current behaviour
is perfectly acceptable -- after all, a delete of a delete adds up
to a delete, so there's no conflict, right?
Now, looking at this from the tree-conflict perspective, things are
a bit more complicated.
Let us assume, for a minute, that Subversion had true renames, and
that the update editor had a callback function like move_file(source, dest).
Imagine a user who has locally deleted a file, and updates the working
copy before committing. As part of the update, the editor tries to run
the move_file() operation on the locally deleted file. Since the source
has been deleted, the operation obviously cannot be carried out as
intended (a non-existent file cannot be moved).
A move transforms the tree in a different way than a delete does,
and, semantically, one operation excludes the other, so we have a
tree-conflict.
Since move_file() does not exist in reality, and since the update and
diff editors handle deletes and adds with history in essentially separate
contexts (i.e. callbacks), we have to try to make semi-educated guesses
about the user's intention by looking *only* at deletes.
The whole tree-conflict branch is essentially about reverse engineering
the developer's real intention behind a tree transformation. We can only
do this after the fact, by looking at the small amount of information
we have left on the client side during update and merge operations.
Rename information is tossed out right in libsvn_client when the
user runs "svn move". We therefore have to live with false positives,
since a delete callback invocation may always be due to an actual delete,
not a move.
So to catch the case where one developer deletes a file, and another
removes it (a sub-case of our "use case 3" in
notes/tree-conflicts/use-cases.txt, in which both operations are
moves), Subversion must not allow a commit of a deleted file if the
file has already been deleted in the repository. Because it could be
that the file was not actually deleted in the repository, but moved!
Instead of the commit succeeding (leaving the developer with the impression
that the file was deleted, even though it was renamed), the developer
should be forced to update (the deleted file being out-of-date).
The developer will then receive the delete of the already locally deleted
file, which our current implementation will flag as a potential tree-conflict.
The developer should then take this a warning sign, check the resulting tree
for sanity, take appropriate action if need be and mark the conflict as
resolved. Now the commit can take place without risk of causing trouble
further down the road.
I admit that this sounds a bit unwieldy, since we could essentially
already flag a tree-conflict during the commit phase, upon discovering
that the file is missing in the repository, without requiring the extra
update. But in any case, the commit needs to be blocked if we want to have
a chance at informing the developer whether a delete about to be committed
may actually end up being a rename.
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: deleted-with-history [sic] after merge
Posted by Julian Foad <ju...@btopenworld.com>.
Ben Collins-Sussman wrote:
> 2008/2/29 Stephen Butler <sb...@elego.de>:
>
>>Hi folks,
>>
>> If an item has been modified or deleted in the repository since
>> the item was last updated in the working copy, Subversion should
>> disallow committing that item. This is a prerequisite for tree
>> conflict detection. The user should have to update in order to
>> integrate the repo changes, possibly revealing tree conflicts.
>>
>> I had assumed that Subversion would block a commit that includes an
>> item that has been modified or deleted on the repo. But that's not
>> true in all cases:
>>
>> WC State Repo State Blocked?
>> ======== ========== ========
>> Modified Modified Yes
>> Modified Deleted Yes
>> Deleted Modified Yes
>> Deleted Deleted No
>>
>> We'd like to plug that gap.
>
>
> Let's back up a moment. Why is that a 'gap'? That's a very
> deliberate design decision, IIRC. The 'auto merging' loop that's part
> of commit finalization considers some changes to be mergeable, and
> some not. If two people are in a commit race and change the same
> file, that's not auto-mergeable: somebody wins the race, the other
> person's commit outright fails. But if the only 'conflict' is that
> they both happened to delete the same file, that's a perfectly safe
> auto-mergeable change: the overlapping changes are in agreement.
> Nobody will be destroying or overwriting somebody else's change by
> allowing the changes to be merged.
>
> In theory, if the two commits made absolutely identical textual
> changes to the same file, that would also be an example of a 'safe'
> auto-mergeable thing; we just never made the commit-finalization loop
> sophisticated enough to notice that case.
>
> In any case, I certainly don't consider this a loophole or oversight;
> it's a deliberate feature. Is this behavior presenting a real problem
> for your model of tree-conflict resolution?
Yes, I acknowledge that was a deliberate policy. We decided that it's usually
what the user wants, and pretty safe, so we'd just do it.
However, this is a policy which is not right for every situation. Our approach
in this designs was to decide what we think is the best compromise between
safety and usability, and hard-code our heuristics into the algorithm.
I'm pretty sure the particular big problem with this is the fact that two
conflicting renames of the same file can only be detected at the moment by the
two conflicting "delete" halves of those renames. But if our policy says that
two deletes of the same thing are not in conflict and are always to be silently
combined, then...
merge (rename A B) with (rename A C)
=> there are now two files, B and C.
I'd like to reexamine the way in which this and similar policy decisions are
embedded into the code. I would like the merge code to act according to a
caller-specified policy, or raise all conflicts like this to a "conflict
resolver" callback in which the client can enact whatever policy it likes -
auto-merge deletes, or don't, for example.
I'm seeing the same kind of issue in the "propset URL@base-rev" thread right
now. There are questions like, "If A tries to delete property P, and B has just
modified P, should the delete succeed or fail?" (I'm not sure this particular
question is open for debate, as it may already be determined by the existing
out-of-dateness rules, but this sort of question is raising its head.)
The tree-conflicts work is closely tied to the "out-of-dateness" policy. As
some (most?) of this policy is enforced repository-side, we need to be a bit
careful about compatibility if we now need to reconsider, extend, or duplicate
these policies on the client side, as I feel we may well need to to detect
conflicts.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: deleted-with-history [sic] after merge
Posted by "C. Michael Pilato" <cm...@collab.net>.
David Glasser wrote:
> On Fri, Feb 29, 2008 at 1:58 PM, Ben Collins-Sussman
> <su...@red-bean.com> wrote:
>> On Fri, Feb 29, 2008 at 11:06 AM, David Glasser
>> <gl...@davidglasser.net> wrote:
>>
>> > You sure? From libsvn_fs_fs/tree.c(merge):
>> >
>> > /* If SOURCE-ENTRY and TARGET-ENTRY are both null, that's a
>> > double delete; flag a conflict. */
>> > if (s_entry == NULL || t_entry == NULL)
>> > return conflict_err(conflict_p,
>> > svn_path_join(target_path,
>> > a_entry->name,
>> > iterpool));
>>
>> I have a vague recollection of this code flip-flopping back and forth
>> over the years, and cmpilato being at the helm of such decisions.
>> Mike, can you remember what's up here?
>
> So what happens (this is based on both reading and running the code)
> is that *most* of the time double deletes are ignored. However, if
> Client #2 starts its commit drive after Client #1 starts its commit
> drive but before Client #1 finalizes its commit, you'll get an error
> from the quoted code above. (I was able to reproduce this by running
> Client #1 under gdb and throwing in a breakpoint.)
Right. The flip-flopping Ben might be thinking of is that we used to build
our commit transactions against the revision used to open the commit
editor's root directory; now we always build the commit transaction against
HEAD.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: deleted-with-history [sic] after merge
Posted by David Glasser <gl...@davidglasser.net>.
On Fri, Feb 29, 2008 at 1:58 PM, Ben Collins-Sussman
<su...@red-bean.com> wrote:
> On Fri, Feb 29, 2008 at 11:06 AM, David Glasser
> <gl...@davidglasser.net> wrote:
>
> > You sure? From libsvn_fs_fs/tree.c(merge):
> >
> > /* If SOURCE-ENTRY and TARGET-ENTRY are both null, that's a
> > double delete; flag a conflict. */
> > if (s_entry == NULL || t_entry == NULL)
> > return conflict_err(conflict_p,
> > svn_path_join(target_path,
> > a_entry->name,
> > iterpool));
>
> I have a vague recollection of this code flip-flopping back and forth
> over the years, and cmpilato being at the helm of such decisions.
> Mike, can you remember what's up here?
So what happens (this is based on both reading and running the code)
is that *most* of the time double deletes are ignored. However, if
Client #2 starts its commit drive after Client #1 starts its commit
drive but before Client #1 finalizes its commit, you'll get an error
from the quoted code above. (I was able to reproduce this by running
Client #1 under gdb and throwing in a breakpoint.)
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: deleted-with-history [sic] after merge
Posted by Ben Collins-Sussman <su...@red-bean.com>.
On Fri, Feb 29, 2008 at 11:06 AM, David Glasser
<gl...@davidglasser.net> wrote:
> You sure? From libsvn_fs_fs/tree.c(merge):
>
> /* If SOURCE-ENTRY and TARGET-ENTRY are both null, that's a
> double delete; flag a conflict. */
> if (s_entry == NULL || t_entry == NULL)
> return conflict_err(conflict_p,
> svn_path_join(target_path,
> a_entry->name,
> iterpool));
I have a vague recollection of this code flip-flopping back and forth
over the years, and cmpilato being at the helm of such decisions.
Mike, can you remember what's up here?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: deleted-with-history [sic] after merge
Posted by David Glasser <gl...@davidglasser.net>.
On Fri, Feb 29, 2008 at 9:06 AM, David Glasser <gl...@davidglasser.net> wrote:
>
> On Fri, Feb 29, 2008 at 6:50 AM, Ben Collins-Sussman
> <su...@red-bean.com> wrote:
> > 2008/2/29 Stephen Butler <sb...@elego.de>:
> >
> > > Hi folks,
> > >
> > > If an item has been modified or deleted in the repository since
> > > the item was last updated in the working copy, Subversion should
> > > disallow committing that item. This is a prerequisite for tree
> > > conflict detection. The user should have to update in order to
> > > integrate the repo changes, possibly revealing tree conflicts.
> > >
> > > I had assumed that Subversion would block a commit that includes an
> > > item that has been modified or deleted on the repo. But that's not
> > > true in all cases:
> > >
> > > WC State Repo State Blocked?
> > > ======== ========== ========
> > > Modified Modified Yes
> > > Modified Deleted Yes
> > > Deleted Modified Yes
> > > Deleted Deleted No
> > >
> > > We'd like to plug that gap.
> >
> > Let's back up a moment. Why is that a 'gap'? That's a very
> > deliberate design decision, IIRC. The 'auto merging' loop that's part
> > of commit finalization considers some changes to be mergeable, and
> > some not. If two people are in a commit race and change the same
> > file, that's not auto-mergeable: somebody wins the race, the other
> > person's commit outright fails. But if the only 'conflict' is that
> > they both happened to delete the same file, that's a perfectly safe
> > auto-mergeable change: the overlapping changes are in agreement.
> > Nobody will be destroying or overwriting somebody else's change by
> > allowing the changes to be merged.
>
> You sure? From libsvn_fs_fs/tree.c(merge):
>
> /* If SOURCE-ENTRY and TARGET-ENTRY are both null, that's a
> double delete; flag a conflict. */
> if (s_entry == NULL || t_entry == NULL)
> return conflict_err(conflict_p,
> svn_path_join(target_path,
> a_entry->name,
> iterpool));
Though to be clear, the repos commit editor takes the opposite
approach: it ignores delete_entry calls made on nonexistent paths.
Perhaps this inconsistency is a bug.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: deleted-with-history [sic] after merge
Posted by David Glasser <gl...@davidglasser.net>.
On Fri, Feb 29, 2008 at 6:50 AM, Ben Collins-Sussman
<su...@red-bean.com> wrote:
> 2008/2/29 Stephen Butler <sb...@elego.de>:
>
> > Hi folks,
> >
> > If an item has been modified or deleted in the repository since
> > the item was last updated in the working copy, Subversion should
> > disallow committing that item. This is a prerequisite for tree
> > conflict detection. The user should have to update in order to
> > integrate the repo changes, possibly revealing tree conflicts.
> >
> > I had assumed that Subversion would block a commit that includes an
> > item that has been modified or deleted on the repo. But that's not
> > true in all cases:
> >
> > WC State Repo State Blocked?
> > ======== ========== ========
> > Modified Modified Yes
> > Modified Deleted Yes
> > Deleted Modified Yes
> > Deleted Deleted No
> >
> > We'd like to plug that gap.
>
> Let's back up a moment. Why is that a 'gap'? That's a very
> deliberate design decision, IIRC. The 'auto merging' loop that's part
> of commit finalization considers some changes to be mergeable, and
> some not. If two people are in a commit race and change the same
> file, that's not auto-mergeable: somebody wins the race, the other
> person's commit outright fails. But if the only 'conflict' is that
> they both happened to delete the same file, that's a perfectly safe
> auto-mergeable change: the overlapping changes are in agreement.
> Nobody will be destroying or overwriting somebody else's change by
> allowing the changes to be merged.
You sure? From libsvn_fs_fs/tree.c(merge):
/* If SOURCE-ENTRY and TARGET-ENTRY are both null, that's a
double delete; flag a conflict. */
if (s_entry == NULL || t_entry == NULL)
return conflict_err(conflict_p,
svn_path_join(target_path,
a_entry->name,
iterpool));
> In theory, if the two commits made absolutely identical textual
> changes to the same file, that would also be an example of a 'safe'
> auto-mergeable thing; we just never made the commit-finalization loop
> sophisticated enough to notice that case.
Not really. The typical example is something like a file containing
the number of modules in a system. The system has 10 modules and the
file contains "10". Two changes are applied at the same time: both of
them adds a module and changes the "10" to "11". This should
conflict, since the file needs to end up saying "12".
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: deleted-with-history [sic] after merge
Posted by Ben Collins-Sussman <su...@red-bean.com>.
2008/2/29 Stephen Butler <sb...@elego.de>:
> Hi folks,
>
> If an item has been modified or deleted in the repository since
> the item was last updated in the working copy, Subversion should
> disallow committing that item. This is a prerequisite for tree
> conflict detection. The user should have to update in order to
> integrate the repo changes, possibly revealing tree conflicts.
>
> I had assumed that Subversion would block a commit that includes an
> item that has been modified or deleted on the repo. But that's not
> true in all cases:
>
> WC State Repo State Blocked?
> ======== ========== ========
> Modified Modified Yes
> Modified Deleted Yes
> Deleted Modified Yes
> Deleted Deleted No
>
> We'd like to plug that gap.
Let's back up a moment. Why is that a 'gap'? That's a very
deliberate design decision, IIRC. The 'auto merging' loop that's part
of commit finalization considers some changes to be mergeable, and
some not. If two people are in a commit race and change the same
file, that's not auto-mergeable: somebody wins the race, the other
person's commit outright fails. But if the only 'conflict' is that
they both happened to delete the same file, that's a perfectly safe
auto-mergeable change: the overlapping changes are in agreement.
Nobody will be destroying or overwriting somebody else's change by
allowing the changes to be merged.
In theory, if the two commits made absolutely identical textual
changes to the same file, that would also be an example of a 'safe'
auto-mergeable thing; we just never made the commit-finalization loop
sophisticated enough to notice that case.
In any case, I certainly don't consider this a loophole or oversight;
it's a deliberate feature. Is this behavior presenting a real problem
for your model of tree-conflict resolution?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: deleted-with-history [sic] after merge
Posted by Erik Huelsmann <eh...@gmail.com>.
On 3/26/08, Julian Foad <ju...@btopenworld.com> wrote:
> Erik Huelsmann wrote:
> > ... the problem you describe below looks an awful lot like issue
>
> Issue #1962: "merge of non-empty subdir committed incorrectly".
>
> Erik, In working on that issue, you added in r26169 a test for this behaviour
> in merge_tests.py: "merge added subtree".
> Could you or I re-add that test as XFAIL now? I tried. All that is needed to
> update it is to add an initial parameter "disk" to the call to
> "compare_trees()". It looks like a valid test to have.
Yes, that's probably a good idea. When I bailed out, I didn't think I
was going to resolve the issue any time soon, so I reversed all the
work, without looking twice to see which parts might have value added.
> Do you have any idea how to progress this? It sounds like your final comment
> below - "... we'll need to design what we want..." - is the crux.
Right. I'm actually trying to come up with a design for wc-ng which
addresses this issue. I'm not sure it's actually worth the trouble of
trying to update wc-1.0 at this stage: the more things we fix, the
more it seems to get broken in unexpected ways.
By coming up with a completely new design, I hope to address many of
these issues. Maybe the new design can even guide further development
of wc-1.0 for a little while, fixing any long standing issues before
wc-ng materializes.
Bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: deleted-with-history [sic] after merge
Posted by Julian Foad <ju...@btopenworld.com>.
Erik Huelsmann wrote:
> ... the problem you describe below looks an awful lot like issue
Issue #1962: "merge of non-empty subdir committed incorrectly".
Erik, In working on that issue, you added in r26169 a test for this behaviour
in merge_tests.py: "merge added subtree".
Could you or I re-add that test as XFAIL now? I tried. All that is needed to
update it is to add an initial parameter "disk" to the call to
"compare_trees()". It looks like a valid test to have.
Do you have any idea how to progress this? It sounds like your final comment
below - "... we'll need to design what we want..." - is the crux.
- Julian
>> If the dir replacement is done manually via 'svn del' and 'svn add',
>> the status is nominal:
>>
>> D 1 1 steve /tmp/del-w-hist/wc/A/X/foo
>> A 0 ? ? /tmp/del-w-hist/wc/A/X/bar
>> R 1 1 steve /tmp/del-w-hist/wc/A/X
>> 1 1 steve /tmp/del-w-hist/wc/A
>> Status against revision: 1
>>
>> and during commit, foo is implicitly deleted with its parent dir:
>>
>> Replacing tmp/del-w-hist/wc/A/X
>> Adding tmp/del-w-hist/wc/A/X/bar
>> Transmitting file data .
>> Committed revision 2.
>>
>> If the dir replacement is done via 'svn merge', then the status looks
>> rather strange (foo appears to be "deleted-with-history"):
>>
>> D + - 3 steve /tmp/del-w-hist/wc/B/X/foo
>> A + - 2 steve /tmp/del-w-hist/wc/B/X/bar
>> R + - 3 steve /tmp/del-w-hist/wc/B/X
>> M 3 3 steve /tmp/del-w-hist/wc/B
>> Status against revision: 3
>
> The '+'-es come from svn_wc_add2() which marks the complete subtree as
> COPIED==TRUE. (ouch!)
>
>
>> and during commit, foo is deleted explicitly *after* its parent dir
>> has been deleted.
>>
>> Sending tmp/del-w-hist/wc/B
>> Replacing tmp/del-w-hist/wc/B/X
>> Adding tmp/del-w-hist/wc/B/X/bar
>> Deleting tmp/del-w-hist/wc/B/X/foo
>> subversion/libsvn_client/commit.c:916: (apr_err=160028)
>> svn: Commit failed (details follow):
>> subversion/libsvn_repos/commit.c:125: (apr_err=160028)
>> svn: File or directory '/B/X/foo' is out of date
>
> The fact that the delete comes after the delete of the parent dir is
> normal: that's how the svn_delta_path_driver works: depth-first,
> processing tree nodes as they are passed
>
>> This causes an error in my patched Subversion, which forbids
>> committing a delete of an item that does not exist in the repository.
>
> Right. Here you have the essence of the problem: in the wc-1.0 schema,
> files which are part of the deleted subtree in a replaced subtree have
> to be marked 'deleted'. If they were not, there would be no way to put
> another file in its place.
>
> However, a schedule delete is a valid state in a added-with-history
> subtree too: you may want to delete part of the subtree before you
> commit.
>
> So, what we're facing here is: we want to be able to tell the file is
> part of a deleted subtree *and* we want to be able to tell a file has
> been removed from the add-with-history subtree we copied on top of it.
> The problem is that we do that with the same schedule-delete status of
> the file, which then becomes ambiguous.
>
>
>> Comparing the working copies before the commits, I noticed something
>> odd in /B/X/.svn/entries: the file foo is marked as "copied". This
>> explains the 'D +' above. It contradicts the .entries format rules
>> in subversion/libsvn_wc/README:
>>
>> copied:
>> A boolean: true if this entry was added with history; only allowed
>> when schedule is add. (### Why aren't the copyfrom attributes
>> enough for this?)
>
> This is plain wrong: files which are part of a
> schedule-add-with-history subtree are marked schedule-normal, but have
> the 'copied' flag. (Not that I actually like this design, but that's
> how it is now.)
>
>
>> Certainly we should not mark schedule-delete items as "copied". Is
>> there a good reason for merge to cause commit to visit all items in a
>> tree that is already deleted?
>
> No, there is not. The copied flag comes from svn_wc_add2() which
> markes all files in the subtree as copied (and rewrites their URL),
> making this operation a *huge* problem for replaced subtrees.
>
>
>> I suspect the practical reason for the current behavior is the code
>> shared with diff, which certainly needs to visit everything. For more
>> on the diff/merge dependency, see my earlier post at
>> http://svn.haxx.se/dev/archive-2008-02/0776.shtml.
>
> I wouldn't go this far: I think it should be possible to get the right
> diffs and commits without a copied flag all over the place, but we'll
> need to design what we want from our library.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: deleted-with-history [sic] after merge
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Mar 24, 2008 at 09:40:53AM +0100, Erik Huelsmann wrote:
> > The attached shell script demonstrates this by replacing a directory
> > 'X' that contains a file 'foo'.
>
> ... the problem you describe below looks an awful lot like issue
> 19<some-value-i-told-you-earlier-tonight-but-can't-find-now>
I think you mean issue #1962 [1], the issue with the same number
as the year when the Beatles' first record, "My Bonnie" with Tony
Sheridan, was released by Polydor [2].
[1] http://subversion.tigris.org/issues/show_bug.cgi?id=1962
[2] http://en.wikipedia.org/wiki/1962#January
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: deleted-with-history [sic] after merge
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Mar 24, 2008 at 09:40:53AM +0100, Erik Huelsmann wrote:
> > copied:
> > A boolean: true if this entry was added with history; only allowed
> > when schedule is add. (### Why aren't the copyfrom attributes
> > enough for this?)
>
> This is plain wrong: files which are part of a
> schedule-add-with-history subtree are marked schedule-normal, but have
> the 'copied' flag. (Not that I actually like this design, but that's
> how it is now.)
Shouldn't the README be updated then?
Index: subversion/libsvn_wc/README
===================================================================
--- subversion/libsvn_wc/README (revision 30014)
+++ subversion/libsvn_wc/README (working copy)
@@ -423,8 +423,9 @@ prop-reject-file:
copied:
A boolean: true if this entry was added with history; only allowed
- when schedule is add. (### Why aren't the copyfrom attributes
- enough for this?)
+ when schedule is add or for files which are marked as schedule-normal
+ as part of a schedule-add-with-history subtree. (### Why aren't the
+ copyfrom attributes enough for this?)
copyfrom-url:
If this entry is added with history, the URL of the copy source.
I left the question marked with ### in place, cause I don't know the answer.
But maybe we could clarify that in the same go?
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: deleted-with-history [sic] after merge
Posted by Erik Huelsmann <eh...@gmail.com>.
> It seems that the merge code depends on the loophole in commit.
Well, if there's a loophole and whether merge depends on it, I don't
know, but ...
> The attached shell script demonstrates this by replacing a directory
> 'X' that contains a file 'foo'.
... the problem you describe below looks an awful lot like issue
19<some-value-i-told-you-earlier-tonight-but-can't-find-now>
> If the dir replacement is done manually via 'svn del' and 'svn add',
> the status is nominal:
>
> D 1 1 steve /tmp/del-w-hist/wc/A/X/foo
> A 0 ? ? /tmp/del-w-hist/wc/A/X/bar
> R 1 1 steve /tmp/del-w-hist/wc/A/X
> 1 1 steve /tmp/del-w-hist/wc/A
> Status against revision: 1
>
> and during commit, foo is implicitly deleted with its parent dir:
>
> Replacing tmp/del-w-hist/wc/A/X
> Adding tmp/del-w-hist/wc/A/X/bar
> Transmitting file data .
> Committed revision 2.
>
> If the dir replacement is done via 'svn merge', then the status looks
> rather strange (foo appears to be "deleted-with-history"):
>
> D + - 3 steve /tmp/del-w-hist/wc/B/X/foo
> A + - 2 steve /tmp/del-w-hist/wc/B/X/bar
> R + - 3 steve /tmp/del-w-hist/wc/B/X
> M 3 3 steve /tmp/del-w-hist/wc/B
> Status against revision: 3
The '+'-es come from svn_wc_add2() which marks the complete subtree as
COPIED==TRUE. (ouch!)
> and during commit, foo is deleted explicitly *after* its parent dir
> has been deleted.
>
> Sending tmp/del-w-hist/wc/B
> Replacing tmp/del-w-hist/wc/B/X
> Adding tmp/del-w-hist/wc/B/X/bar
> Deleting tmp/del-w-hist/wc/B/X/foo
> subversion/libsvn_client/commit.c:916: (apr_err=160028)
> svn: Commit failed (details follow):
> subversion/libsvn_repos/commit.c:125: (apr_err=160028)
> svn: File or directory '/B/X/foo' is out of date
The fact that the delete comes after the delete of the parent dir is
normal: that's how the svn_delta_path_driver works: depth-first,
processing tree nodes as they are passed
> This causes an error in my patched Subversion, which forbids
> committing a delete of an item that does not exist in the repository.
Right. Here you have the essence of the problem: in the wc-1.0 schema,
files which are part of the deleted subtree in a replaced subtree have
to be marked 'deleted'. If they were not, there would be no way to put
another file in its place.
However, a schedule delete is a valid state in a added-with-history
subtree too: you may want to delete part of the subtree before you
commit.
So, what we're facing here is: we want to be able to tell the file is
part of a deleted subtree *and* we want to be able to tell a file has
been removed from the add-with-history subtree we copied on top of it.
The problem is that we do that with the same schedule-delete status of
the file, which then becomes ambiguous.
> Comparing the working copies before the commits, I noticed something
> odd in /B/X/.svn/entries: the file foo is marked as "copied". This
> explains the 'D +' above. It contradicts the .entries format rules
> in subversion/libsvn_wc/README:
>
> copied:
> A boolean: true if this entry was added with history; only allowed
> when schedule is add. (### Why aren't the copyfrom attributes
> enough for this?)
This is plain wrong: files which are part of a
schedule-add-with-history subtree are marked schedule-normal, but have
the 'copied' flag. (Not that I actually like this design, but that's
how it is now.)
> Certainly we should not mark schedule-delete items as "copied". Is
> there a good reason for merge to cause commit to visit all items in a
> tree that is already deleted?
No, there is not. The copied flag comes from svn_wc_add2() which
markes all files in the subtree as copied (and rewrites their URL),
making this operation a *huge* problem for replaced subtrees.
> I suspect the practical reason for the current behavior is the code
> shared with diff, which certainly needs to visit everything. For more
> on the diff/merge dependency, see my earlier post at
> http://svn.haxx.se/dev/archive-2008-02/0776.shtml.
I wouldn't go this far: I think it should be possible to get the right
diffs and commits without a copied flag all over the place, but we'll
need to design what we want from our library.
HTH,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org