You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2003/06/19 22:32:46 UTC
issue 919 solution
I want to see if people are coolt with my planned solution to issue 919.
But first, here's a quick recap of the bug:
### start with a standard 'greek-tree' at r1
$ svn up -r0 iota
D iota
$ svn up
### nothing happens; iota never comes back.
The reason the bug happens is that iota's entry is removed
permanently, rather than marked as 'deleted'. This causes the
parent directory to lose track of its proper list of children; in
the second update, libsvn_wc erroneously tells the server it has a
complete copy of r1 of the directory... so the server does nothing.
We've solved this 'ghudson'-class bug before, but specifically in the
case of commits. When we commit a deletion, we mark the item as
'deleted' in parent (in post-commit-processing) IFF the item's revnum
is later than the parent.
But now it seems we need to do something similar in the update case.
So I've zoomed in on the libsvn_wc update_editor. This editor is very
much aware of the anchor/target situation. Thinking about it for a
while, it seems to be that bug 919 *only* happens in situations where
the update target is non-NULL (i.e. you're updating a specific child
within a versioned directory.) That's the only scenario in which the
child might be deleted, but the parent's revnum go unchanged.
So I think the delete_entry() function can grow some new logic: if the
thing being deleted *is* the target of the update, then we can have
its entry marked 'deleted'. Very simple.
(Philip, does this sound right to you?)
[P.S. I also thought of a "sneaky but lazy" alternative
solution... instead of marking the child as 'deleted', we can mark the
parent as 'incomplete'. This guarantees the next update of the parent
will be one of the new 'low confidence' reports. Heck, we might even
be able to get rid of the *whole* 'deleted' flag this way!]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Greg Hudson <gh...@MIT.EDU>.
On Thu, 2003-06-19 at 19:42, Branko Čibej wrote:
> Even better, instead of marking the directory incomplete, the right
> solution would be to set the entry's revision correctly -- in this case,
> the "correct" revision would be SVN_INVALID_REVNUM, since the thing
> doesn't exist.
I wonder if it wouldn't be better to use the actual revision where the
file doesn't exist. If I "svn up -r0 filename", then that revision is 0
(but it might be something else, of course); if I svn rm a file and
commit that, then it's the newly committed revision. Of course, we
would also need an entry flag for "this entry doesn't exist as of this
rev", so that the code knows not to expect a base text. But when we're
reporting to the server, we don't have to check this flag and call
delete_entry(); instead, we just pass in the revision where the file
doesn't exist.
I can't immediately think of any case where it's important to remember
which deleted revision we're at, but it feels better to me to preserve
this information.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Branko Čibej <br...@xbc.nu>.
Ben Collins-Sussman wrote:
>[P.S. I also thought of a "sneaky but lazy" alternative
>solution... instead of marking the child as 'deleted', we can mark the
>parent as 'incomplete'. This guarantees the next update of the parent
>will be one of the new 'low confidence' reports.
>
I think this is a better solution because it doesn't try to lie to the
WC or server. If you updated an entry to revisoin 0, you _didn't_ delete
it. It would look really weird if "svn st" mentioned it as such, IMHO.
Even better, instead of marking the directory incomplete, the right
solution would be to set the entry's revision correctly -- in this case,
the "correct" revision would be SVN_INVALID_REVNUM, since the thing
doesn't exist. The reporter could then do exactly the right thing --
tell the server that particular entry is missing.
I expect the reporter already knows how to do that, or at least "almost"
knows -- it may be just a question of SVN_INVALID_REVNUM being a valid
value in the tree report.
> Heck, we might even
>be able to get rid of the *whole* 'deleted' flag this way!]
>
>
Nah. "deleted" has its own, very specific meaning.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Jack Repenning <jr...@collab.net>.
At 7:08 PM -0500 6/19/03, Ben Collins-Sussman wrote:
>1. The 'deleted' entry might have been created long ago in post-commit
> situation.
Does this mean that 'deleted' means "svn rm'ed"?
In that case
>It's starting to look like we need to add another entry flag, one
>which indicates the history/purpose of the 'deleted' entry.
Well, yeah: no existing flag means what you want to say, of course
you need a new one.
>I think I may go mad...
Only if you lose track of your own Monte!
--
-==-
Jack Repenning
CollabNet, Inc.
8000 Marina Boulevard, Suite 600
Brisbane, California 94015
o: 650.228.2562
c: 408.835-8090
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Ben Collins-Sussman <su...@collab.net>.
Ben Collins-Sussman <su...@collab.net> writes:
> So I think your approach will work better, Philip:
>
> 1. have delete_entry() mark the entry 'deleted' no matter what, and
> attach the update's target_revnum to it.
>
> 2. have the cleanup code (tweak_entry()) compare revnums. If the
> 'deleted' entry revnum is the same as the parent, then it's safe
> to remove the entry altogether (i.e. the delete was part of
> updating the parent, not an action on an isolated target.)
Nope, this doesn't work either. We're really screwed here.
Suppose we're in the post-update processing, happily bumping working
revs. We see a 'deleted' entry with a revnum lower than its parent's
revnum.
What does it mean?
1. The 'deleted' entry might have been created long ago in post-commit
situation. That would indicate it's time to remove deleted child
forever. (The user updated to HEAD, and server thought it was
supposed to be gone, so it didn't say anything).
2. The 'deleted' entry might have been created in our backdate-a-file
scenario. So of course the deleted child still has a lower revnum
than its parent; but in this case, we want the deleted child to
*stay*, to ensure better more accurate reports to the server.
It's starting to look like we need to add another entry flag, one
which indicates the history/purpose of the 'deleted' entry.
I think I may go mad...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Ben Collins-Sussman <su...@collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> > So I think the delete_entry() function can grow some new logic: if the
> > thing being deleted *is* the target of the update, then we can have
> > its entry marked 'deleted'. Very simple.
> >
> > (Philip, does this sound right to you?)
>
> I was wondering if we could do it without any special case code: have
> delete_entry (or some low level function?) always mark the item
> 'deleted', then the existing code to remove 'deleted' flags would
> clean-up when the parent was updated.
Hmmm, well, it seems that in practice, my proposal isn't working
right. delete_entry() sees that (eb->target != NULL), so after
deleting the entry, it recreates a 'deleted' entry.
But then in close_edit(), we call svn_wc__do_update_cleanup() which
ultimately calls svn_wc__tweak_entry(), which decides to
*deliberately* remove all 'deleted' files. This logic was assuming
that the 'deleted' state could only happen in post-commit
processing... and that "if the server didn't overwrite it, then it
meant for the thing to be gone!"
So I think your approach will work better, Philip:
1. have delete_entry() mark the entry 'deleted' no matter what, and
attach the update's target_revnum to it.
2. have the cleanup code (tweak_entry()) compare revnums. If the
'deleted' entry revnum is the same as the parent, then it's safe
to remove the entry altogether (i.e. the delete was part of
updating the parent, not an action on an isolated target.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Collins-Sussman <su...@collab.net> writes:
> So I think the delete_entry() function can grow some new logic: if the
> thing being deleted *is* the target of the update, then we can have
> its entry marked 'deleted'. Very simple.
>
> (Philip, does this sound right to you?)
I was wondering if we could do it without any special case code: have
delete_entry (or some low level function?) always mark the item
'deleted', then the existing code to remove 'deleted' flags would
clean-up when the parent was updated.
> [P.S. I also thought of a "sneaky but lazy" alternative
> solution... instead of marking the child as 'deleted', we can mark the
> parent as 'incomplete'. This guarantees the next update of the parent
> will be one of the new 'low confidence' reports. Heck, we might even
> be able to get rid of the *whole* 'deleted' flag this way!]
I guess that's less efficient, at least as far as client-server
communication is concerned? Anything to make 'deleted' more robust
would be welcome, that's partly why I was considering the idea of
always setting the 'deleted' flag--make it normal rather than special.
(Although this may just move the special case, deletion of schedule
add might perhaps become 'special'.)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by ju...@cox.net.
>>>>> "BS" == Ben Collins-Sussman <su...@collab.net> writes:
BS> 'incomplete' != 'aborted'. Not at all! It means that the directory
BS> isn't 100% confident that it has a correct/complete list of entries.
BS> ...
BS> For example... this is how we do checkouts now: we create a root
BS> directory with a single entry for '.', mark it 'incomplete', and run
BS> 'svn update'. Has nothing to do with abortions or failures.
The "incomplete" route may also solve the asymmetric behaviour [*]
between "svn add -N" vs "svn co -N", I would presume. If that
is the case I would very much like it.
[*] Here is the asymmetry I am annoyed about.
When you are adding a tree with children, you could say:
$ ls dir
file1 little-subdirectory file3 huge-subdirectory
$ svn add -N dir
$ cd dir
$ svn add file1 little-subdirectory
$ cd ..
$ svn commit dir
to selectively add to the set of things to be placed in the
repository. But checkout does not work that way nicely:
$ svn co -N dir
would create dir, but it does not seem to know what hangs under
it, so the following after the above checkout would not give me
little-subdirectory nor file1.
$ cd dir
$ svn update
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Ben Collins-Sussman <su...@collab.net>.
Jack Repenning <jr...@collab.net> writes:
> But if you mark a directory "incomplete" when it's actually properly
> formed, merely because that same code will not notice its confusion,
> trustingly do the same thing, and luck into the behavior we want in
> this case as well ... well, you should maybe investigate a side career
> in Three Card Monte! :-)
Alas, you speak...... wisely. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Jack Repenning <jr...@collab.net>.
At 5:58 PM -0500 6/19/03, Ben Collins-Sussman wrote:
>'incomplete' != 'aborted'. Not at all! It means that the directory
>isn't 100% confident that it has a correct/complete list of entries.
Still now what we have here: we have complete confidence that we've
deleted the entry.
>And the only thing that makes use of this flag is 'svn update'.
There you go again, analyzing all the known code.
>For example... this is how we do checkouts now: we create a root
>directory with a single entry for '.',
... which is to say, "incomplete."
>mark it 'incomplete',
... thereby speaking a truth.
>and run 'svn update'.
Which sees this true state description and does the right thing. Bully for it.
But if you mark a directory "incomplete" when it's actually properly
formed, merely because that same code will not notice its confusion,
trustingly do the same thing, and luck into the behavior we want in
this case as well ... well, you should maybe investigate a side
career in Three Card Monte! :-)
--
-==-
Jack Repenning
CollabNet, Inc.
8000 Marina Boulevard, Suite 600
Brisbane, California 94015
o: 650.228.2562
c: 408.835-8090
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Ben Collins-Sussman <su...@collab.net>.
Jack Repenning <jr...@collab.net> writes:
> >[P.S. I also thought of a "sneaky but lazy" alternative
> >solution... instead of marking the child as 'deleted', we can mark the
> >parent as 'incomplete'.
>
> Ah, yes, thanks: great counter-example! The 'incomplete' state means
> "something was aborted," I believe.
'incomplete' != 'aborted'. Not at all! It means that the directory
isn't 100% confident that it has a correct/complete list of entries.
And the only thing that makes use of this flag is 'svn update'. When
libsvn_wc begins an update-report and sees the flag, it says, "ok,
time to resort to the cvs style of reporting: iterate all entries to
the server, rather than just throwing out a directory revnum."
For example... this is how we do checkouts now: we create a root
directory with a single entry for '.', mark it 'incomplete', and run
'svn update'. Has nothing to do with abortions or failures.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: issue 919 solution
Posted by Jack Repenning <jr...@collab.net>.
At 5:32 PM -0500 6/19/03, Ben Collins-Sussman wrote:
>So I think the delete_entry() function can grow some new logic: if the
>thing being deleted *is* the target of the update, then we can have
>its entry marked 'deleted'. Very simple.
I leave to others to check your claim that this is the only case.
But marking it 'deleted' because it's being deleted? Lemme see ...
how could that be right? ;-)
Actually, of course, it sounds exactly right, because it's simply
telling the truth (/me likes truth). If there is some code somewhere
else that doesn't like having deleted files marked 'deleted,' ...
well ... *it's* wrong!
>[P.S. I also thought of a "sneaky but lazy" alternative
>solution... instead of marking the child as 'deleted', we can mark the
>parent as 'incomplete'.
Ah, yes, thanks: great counter-example! The 'incomplete' state means
"something was aborted," I believe. Recycling it (or even
"rethinking" it) for this new state is scary, because the new meaning
is not the same as the old. Some thought on all known code paths
suggests it'll do the right thing, but what about the code paths
unconsidered, including the ones not yet written? As a general
thing, I can imagine a great many reactions to "something aborted"
that are inappropriate for the situation "something was carefully and
completely deleted."
--
-==-
Jack Repenning
CollabNet, Inc.
8000 Marina Boulevard, Suite 600
Brisbane, California 94015
o: 650.228.2562
c: 408.835-8090
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org