You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2002/07/13 20:47:07 UTC
[PATCH] Revert r2504
Hello
I believe r2504 is incorrect, if we do not find the expected lock at
the start of post-commit processing then there is a bug somewhere. In
that circumstance it is wrong to do as r2504 does and try and acquire
the lock. There is an unexpected condition, we should not continue
and modify the working copy, that may do more harm then good. It may
also hide other bugs.
* subversion/libsvn_client/commit.c (svn_client_commit): Revert
r2504, just return an error instead.
Index: ../svn/subversion/libsvn_client/commit.c
===================================================================
--- ../svn/subversion/libsvn_client/commit.c
+++ ../svn/subversion/libsvn_client/commit.c Sat Jul 13 21:26:18 2002
@@ -919,15 +919,12 @@
svn_path_split_nts (item->path, &adm_access_path, NULL, pool);
adm_access = apr_hash_get (locked_dirs, adm_access_path,
APR_HASH_KEY_STRING);
-
- if ( ! adm_access )
- {
- SVN_ERR (svn_wc_adm_open (&adm_access, adm_access_path, TRUE,
- pool));
- apr_hash_set (locked_dirs, apr_pstrdup(pool, adm_access_path),
- APR_HASH_KEY_STRING, adm_access);
- }
-
+ if (! adm_access)
+ /* This should *never* happen */
+ return svn_error_createf(SVN_ERR_GENERAL, 0, NULL, pool,
+ "BUG: no lock for %s",
+ item->path);
+
if ((item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD)
&& (item->kind == svn_node_dir)
&& (item->copyfrom_url))
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Revert r2504
Posted by Philip Martin <ph...@codematters.co.uk>.
Justin Erenkrantz <je...@apache.org> writes:
> Here's a quick reproduction recipe:
>
> svnadmin create `pwd`/testrepos
> svn co file://`pwd`/testrepos -d test
> cd test
> mkdir foo
> touch foo/bar
> mkdir foo/foo2
> touch foo/foo2/spaz
> svn add --recursive foo
> svn commit
> svn cp foo bar
> echo "I am on the branch" > bar/foo2/spaz
> svn commit
At last, a receipe, thanks! I'll add it to my regression tests. I
wonder why a subdir of the copied bar is getting locked as well as
bar? Is it meant to, or is that a bug?
>
> The second commit will fail with a wc-locked error. With the
> patch to enter svn_wc_process_committed() with no locks held
> (and r2504 in place), it does not fail.
>
> I believe r2504 needs to stay until svn_wc_process_committed() can
> handle the above case with the locks held. At that point, the
Yes, I see. r2504 isn't enough as you said, we also need to unlock. I
still think unlocking during a commit is wrong.
> assumption that we can enter that function with the locks is correct
> (and r2504 can be reverted). As I have been trying to point out,
> that is currently a false assumption. -- justin
I have a patch that implements the parent-child relationship in the
locks. This will allow the entire tree to be locked in the pre-commit
processing. I'll try the new testcase on it.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Revert r2504
Posted by Justin Erenkrantz <je...@apache.org>.
On Sat, Jul 13, 2002 at 09:47:07PM +0100, Philip Martin wrote:
> Hello
>
> I believe r2504 is incorrect, if we do not find the expected lock at
> the start of post-commit processing then there is a bug somewhere. In
> that circumstance it is wrong to do as r2504 does and try and acquire
> the lock. There is an unexpected condition, we should not continue
> and modify the working copy, that may do more harm then good. It may
> also hide other bugs.
Based on my interpretation of the API, I think it would be valid
for the lock not to be held at the beginning of
svn_wc_process_committed().
In fact, that is a requirement in the code as it stands right now.
No locks in subdirectories may be held when svn_wc_process_committed
starts or it will attempt to recursively lock. Only the top-level
directory *may* be locked. The current code is broken right now with
recursive copies.
Here's a quick reproduction recipe:
svnadmin create `pwd`/testrepos
svn co file://`pwd`/testrepos -d test
cd test
mkdir foo
touch foo/bar
mkdir foo/foo2
touch foo/foo2/spaz
svn add --recursive foo
svn commit
svn cp foo bar
echo "I am on the branch" > bar/foo2/spaz
svn commit
The second commit will fail with a wc-locked error. With the
patch to enter svn_wc_process_committed() with no locks held
(and r2504 in place), it does not fail.
I believe r2504 needs to stay until svn_wc_process_committed() can
handle the above case with the locks held. At that point, the
assumption that we can enter that function with the locks is correct
(and r2504 can be reverted). As I have been trying to point out,
that is currently a false assumption. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org