You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Chia-liang Kao <cl...@clkao.org> on 2003/07/11 18:41:45 UTC
[PATCH] "svn up" should not tree lock the anchor unless it's wc root
When doing "svn up", lock the anchor with write lock, and the traget
with treelock. but if anchor is working copy root or the target is
missing, lock the anchor with treelock.
The checking could be simplified if the accumulative locking API is
implemented.
* lisvn_client/update.c
(svn_client__update): narrow down the scope of tree lock if possible.
Index: subversion/libsvn_client/update.c
===================================================================
--- subversion/libsvn_client/update.c (revision 6432)
+++ subversion/libsvn_client/update.c (working copy)
@@ -55,17 +55,23 @@
svn_error_t *err;
svn_revnum_t revnum;
svn_wc_traversal_info_t *traversal_info = svn_wc_init_traversal_info (pool);
- svn_wc_adm_access_t *adm_access;
+ svn_wc_adm_access_t *adm_access, *target_access;
svn_boolean_t sleep_here = FALSE;
svn_boolean_t *use_sleep = timestamp_sleep ? timestamp_sleep : &sleep_here;
const char *diff3_cmd;
+ svn_node_kind_t kind;
/* Sanity check. Without this, the update is meaningless. */
assert (path);
/* Use PATH to get the update's anchor and targets and get a write lock */
SVN_ERR (svn_wc_get_actual_target (path, &anchor, &target, pool));
- SVN_ERR (svn_wc_adm_open (&adm_access, NULL, anchor, TRUE, TRUE, pool));
+ SVN_ERR (svn_io_check_path (path, &kind, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, anchor, TRUE,
+ target && (kind != svn_node_none) ? FALSE : TRUE,
+ pool));
+ SVN_ERR (svn_wc_adm_probe_try (&target_access, adm_access, path,
+ TRUE, TRUE, pool));
/* Get full URL from the ANCHOR. */
SVN_ERR (svn_wc_entry (&entry, anchor, adm_access, FALSE, pool));
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] "svn up" should not tree lock the anchor unless it's wc root
Posted by Chia-liang Kao <cl...@clkao.org>.
I do like elegant solutions of course, but my current knowledge with
the api and the internals only allows me to solve things this way at
this moment.
About this patch for 'svn up', I don't think it's uglier than the already
existing get_actual_target, as commetted by philip, it reads the entry
and discards it, and it's likely to be opened again immediately afterward.
but the patch it solves the problem, IMHO it's reasonable to move from
ugly A to ugly B, then clean C after all, if people do want to see the
addressed issue fixed, especially when it's about users' experience.
But I do respect the decision of the project team.
And for sure, I'll continue to dig deeper with the internals to try make
things cleaner. :)
Cheers,
CLK
On Mon, Jul 14, 2003 at 09:10:20AM -0500, cmpilato@collab.net wrote:
> > I don't like it, it all looks a bit ad-hoc. Do other people think we
> > should be applying patches like this? The patches probably work (I
> > haven't tried them) and so solve the problem Chia-liang Kao is having.
> > It's just that if the locking is going to be changed I'd prefer code
> > that is "more elegant", for some value of "elegant".
>
> I'm certainly opposed to this patch doing things this way, but I'm not
> sufficiently horrified to lump this nice volunteer's work into the
> "patches like this" Category o' Doom. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] "svn up" should not tree lock the anchor unless it's wc root
Posted by cm...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> >> Why is it necessary to check the node kind of path? The original code
> >> didn't do that.
> >
> > because it failed update_test#14 (update_deleted_missing_dir) without the
> > check. It seems in the case update would fail because it assumes other
> > directory under anchor is locked. so I just special-cased it to fallback the
> > original behavior - treelock the anchor.
>
> I don't like it, it all looks a bit ad-hoc. Do other people think we
> should be applying patches like this? The patches probably work (I
> haven't tried them) and so solve the problem Chia-liang Kao is having.
> It's just that if the locking is going to be changed I'd prefer code
> that is "more elegant", for some value of "elegant".
I'm certainly opposed to this patch doing things this way, but I'm not
sufficiently horrified to lump this nice volunteer's work into the
"patches like this" Category o' Doom. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] "svn up" should not tree lock the anchor unless it's wc
root
Posted by Philip Martin <ph...@codematters.co.uk>.
Chia-liang Kao <cl...@clkao.org> writes:
> On Sun, Jul 13, 2003 at 11:13:28AM +0100, Philip Martin wrote:
>> > - SVN_ERR (svn_wc_adm_open (&adm_access, NULL, anchor, TRUE, TRUE, pool));
>> > + SVN_ERR (svn_io_check_path (path, &kind, pool));
>> > + SVN_ERR (svn_wc_adm_open (&adm_access, NULL, anchor, TRUE,
>> > + target && (kind != svn_node_none) ? FALSE : TRUE,
>> > + pool));
>>
>> Why is it necessary to check the node kind of path? The original code
>> didn't do that.
>
> because it failed update_test#14 (update_deleted_missing_dir) without the
> check. It seems in the case update would fail because it assumes other
> directory under anchor is locked. so I just special-cased it to fallback the
> original behavior - treelock the anchor.
I don't like it, it all looks a bit ad-hoc. Do other people think we
should be applying patches like this? The patches probably work (I
haven't tried them) and so solve the problem Chia-liang Kao is having.
It's just that if the locking is going to be changed I'd prefer code
that is "more elegant", for some value of "elegant".
Consider 'svn up foo', in most cases it needs a non-recursive lock for
the parent of foo, and a recursive or non-recursive lock for foo
itself if foo is a versioned directory. This allows for foo to get
deleted during the update. The only possible exceptions are if foo is
a working copy root, or if foo is '.' (I suspect the existing code
doesn't handle deletes for these cases, think there is already an
issue for the related 'svn switch' problem.)
The locking code for 'svn rm' needs to do the same locking as 'svn up'
although 'svn rm' doesn't have a non-recursive case. I'd like to see
a simple function that does the right thing that both could share, it
should probably be used instead of svn_wc_get_actual_target function,
as that opens and reads the entries files, and then discards the
batons.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] "svn up" should not tree lock the anchor unless it's wc root
Posted by Chia-liang Kao <cl...@clkao.org>.
On Sun, Jul 13, 2003 at 11:13:28AM +0100, Philip Martin wrote:
> > - SVN_ERR (svn_wc_adm_open (&adm_access, NULL, anchor, TRUE, TRUE, pool));
> > + SVN_ERR (svn_io_check_path (path, &kind, pool));
> > + SVN_ERR (svn_wc_adm_open (&adm_access, NULL, anchor, TRUE,
> > + target && (kind != svn_node_none) ? FALSE : TRUE,
> > + pool));
>
> Why is it necessary to check the node kind of path? The original code
> didn't do that.
because it failed update_test#14 (update_deleted_missing_dir) without the
check. It seems in the case update would fail because it assumes other
directory under anchor is locked. so I just special-cased it to fallback the
original behavior - treelock the anchor.
Cheers,
CLK
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] "svn up" should not tree lock the anchor unless it's wc
root
Posted by Philip Martin <ph...@codematters.co.uk>.
Chia-liang Kao <cl...@clkao.org> writes:
> SVN_ERR (svn_wc_get_actual_target (path, &anchor, &target, pool));
> - SVN_ERR (svn_wc_adm_open (&adm_access, NULL, anchor, TRUE, TRUE, pool));
> + SVN_ERR (svn_io_check_path (path, &kind, pool));
> + SVN_ERR (svn_wc_adm_open (&adm_access, NULL, anchor, TRUE,
> + target && (kind != svn_node_none) ? FALSE : TRUE,
> + pool));
Why is it necessary to check the node kind of path? The original code
didn't do that.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] "svn up" should not tree lock the anchor unless it's wc root
Posted by Paul Lussier <pl...@lanminds.com>.
Filed as issue 1435;
http://subversion.tigris.org/issues/show_bug.cgi?id=1435
--
Seeya,
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org