You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Neels J Hofmeyr <ne...@elego.de> on 2010/05/05 09:37:09 UTC
another hole in tree-conflicts! -- was: Re: svn commit: r940111
- in /subversion/trunk/subversion: include/private/svn_wc_private.h include/svn_wc.h
libsvn_wc/node.c libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
On 05/03/2010 10:25 PM, Daniel Näslund wrote:
> On Mon, May 03, 2010 at 03:07:49PM +0200, Neels J Hofmeyr wrote:
[...]
>>> stat->lock_creation_date = 0;
>>> + stat->conflicted = (tree_conflict != NULL);
>>
>> I don't understand -- what about prop and text conflicts? Below,
>> ->conflicted stands for all three of them.
>> (If this is correct, I guess it needs a comment)
>
> Added a comment in r940600. AFAIK, the only scenario where we can get a
> conflict on an unversioned node is for an incoming delete to a locally
> deleted path. (It will be troublesome to fit that case into the idea of
> storing tree conflict info on the node instead of its parent).
ah, ok, that makes sense. Shouldn't be trouble to store such tree-conflict
in the ng-ACTUAL tree? ...but, thinking aloud a little:
The entry can't be NULL for a schedule-deleted node (asked for with
show_hidden == TRUE). Analogously, the wc-ng tables do have both ng-BASE and
ng-WORKING nodes for a locally deleted path.
So, if there is no metadata in the WC on the node, there can't be *any*
(tree-)conflict on it! Right? Let's see:
Another case: obstructed; there is no metadata in the WC, but a file/folder
sits at a path. Say update wants to put something at that path: it'll create
a base node (above theory holds -- TC needs to have metadata).
Ok, furthermore, say merge wants to put something there, at the obstructed
path. It'll create a working node that is added (obstructed_add). Again,
there is a WC metadata entry.
Ok, even furthermore, say there is nothing, absolutely nothing at WC path
foo.c, and I do a two-URL merge. The URL I merge from has foo.c on the left,
but no foo.c on the right. So technically, the merge should want to delete
foo.c in my WC, where it never existed in any way. (I never thought about
this before!) How does merge handle that?
* neels writes a test script
[[[
[foo.c never existed on trunk, and r3 deletes foo.c on unrelated branch]
+ svn merge -c3 '^/branch' trunk/
--- Merging r3 into 'trunk':
C trunk/foo.c
--- Recording mergeinfo for merge of r3 into 'trunk':
U trunk
Summary of conflicts:
Tree conflicts: 1
+ svn info trunk/foo.c
Path: trunk/foo.c
Name: foo.c
Node Kind: none
Tree conflict: local delete, incoming delete upon merge
Source left: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@2
Source right: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@3
+ svn status trunk
M trunk
! C trunk/foo.c
> local delete, incoming delete upon merge
Summary of conflicts:
Tree conflicts: 1
]]]
Good grief, another hole in tree-conflicts! It says it was locally deleted
and status '!', but it never existed locally. It should rather say something
like... er... 'missing' doesn't really hit home either. 'local void,
incoming delete' ?? Do we need another conflict_reason to flag this?
Resolving this automatically would definitely need different behaviour than
a truly locally deleted node or truly missing node.
* neels attaches the test script
Is this the real justification why there could possibly be a tree-conflict
on an entry==NULL node?
> Thanks for your review,
heh, I'm glad I can say some intelligent stuff, too. Usually it was Julian
telling me about my patches in much the same way. :)
~Neels
RE: another hole in tree-conflicts! -- was: Re: svn commit: r940111 - in /subversion/trunk/subversion: include/private/svn_wc_private.h include/svn_wc.h libsvn_wc/node.c libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Neels J Hofmeyr [mailto:neels@elego.de]
> Sent: woensdag 5 mei 2010 11:37
> To: dev@subversion.apache.org; Julian Foad; Stefan Sperling; Stephen
> Butler
> Subject: another hole in tree-conflicts! -- was: Re: svn commit:
> r940111 - in /subversion/trunk/subversion:
> include/private/svn_wc_private.h include/svn_wc.h libsvn_wc/node.c
> libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
>
> On 05/03/2010 10:25 PM, Daniel Näslund wrote:
> > On Mon, May 03, 2010 at 03:07:49PM +0200, Neels J Hofmeyr wrote:
> [...]
> >>> stat->lock_creation_date = 0;
> >>> + stat->conflicted = (tree_conflict != NULL);
> >>
> >> I don't understand -- what about prop and text conflicts? Below,
> >> ->conflicted stands for all three of them.
> >> (If this is correct, I guess it needs a comment)
> >
> > Added a comment in r940600. AFAIK, the only scenario where we can get
> a
> > conflict on an unversioned node is for an incoming delete to a
> locally
> > deleted path. (It will be troublesome to fit that case into the idea
> of
> > storing tree conflict info on the node instead of its parent).
>
> ah, ok, that makes sense. Shouldn't be trouble to store such tree-
> conflict
> in the ng-ACTUAL tree? ...but, thinking aloud a little:
>
> The entry can't be NULL for a schedule-deleted node (asked for with
> show_hidden == TRUE). Analogously, the wc-ng tables do have both ng-
> BASE and
> ng-WORKING nodes for a locally deleted path.
This is the case where a node was deleted locally (schedule delete) and
deleted remotely.
After the update we couldn't leave it schedule delete as the entry was
removed from the repository, so the complete node was removed in 1.6 and a
tree conflict recorded in the parent *entry*.
(All cases where we have a node would fall through a different codepath in
the status walk)
We discussed this scenario a few weeks ago on irc, and there are two ways to
recording this in WC-NG:
1. Allow an ACTUAL_NODE record for nodes without BASE_NODE and WORKING_NODE
2. Keep a NOT-PRESENT BASE_NODE record for this case
I recommended option 2, as it doesn't introduce another status just for
recording this conflict type which we would have to check everywhere.
(Adding another status would require every loop over the result of
svn_wc__db_read_children() to check for this status).
> So, if there is no metadata in the WC on the node, there can't be *any*
> (tree-)conflict on it! Right? Let's see:
>
> Another case: obstructed; there is no metadata in the WC, but a
> file/folder
> sits at a path. Say update wants to put something at that path: it'll
> create
> a base node (above theory holds -- TC needs to have metadata).
In WC-1.0 we record this on the parent, without an entry.
In WC-NG we can create the node, but with an 'obstructed' conflict on it.
(So we can allow the 'svn update to continue')
The rest of your mail talks about how merge handles this conflict, which is
a less interesting case for the wc-ng work where I'm looking at as it is a
client feature. (But it might require recording a conflict on a not-present
node anyway)
Bert
Re: another hole in tree-conflicts!
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-05-05 at 11:37 +0200, Neels J Hofmeyr wrote:
> On 05/03/2010 10:25 PM, Daniel Näslund wrote:
> > On Mon, May 03, 2010 at 03:07:49PM +0200, Neels J Hofmeyr wrote:
> [...]
> >>> stat->lock_creation_date = 0;
> >>> + stat->conflicted = (tree_conflict != NULL);
> >>
> >> I don't understand -- what about prop and text conflicts? Below,
> >> ->conflicted stands for all three of them.
> >> (If this is correct, I guess it needs a comment)
> >
> > Added a comment in r940600. AFAIK, the only scenario where we can get a
> > conflict on an unversioned node is for an incoming delete to a locally
> > deleted path.
You must be talking about during an update (or switch) that results in a
tree conflict, because if it was a *merge* then the node wouldn't become
unversioned yet, it would still be scheduled for deletion, so it would
still have an "entry" and there would be no problem in storing the
metadata. That's what I'm assuming in the rest of this email.
So what about an incoming delete to a locally *edited* path, during
up/sw? That is another combination that would also leave the node
unversioned and with a tree conflict.
> (It will be troublesome to fit that case into the idea of
> > storing tree conflict info on the node instead of its parent).
>
> ah, ok, that makes sense. Shouldn't be trouble to store such tree-conflict
> in the ng-ACTUAL tree? ...but, thinking aloud a little:
>
> The entry can't be NULL for a schedule-deleted node (asked for with
> show_hidden == TRUE). Analogously, the wc-ng tables do have both ng-BASE and
> ng-WORKING nodes for a locally deleted path.
>
> So, if there is no metadata in the WC on the node, there can't be *any*
> (tree-)conflict on it! Right?
I'm not sure if you're making a statement about how we should design
WC-NG tree conflict handling, or what.
> Let's see:
>
> Another case: obstructed; there is no metadata in the WC, but a file/folder
> sits at a path. Say update wants to put something at that path: it'll create
> a base node (above theory holds -- TC needs to have metadata).
That's good.
> Ok, furthermore, say merge wants to put something there, at the obstructed
> path. It'll create a working node that is added (obstructed_add). Again,
> there is a WC metadata entry.
That's good.
> Ok, even furthermore, say there is nothing, absolutely nothing at WC path
> foo.c, and I do a two-URL merge. The URL I merge from has foo.c on the left,
> but no foo.c on the right. So technically, the merge should want to delete
> foo.c in my WC, where it never existed in any way. (I never thought about
> this before!)
That's the case commonly known as "incoming delete, locally deleted on
my branch (and committed)".
> How does merge handle that?
>
> * neels writes a test script
>
> [[[
> [foo.c never existed on trunk, and r3 deletes foo.c on unrelated branch]
> + svn merge -c3 '^/branch' trunk/
> --- Merging r3 into 'trunk':
> C trunk/foo.c
> --- Recording mergeinfo for merge of r3 into 'trunk':
> U trunk
> Summary of conflicts:
> Tree conflicts: 1
> + svn info trunk/foo.c
> Path: trunk/foo.c
> Name: foo.c
> Node Kind: none
> Tree conflict: local delete, incoming delete upon merge
> Source left: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@2
> Source right: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@3
>
> + svn status trunk
> M trunk
> ! C trunk/foo.c
> > local delete, incoming delete upon merge
> Summary of conflicts:
> Tree conflicts: 1
> ]]]
>
> Good grief, another hole in tree-conflicts! It says it was locally deleted
That's all good IF we understand "deleted" to mean "not here" or
"deleted, possibly a long time ago on this branch".
> and status '!',
Ah, that status doesn't look right. It should have a space instead of
"!", I think.
> but it never existed locally. It should rather say something
> like... er... 'missing' doesn't really hit home either. 'local void,
> incoming delete' ?? Do we need another conflict_reason to flag this?
It would be better if we recorded a different reason. My suggestion was
always "not here".
> Resolving this automatically would definitely need different behaviour than
> a truly locally deleted node or truly missing node.
>
> * neels attaches the test script
>
> Is this the real justification why there could possibly be a tree-conflict
> on an entry==NULL node?
Yes, I believe so.
Stefan Sperling wrote:
> What it should really be doing is realise that the node never existed
> in the branch and so deleting it is not a conflict (I know svn is not
> that smart yet and won't be for a while...)
>
> But I think the missing status notification is just fine in the
> meantime.
> I agree the text should say "local missing" instead of "local delete",
> but IIRC we don't know the difference since we're not searching branch
> history [yet].
About the reporting of the "local" state
----------------------------------------
I recommend the following states for "not present":
* "not here" if the node is not on the tip of the branch
* "deleted" if the node is scheduled for deletion
* "missing" if the node is supposed to be present but is not found on
the disk
"Missing" is true in a general English sense for any of these states,
but it might be better to reserve it to describe the case where the
metadata says the node is present but it's not there on disk.
Similarly, "local delete" could, in a general English sense, mean "it's
not here and so was presumably deleted by somebody, some time,
somewhere", but instead we should reserve it for its normal Subversion
meaning of "scheduled for deletion on the next commit".
We can determine the difference between these three states by looking at
the local WC; that doesn't require searching the history of the branch.
(If we later want to indicate more precisely what happened to this file
in the history of the branch, it's not as simple as saying "was deleted
on branch" because that doesn't necessarily mean "was present at branch
creation and was deleted exactly once in its history" - there are
several other variants possible.)
- Julian
Re: another hole in tree-conflicts! -- was: Re: svn commit:
r940111 - in /subversion/trunk/subversion:
include/private/svn_wc_private.h include/svn_wc.h libsvn_wc/node.c
libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, May 05, 2010 at 11:37:09AM +0200, Neels J Hofmeyr wrote:
> Ok, even furthermore, say there is nothing, absolutely nothing at WC path
> foo.c, and I do a two-URL merge. The URL I merge from has foo.c on the left,
> but no foo.c on the right. So technically, the merge should want to delete
> foo.c in my WC, where it never existed in any way. (I never thought about
> this before!) How does merge handle that?
>
> * neels writes a test script
>
> [[[
> [foo.c never existed on trunk, and r3 deletes foo.c on unrelated branch]
> + svn merge -c3 '^/branch' trunk/
> --- Merging r3 into 'trunk':
> C trunk/foo.c
> --- Recording mergeinfo for merge of r3 into 'trunk':
> U trunk
> Summary of conflicts:
> Tree conflicts: 1
> + svn info trunk/foo.c
> Path: trunk/foo.c
> Name: foo.c
> Node Kind: none
> Tree conflict: local delete, incoming delete upon merge
> Source left: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@2
> Source right: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@3
>
> + svn status trunk
> M trunk
> ! C trunk/foo.c
> > local delete, incoming delete upon merge
> Summary of conflicts:
> Tree conflicts: 1
> ]]]
>
> Good grief, another hole in tree-conflicts! It says it was locally deleted
> and status '!', but it never existed locally. It should rather say something
> like... er... 'missing' doesn't really hit home either. 'local void,
> incoming delete' ?? Do we need another conflict_reason to flag this?
> Resolving this automatically would definitely need different behaviour than
> a truly locally deleted node or truly missing node.
What it should really be doing is realise that the node never existed
in the branch and so deleting it is not a conflict (I know svn is not
that smart yet and won't be for a while...)
But I think the missing status notification is just fine in the meantime.
I agree the text should say "local missing" instead of "local delete",
but IIRC we don't know the difference since we're not searching branch
history [yet].
Stefan