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 2001/10/17 20:19:20 UTC
the last M4 (M5) bugs
Ben Collins-Sussman <su...@collab.net> writes:
>
> > So I suspect that this Hudson Scenario has always failed over DAV,
> > ever since M2 last April. We just never noticed it, because we didn't
> > get the python tests working over DAV until last month. :-(
So here's the latest. M5 is done, and M4 is waiting on two bugs. The
bugs are both python-regression-test failures that work over ra_local,
but fail over ra_dav.
(These are two of the infamous "Greg Hudson" scenarios. If developers
are interested, look at doc/user/manual/dirversioning.texi for more
details.)
Here's are the ra_dav bugs and their proposed solutions. Greg Stein,
please feel free to comment on the solutions. Karl and I have figured
out ways to tweak the network layer to avoid these bugs, and we also
feel that the tweaks are theoretically correct, not just "hacks".
More importantly, it means we don't have to go rewrite the whole
commit system right now -- that can remain a Beta issue.
1. to replace a file, the user must {delete, add, commit}. If the
user instead attempts to {delete, commit, add, commit}, the second
commit will fail.
ANALYSIS
After the first commit, the working copy is in an interesting
state. The parent dir is at revision 1, and the deleted item is
at revision 2 but marked as 'deleted'. (This state will go away
if the user ever updates.) Meanwhile, the user schedules a new
item (of the same name) for addition, and tries to commit.
As cmpilato pointed out, the commit-driver does a replace_dir(1)
for the parent, and then tries to delete the item before adding
it. The commit-driver is assuming that some kind of "mirror" of
the working copy is being built on the server; it assumes that
it's editing a transaction based on revision 1, hence the delete
before the add. And in fact, this *is* how ra_local currently
works.
Of course, ra_local is building a transaction based on the HEAD
revision, and thus is merging-on-the-fly. When the commit-driver
tries to delete the item, mod_dav_svn justly complains that no
such item exists.
SOLUTION
mod_dav_svn should be trapping and ignoring the "item already
missing" error that it's getting from the filesystem. Why?
Because mod_dav_svn is *merging* on the fly, and in the world of
merging, deletion is an idempotent operation. (svn_fs_merge() has
the same rule: if you need to delete something, and it's already
gone, it just moves on.) This is a pretty easy fix, and I'm
almost done with it.
2. ra_dav is unable to prevent users from committing propchanges on
out-of-date dirs. Thus users could end up with working copies
containing dirs whose revision numbers are incorrect.
ANALYSIS
Well, duh. When ra_dav sends PROPPATCH on a directory,
mod_dav_svn is always applying it to the "latest" version of the
directory. There's a good chance that the change will apply
cleanly, and then ra_dav goes and bumps the directory revision --
but the whole thing should have failed an out-of-dateness check to
begin with.
SOLUTION
ra_dav can't know if the directory is out-of-date or not; only
someone looking at the filesystem can see that. So the change
needs to happen in mod_dav_svn.
Specifically, the "working" revision of the directory needs to be
sent in the PROPPATCH request body. mod_dav_svn can then look at
the node-rev-id of the latest version of the directory, see what
revision it was *created* in (it's part of the skel), and compare
it to the "working" revision from the client. It's an easy
out-of-dateness check that requires no undeltifying, and is a
foreshadowing of the commit-system of the future. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: the last M4 (M5) bugs
Posted by kf...@collab.net.
Greg Stein <gs...@lyra.org> writes:
> > All this change does is to implement the same merging logic that has
> > been in tree.c:merge() all along. Remember, we're merging into HEAD
> > here -- if the request is to delete entry E, and it turns out that
> > entry E is already gone,
>
> What if E *never* existed?
>
> IMO, I'd call that an error which should be trapped.
That case is impossible, because the user *has* a resource E locally,
and her records indicate E was versioned in this directory at some
point in the past (and the directory's existence on the server *is*
checked). For her to attempt to delete it on the repository now and
discover it's already gone just means the work she wanted to do is
already done.
Now, I'll grant you that someone could tweak their wc, or manually
send deletion commands over the network, or whatever, to attempt to
delete something that never existed at that path. So what? It's not
important to answer spoofs. :-) Note that the user's records have been
verified as accurate right up to the parent directory of E, so it's
not like this method will fail to detect a mismatch between wc and
repos.
> But simply throwing out an error doesn't have the case of "never existed".
> When I meant "deleted originally", I meant the initial request-to-delete of
> something that just doesn't exist. (as opposed to a second deletion of
> something which existed but had been deleted before)
Then the repository should detect if the thing ever existed here in
the past (which it is perfectly capable of doing).
> So... maybe we have some issues distinguishing the two cases, and more
> getting them reported back to the client. But tossing the error doesn't seem
> like the right solution.
The question here is whether or not it *is* an error. :-) If we
determine it isn't, then tossing it is the right solution, although
"tossing the error" might be a hair-raising buzzword designed to cause
riots in the streets of San Jose. :-)
Anyway, as we all just agreed on IRC, we'll do this by ignoring
ra_dav's 404 on the deletion, and everyone's happy .
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: the last M4 (M5) bugs
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Oct 18, 2001 at 03:15:59PM -0500, kfogel@collab.net wrote:
> Greg Stein <gs...@lyra.org> writes:
> > > mod_dav_svn should be trapping and ignoring the "item already
> > > missing" error that it's getting from the filesystem. Why?
> > > Because mod_dav_svn is *merging* on the fly, and in the world of
> > > merging, deletion is an idempotent operation. (svn_fs_merge() has
> > > the same rule: if you need to delete something, and it's already
> > > gone, it just moves on.) This is a pretty easy fix, and I'm
> > > almost done with it.
> >
> > I don't understand how this can be the correct fix. What if a DELETE is
> > attempted and the file *truly* isn't there? I see no way to distinguish
> > between "deleting *again*" and "deleting *originally*"
>
> That's because there *is* no difference. :-)
>...
> Hmmm, not sure what two cases you're trying to distinguish here...
>
> All this change does is to implement the same merging logic that has
> been in tree.c:merge() all along. Remember, we're merging into HEAD
> here -- if the request is to delete entry E, and it turns out that
> entry E is already gone,
What if E *never* existed?
IMO, I'd call that an error which should be trapped.
> than that means the deletion was already done
> by someone else. No problem -- not an error, it just means that you
> attempting to delete the entry is not a conflict (as opposed to, say,
> trying to modify its text). This has always been part of the merge
> logic, see this empty `else' clause in tree.c:
>
> else
> {
> /* It's a double delete, so do nothing.
> ### kff todo: what about the rename case? */
> }
One tree deleted something, and the other tree requested a delete of that
same thing. Sure... that is fine because the requested-to-delete was
deleting something that used to exist.
But simply throwing out an error doesn't have the case of "never existed".
When I meant "deleted originally", I meant the initial request-to-delete of
something that just doesn't exist. (as opposed to a second deletion of
something which existed but had been deleted before)
So... maybe we have some issues distinguishing the two cases, and more
getting them reported back to the client. But tossing the error doesn't seem
like the right solution.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: the last M4 (M5) bugs
Posted by kf...@collab.net.
Greg Stein <gs...@lyra.org> writes:
> > mod_dav_svn should be trapping and ignoring the "item already
> > missing" error that it's getting from the filesystem. Why?
> > Because mod_dav_svn is *merging* on the fly, and in the world of
> > merging, deletion is an idempotent operation. (svn_fs_merge() has
> > the same rule: if you need to delete something, and it's already
> > gone, it just moves on.) This is a pretty easy fix, and I'm
> > almost done with it.
>
> I don't understand how this can be the correct fix. What if a DELETE is
> attempted and the file *truly* isn't there? I see no way to distinguish
> between "deleting *again*" and "deleting *originally*"
That's because there *is* no difference. :-)
> We cannot simply toss out an error message if it accidentally captures the
> second issue.
>
> IMO, we just mark this as an inconvenience until the WC is fixed. If you
> want to "delete, commit, add, commit", then the user must instead do
> "delete, commit, *update*, add, commit". Just tell them to stick an extra
> update in there for the odd chance that they committed between the delete
> and add.
>
> Basically: why the dubious act of ignoring an error, which was originally
> caused by erroneous behavior on the client. And if we have a low-pain
> workaround, then let's simply use that one.
Hmmm, not sure what two cases you're trying to distinguish here...
All this change does is to implement the same merging logic that has
been in tree.c:merge() all along. Remember, we're merging into HEAD
here -- if the request is to delete entry E, and it turns out that
entry E is already gone, than that means the deletion was already done
by someone else. No problem -- not an error, it just means that you
attempting to delete the entry is not a conflict (as opposed to, say,
trying to modify its text). This has always been part of the merge
logic, see this empty `else' clause in tree.c:
else
{
/* It's a double delete, so do nothing.
### kff todo: what about the rename case? */
}
(The rename case is irrelevant to this discussion, don't be misled by
that comment.)
I think Ben has a mail already composed responding to the second half
(below), so I'll sign off here and we'll see what he says...
-K
> > 2. ra_dav is unable to prevent users from committing propchanges on
> > out-of-date dirs. Thus users could end up with working copies
> > containing dirs whose revision numbers are incorrect.
> >
> > ANALYSIS
> >
> > Well, duh. When ra_dav sends PROPPATCH on a directory,
> > mod_dav_svn is always applying it to the "latest" version of the
> > directory. There's a good chance that the change will apply
> > cleanly, and then ra_dav goes and bumps the directory revision --
> > but the whole thing should have failed an out-of-dateness check to
> > begin with.
> >
> > SOLUTION
> >
> > ra_dav can't know if the directory is out-of-date or not; only
> > someone looking at the filesystem can see that. So the change
> > needs to happen in mod_dav_svn.
> >
> > Specifically, the "working" revision of the directory needs to be
> > sent in the PROPPATCH request body. mod_dav_svn can then look at
> > the node-rev-id of the latest version of the directory, see what
> > revision it was *created* in (it's part of the skel), and compare
> > it to the "working" revision from the client. It's an easy
> > out-of-dateness check that requires no undeltifying, and is a
> > foreshadowing of the commit-system of the future. :-)
>
> No. You cannot send the working revision in the body. Blech.
>
> A CHECKOUT is performed before doing the PROPPATCH. That checkout uses the
> version url from when the directory was initially checked out. The system
> should have punted on that initial CHECKOUT. If it didn't, then the question
> is "why?"
>
> Two answers that I can see:
>
> 1) the client had the wrong version url stored
> 2) the server does the comparison wrong
>
> Karl did some work on #1 recently, maybe that fixed this problem? In any
> case, tracking that URL is a precondition to fixing this issue. We need to
> know if it is getting bumped improperly (e.g. the version url must remain
> unchanged w.r.t revision; if the revision doesn't change, neither does the
> url).
>
> For the second, I'd be a bit surprised. It does the check for other
> resources. There is a bit of special casing in the code, with a comment
> about it (although rereading the comment, it isn't perfectly clear why, so
> maybe the comment needs some more tweaking).
>
>
>
> So on the problems, I'd say the first isn't an issue right now, and the
> second needs a bit of investigation before any whacking on it.
>
> Cheers,
> -g
>
> --
> Greg Stein, http://www.lyra.org/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: the last M4 (M5) bugs
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
>
> > How could either system know to punt?
>
> ID mismatch
>
> > I mean, directory CHECKOUTS are done all the time -- not just before
> > proppatching a directory, but anytime we edit a file too (the file's
> > parent is checked out first.)
>
> We never do a CHECKOUT on the parent directory to edit a file. We check out
> the file itself, then PUT changes or PROPPATCH props.
>
> The parent is checked out when we add or delete files.
Ah, right. I see it there in commit.c.
> > So -- if the server receives a PROPPATCH on a directory, is there any
> > way mod_dav_svn can "recall" the vsn_url that was used to initially
> > checkout the directory? If so, it can examine the vsn_url and reject
> > the PROPPATCH as out-of-date.
>
> Hmm. So the problem is that we need to allow out-of-date checkouts cuz of
> the add/delete problem. On the other hand, we must enforce them for a
> proppatch.
>
> That seems like the crux of the problem, no?
Exactly.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: the last M4 (M5) bugs
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Oct 18, 2001 at 03:11:36PM -0500, Ben Collins-Sussman wrote:
> Greg Stein <gs...@lyra.org> writes:
> > > Specifically, the "working" revision of the directory needs to be
> > > sent in the PROPPATCH request body. mod_dav_svn can then look at
> > > the node-rev-id of the latest version of the directory, see what
> > > revision it was *created* in (it's part of the skel), and compare
> > > it to the "working" revision from the client. It's an easy
> > > out-of-dateness check that requires no undeltifying, and is a
> > > foreshadowing of the commit-system of the future. :-)
> >
> > No. You cannot send the working revision in the body. Blech.
> >
> > A CHECKOUT is performed before doing the PROPPATCH. That checkout uses the
> > version url from when the directory was initially checked out. The system
> > should have punted on that initial CHECKOUT. If it didn't, then the question
> > is "why?"
>
> The "system"? Do you mean client, or server?
Both. All parts that comprise the system. In this case, it would really be
mod_dav_svn that punted on the CHECKOUT.
> How could either system know to punt?
ID mismatch
> I mean, directory CHECKOUTS are done all the time -- not just before
> proppatching a directory, but anytime we edit a file too (the file's
> parent is checked out first.)
We never do a CHECKOUT on the parent directory to edit a file. We check out
the file itself, then PUT changes or PROPPATCH props.
The parent is checked out when we add or delete files.
> So -- if the server receives a PROPPATCH on a directory, is there any
> way mod_dav_svn can "recall" the vsn_url that was used to initially
> checkout the directory? If so, it can examine the vsn_url and reject
> the PROPPATCH as out-of-date.
Hmm. So the problem is that we need to allow out-of-date checkouts cuz of
the add/delete problem. On the other hand, we must enforce them for a
proppatch.
That seems like the crux of the problem, no?
Cheers,
-g
p.s. either way: passing info during PROPPATCH is wrong
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: the last M4 (M5) bugs
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
> > Specifically, the "working" revision of the directory needs to be
> > sent in the PROPPATCH request body. mod_dav_svn can then look at
> > the node-rev-id of the latest version of the directory, see what
> > revision it was *created* in (it's part of the skel), and compare
> > it to the "working" revision from the client. It's an easy
> > out-of-dateness check that requires no undeltifying, and is a
> > foreshadowing of the commit-system of the future. :-)
>
> No. You cannot send the working revision in the body. Blech.
>
> A CHECKOUT is performed before doing the PROPPATCH. That checkout uses the
> version url from when the directory was initially checked out. The system
> should have punted on that initial CHECKOUT. If it didn't, then the question
> is "why?"
The "system"? Do you mean client, or server?
How could either system know to punt?
I mean, directory CHECKOUTS are done all the time -- not just before
proppatching a directory, but anytime we edit a file too (the file's
parent is checked out first.)
So -- if the server receives a PROPPATCH on a directory, is there any
way mod_dav_svn can "recall" the vsn_url that was used to initially
checkout the directory? If so, it can examine the vsn_url and reject
the PROPPATCH as out-of-date.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: the last M4 (M5) bugs
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Oct 17, 2001 at 03:19:20PM -0500, Ben Collins-Sussman wrote:
>...
> 1. to replace a file, the user must {delete, add, commit}. If the
> user instead attempts to {delete, commit, add, commit}, the second
> commit will fail.
>...
> mod_dav_svn should be trapping and ignoring the "item already
> missing" error that it's getting from the filesystem. Why?
> Because mod_dav_svn is *merging* on the fly, and in the world of
> merging, deletion is an idempotent operation. (svn_fs_merge() has
> the same rule: if you need to delete something, and it's already
> gone, it just moves on.) This is a pretty easy fix, and I'm
> almost done with it.
I don't understand how this can be the correct fix. What if a DELETE is
attempted and the file *truly* isn't there? I see no way to distinguish
between "deleting *again*" and "deleting *originally*"
We cannot simply toss out an error message if it accidentally captures the
second issue.
IMO, we just mark this as an inconvenience until the WC is fixed. If you
want to "delete, commit, add, commit", then the user must instead do
"delete, commit, *update*, add, commit". Just tell them to stick an extra
update in there for the odd chance that they committed between the delete
and add.
Basically: why the dubious act of ignoring an error, which was originally
caused by erroneous behavior on the client. And if we have a low-pain
workaround, then let's simply use that one.
> 2. ra_dav is unable to prevent users from committing propchanges on
> out-of-date dirs. Thus users could end up with working copies
> containing dirs whose revision numbers are incorrect.
>
> ANALYSIS
>
> Well, duh. When ra_dav sends PROPPATCH on a directory,
> mod_dav_svn is always applying it to the "latest" version of the
> directory. There's a good chance that the change will apply
> cleanly, and then ra_dav goes and bumps the directory revision --
> but the whole thing should have failed an out-of-dateness check to
> begin with.
>
> SOLUTION
>
> ra_dav can't know if the directory is out-of-date or not; only
> someone looking at the filesystem can see that. So the change
> needs to happen in mod_dav_svn.
>
> Specifically, the "working" revision of the directory needs to be
> sent in the PROPPATCH request body. mod_dav_svn can then look at
> the node-rev-id of the latest version of the directory, see what
> revision it was *created* in (it's part of the skel), and compare
> it to the "working" revision from the client. It's an easy
> out-of-dateness check that requires no undeltifying, and is a
> foreshadowing of the commit-system of the future. :-)
No. You cannot send the working revision in the body. Blech.
A CHECKOUT is performed before doing the PROPPATCH. That checkout uses the
version url from when the directory was initially checked out. The system
should have punted on that initial CHECKOUT. If it didn't, then the question
is "why?"
Two answers that I can see:
1) the client had the wrong version url stored
2) the server does the comparison wrong
Karl did some work on #1 recently, maybe that fixed this problem? In any
case, tracking that URL is a precondition to fixing this issue. We need to
know if it is getting bumped improperly (e.g. the version url must remain
unchanged w.r.t revision; if the revision doesn't change, neither does the
url).
For the second, I'd be a bit surprised. It does the check for other
resources. There is a bit of special casing in the code, with a comment
about it (although rereading the comment, it isn't perfectly clear why, so
maybe the comment needs some more tweaking).
So on the problems, I'd say the first isn't an issue right now, and the
second needs a bit of investigation before any whacking on it.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org