Re: svn commit: r38800 - trunk/subversion/svnsync

Didn't we already go through the discussion of "don't use --force
anymore" ?? ie. use a more descriptive option like
--overwrite-exist-revisions ?

The name --force is not descriptive of what it actually does. Which
means we need all kinds of supporting documentation to detail it. But
if somebody doesn't read the documentation and uses --force according
to an assumption of *another* meaning... they could run into
unexpected and disastrous results.

For the few cases where something like this would be needed, I have no
qualms making people type a long option name.

Cheers,
-g

On Tue, Aug 18, 2009 at 16:18, C. Michael Pilato<cmpilato_at_collab.net> wrote:
> Author: cmpilato
> Date: Tue Aug 18 07:18:50 2009
> New Revision: 38800
>
> Log:
> Make 'svnsync init' accept a --force flag which overrides the sanity
> checks ("source has revisions", "source already has svn:sync-* props")
> so that folks can easily init (or re-init) a repository backup as a
> new mirror.
>
> * subversion/svnsync/main.c
>  (enum svnsync__opt): New 'svnsync_opt_force' value.
>  (svnsync_cmd_table): Make 'initialize' subcommand accept --force,
>    and tweak usage message to explain why that's useful.
>  (struct subcommand_baton_t, opt_baton_t): Add 'force' member.
>  (make_subcommand_baton): Init new 'force' member of subcommand baton.
>  (do_initialize, main): Handle the new --force option.
>
> Modified:
>   trunk/subversion/svnsync/main.c
>
> Modified: trunk/subversion/svnsync/main.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svnsync/main.c?pathrev=38800&r1=38799&r2=38800
> ==============================================================================
> --- trunk/subversion/svnsync/main.c     Tue Aug 18 07:16:37 2009        (r38799)
> +++ trunk/subversion/svnsync/main.c     Tue Aug 18 07:18:50 2009        (r38800)
> @@ -60,7 +60,8 @@ enum svnsync__opt {
>   svnsync_opt_config_dir,
>   svnsync_opt_config_options,
>   svnsync_opt_version,
> -  svnsync_opt_trust_server_cert
> +  svnsync_opt_trust_server_cert,
> +  svnsync_opt_force
>  };
>
>  #define SVNSYNC_OPTS_DEFAULT svnsync_opt_non_interactive, \
> @@ -83,18 +84,24 @@ static const svn_opt_subcommand_desc2_t
>          "Initialize a destination repository for synchronization from\n"
>          "another repository.\n"
>          "\n"
> -         "The destination URL must point to the root of a repository with\n"
> -         "no committed revisions.  The destination repository must allow\n"
> -         "revision property changes.\n"
> -         "\n"
>          "If the source URL is not the root of a repository, only the\n"
>          "specified part of the repository will be synchronized.\n"
>          "\n"
> +         "The destination URL must point to the root of a repository which\n"
> +         "has been configured to allow revision property changes.  In\n"
> +         "the general case, the destination repository must contain no\n"
> +         "committed revisions.  Use --force to override this restriction,\n"
> +         "but understand that svnsync will assume that any revisions\n"
> +         "already present in the destination repository perfectly mirror\n"
> +         "their counterparts in the source repository.  (This is useful\n"
> +         "when initializing a repository made from a copy of a repository\n"
> +         "as a mirror of that same repository, for example.)\n"
> +         "\n"
>          "You should not commit to, or make revision property changes in,\n"
>          "the destination repository by any method other than 'svnsync'.\n"
>          "In other words, the destination repository should be a read-only\n"
>          "mirror of the source repository.\n"),
> -      { SVNSYNC_OPTS_DEFAULT, 'q' } },
> +      { SVNSYNC_OPTS_DEFAULT, 'q', svnsync_opt_force } },
>     { "synchronize", synchronize_cmd, { "sync" },
>       N_("usage: svnsync synchronize DEST_URL\n"
>          "\n"
> @@ -135,6 +142,8 @@ static const apr_getopt_option_t svnsync
>   {
>     {"quiet",          'q', 0,
>                        N_("print as little as possible") },
> +    {"force",          svnsync_opt_force, 0,
> +                       N_("force operation to run") },
>     {"non-interactive", svnsync_opt_non_interactive, 0,
>                        N_("do no interactive prompting") },
>     {"no-auth-cache",  svnsync_opt_no_auth_cache, 0,
> @@ -191,6 +200,7 @@ typedef struct {
>   const char *config_dir;
>   apr_hash_t *config;
>   svn_boolean_t quiet;
> +  svn_boolean_t force;
>   svn_boolean_t version;
>   svn_boolean_t help;
>  } opt_baton_t;
> @@ -307,6 +317,7 @@ typedef struct {
>   svn_ra_callbacks2_t source_callbacks;
>   svn_ra_callbacks2_t sync_callbacks;
>   svn_boolean_t quiet;
> +  svn_boolean_t force;
>   const char *to_url;
>
>   /* initialize only */
> @@ -704,6 +715,7 @@ make_subcommand_baton(opt_baton_t *opt_b
>   b->sync_callbacks.open_tmp_file = open_tmp_file;
>   b->sync_callbacks.auth_baton = opt_baton->sync_auth_baton;
>   b->quiet = opt_baton->quiet;
> +  b->force = opt_baton->force;
>   b->to_url = to_url;
>   b->from_url = from_url;
>   b->start_rev = start_rev;
> @@ -725,26 +737,24 @@ do_initialize(svn_ra_session_t *to_sessi
>  {
>   svn_ra_session_t *from_session;
>   svn_string_t *from_url;
> -  svn_revnum_t latest;
> +  svn_revnum_t latest, from_latest;
>   const char *uuid, *root_url;
>   int normalized_rev_props_count;
>
> -  /* First, sanity check to see that we're copying into a brand new repos. */
> -
> +  /* First, sanity check to see that we're copying into a brand new
> +     repos.  If we aren't, and we aren't being asked to forcibly
> +     complete this initialization, that's a bad news.  */
>   SVN_ERR(svn_ra_get_latest_revnum(to_session, &latest, pool));
> -
> -  if (latest != 0)
> +  if ((latest != 0) && (! baton->force))
>     return svn_error_create
>       (APR_EINVAL, NULL,
> -       _("Cannot initialize a repository with content in it"));
> -
> -  /* And check to see if anyone's run initialize on it before...  We
> -     may want a --force option to override this check. */
> +       _("Destination repository already contains revision history; consider "
> +         "using --force if the revisions in the repository are known to be "
> +         "a mirror of the respective revisions in the source repository"));
>
>   SVN_ERR(svn_ra_rev_prop(to_session, 0, SVNSYNC_PROP_FROM_URL,
>                           &from_url, pool));
> -
> -  if (from_url)
> +  if (from_url && (! baton->force))
>     return svn_error_createf
>       (APR_EINVAL, NULL,
>        _("Destination repository is already synchronizing from '%s'"),
> @@ -777,25 +787,41 @@ do_initialize(svn_ra_session_t *to_sessi
>                                 NULL);
>     }
>
> +  /* If we're initializing a non-empty destination, we'll make sure
> +     that it at least doesn't have more revisions than the source. */
> +  if (latest != 0)
> +    {
> +      SVN_ERR(svn_ra_get_latest_revnum(from_session, &from_latest, pool));
> +      if (from_latest < latest)
> +        return svn_error_create
> +          (APR_EINVAL, NULL,
> +           _("Destination repository has more revisions than source "
> +             "repository"));
> +    }
> +
>   SVN_ERR(svn_ra_change_rev_prop(to_session, 0, SVNSYNC_PROP_FROM_URL,
>                                  svn_string_create(baton->from_url, pool),
>                                  pool));
>
>   SVN_ERR(svn_ra_get_uuid2(from_session, &uuid, pool));
> -
>   SVN_ERR(svn_ra_change_rev_prop(to_session, 0, SVNSYNC_PROP_FROM_UUID,
>                                  svn_string_create(uuid, pool), pool));
>
>   SVN_ERR(svn_ra_change_rev_prop(to_session, 0, SVNSYNC_PROP_LAST_MERGED_REV,
> -                                 svn_string_create("0", pool), pool));
> -
> -  /* Finally, copy all non-svnsync revprops from rev 0 of the source
> -     repos into the dest repos. */
> +                                 svn_string_createf(pool, "%ld", latest),
> +                                 pool));
>
> -  SVN_ERR(copy_revprops(from_session, to_session, 0, FALSE,
> +  /* Copy all non-svnsync revprops from the LATEST rev in the source
> +     repository into the destination, notifying about normalized
> +     props, if any.  When LATEST is 0, this serves the practical
> +     purpose of initializing data that would otherwise be overlooked
> +     by the sync process (which is going to begin with r1).  When
> +     LATEST is not 0, this really serves merely aesthetic and
> +     informational purposes, keeping the output of this command
> +     consistent while allowing folks to see what the latest revision is.  */
> +  SVN_ERR(copy_revprops(from_session, to_session, latest, FALSE,
>                         baton->quiet, &normalized_rev_props_count, pool));
> -
> -  /* Notify about normalized props, if any. */
> +
>   SVN_ERR(log_properties_normalized(normalized_rev_props_count, 0, pool));
>
>   /* TODO: It would be nice if we could set the dest repos UUID to be
> @@ -2225,6 +2251,10 @@ main(int argc, const char *argv[])
>             opt_baton.version = TRUE;
>             break;
>
> +          case svnsync_opt_force:
> +            opt_baton.force = TRUE;
> +            break;
> +
>           case 'q':
>             opt_baton.quiet = TRUE;
>             break;
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=2384785
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2384816
Received on 2009-08-18 17:50:54 CEST