On Fri, Nov 27, 2009 at 04:14:08PM -0000, Jon Foster wrote:
> Hi,
>
> As discussed on this list, we don't always need svnsync's networked
> lock. If svnsync only runs on a single server, the administrator
> can use the "flock" tool to prevent running multiple copies of
> svnsync at the same time.
>
> And if svnsync's lock is not needed, then it is actually an
> inconvenience. E.g. if the network connection fails, then a stale
> lock can be left behind. This requires manual administrator
> intervention to fix.
>
> One workaround is for your scripts to run this command immediately
> before they start svnsync:
> svn propdel svn:sync-lock --revprop -r0 $target_url
>
> However, it would be better to provide a command-line option to
> disable svnsync's locking. So, here's a patch to do that.
>
> The option name has been chosen to try to make it obvious that no
> locking is a bad idea, and administrators should either use
> svnsync's internal locking or have their own external locking.
>
> [[[
> Add --using-external-locking option to svnsync.
>
> * subversion/svnsync/main.c
> (svnsync__opt): Add svnsync_opt_using_external_locking.
> (SVNSYNC_OPTS_DEFAULT): Add svnsync_opt_using_external_locking.
> (svnsync_options): Add entry for --using-external-locking.
> (opt_baton_t): Add using_external_locking member.
> (initialize_cmd, synchronize_cmd, copy_revprops_cmd): Don't take the
> lock if using external locking.
> (main): Handle svnsync_opt_using_external_locking option.
>
> Patch by: Jon Foster <jon.fos...@cabot.co.uk>
> ]]]
Hi Jon,
I've finally found time to take a look at this. Sorry for taking so long.
I have committed your patch in r910212, with some tweaks.
Just so you know what the tweaks were:
> Index: subversion/svnsync/main.c
> ===================================================================
> --- subversion/svnsync/main.c (revision 884900)
> +++ subversion/svnsync/main.c (working copy)
> @@ -61,6 +61,7 @@
> svnsync_opt_sync_password,
> svnsync_opt_config_dir,
> svnsync_opt_config_options,
> + svnsync_opt_using_external_locking,
I've renamed this option to --disable-locking. Matter of taste, really.
> svnsync_opt_version,
> svnsync_opt_trust_server_cert,
> svnsync_opt_allow_non_empty,
> @@ -76,7 +77,8 @@
> svnsync_opt_sync_username, \
> svnsync_opt_sync_password, \
> svnsync_opt_config_dir, \
> - svnsync_opt_config_options
> + svnsync_opt_config_options, \
> + svnsync_opt_using_external_locking
You've added the option as a global option for all subcommands.
I've made the option specific to the 'init', 'sync', and 'copy-revprops'
subcommands instead. It's not useful with other subcommands.
>
> static const svn_opt_subcommand_desc2_t svnsync_cmd_table[] =
> {
> @@ -180,6 +182,10 @@
> "For example:\n"
> " "
> " servers:global:http-library=serf")},
> + {"using-external-locking", svnsync_opt_using_external_locking, 0,
> + N_("Disable built in locking. You must ensure that\n"
> + " "
> + "no other instance of svnsync is running.")},
I've tweaked the help text a bit, to make clear that the mirror
can get corrupted if this option is used and nothing is done
to avoid running multiple instances of svnsync in parallel.
> {"version", svnsync_opt_version, 0,
> N_("show program version information")},
> {"help", 'h', 0,
> @@ -201,6 +207,7 @@
> const char *sync_password;
> const char *config_dir;
> apr_hash_t *config;
> + svn_boolean_t using_external_locking;
> svn_boolean_t quiet;
> svn_boolean_t allow_non_empty;
> svn_boolean_t version;
> @@ -798,7 +805,14 @@
> SVN_ERR(svn_ra_open3(&to_session, baton->to_url, NULL,
> &(baton->sync_callbacks), baton, baton->config,
> pool));
> SVN_ERR(check_if_session_is_at_repos_root(to_session, baton->to_url,
> pool));
> - SVN_ERR(with_locked(to_session, do_initialize, baton, pool));
> + if (opt_baton->using_external_locking)
> + {
We don't require braces around single-line if-blocks, so I've removed them.
> + SVN_ERR(do_initialize(to_session, baton, pool));
> + }
> + else
> + {
> + SVN_ERR(with_locked(to_session, do_initialize, baton, pool));
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -1276,7 +1290,14 @@
> SVN_ERR(svn_ra_open3(&to_session, baton->to_url, NULL,
> &(baton->sync_callbacks), baton, baton->config,
> pool));
> SVN_ERR(check_if_session_is_at_repos_root(to_session, baton->to_url,
> pool));
> - SVN_ERR(with_locked(to_session, do_synchronize, baton, pool));
> + if (opt_baton->using_external_locking)
> + {
> + SVN_ERR(do_synchronize(to_session, baton, pool));
> + }
> + else
> + {
> + SVN_ERR(with_locked(to_session, do_synchronize, baton, pool));
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -1433,7 +1454,14 @@
> SVN_ERR(svn_ra_open3(&to_session, baton->to_url, NULL,
> &(baton->sync_callbacks), baton, baton->config,
> pool));
> SVN_ERR(check_if_session_is_at_repos_root(to_session, baton->to_url,
> pool));
> - SVN_ERR(with_locked(to_session, do_copy_revprops, baton, pool));
> + if (opt_baton->using_external_locking)
> + {
> + SVN_ERR(do_synchronize(to_session, baton, pool));
> + }
> + else
> + {
> + SVN_ERR(with_locked(to_session, do_synchronize, baton, pool));
> + }
This bit you added here is calling the wrong function.
It should call do_copy_revprops(), not do_synchronize().
>
> return SVN_NO_ERROR;
> }
> @@ -1664,6 +1692,10 @@
> if (err)
> return svn_cmdline_handle_exit_error(err, pool, "svnsync: ");
> break;
> +
> + case svnsync_opt_using_external_locking:
> + opt_baton.using_external_locking = TRUE;
> +
You've missed adding a 'break;' here. I've fixed this.
> case svnsync_opt_version:
> opt_baton.version = TRUE;
> break;
Thanks for the patch!
Stefan
