Re: [PATCH] Document and check svnsync revprops

David Glasser wrote:
> The following patch documents the meaning of the svnsync revprops and,
> at sync time, throws an error if they are not consistent.
>
> --dave
>
> [[[
> Make svnsync check more carefully that the destination repository's
> revprops and latest revision are consistent, and test this.
>
> * subversion/svnsync/main.c
> Document the revision properties at their #defines.
> (do_synchronize): Check that the newly-documented condition
> on latest-merged-rev, currently-copying, and latest_revision
> holds, throwing an error otherwise. Also, if a revision has
> completely copied except for resetting the currently-copying
> prop, don't recopy revprops.
>
> * subversion/tests/cmdline/svnsync_tests.py
> (run_sync): Allow tests which expect svnsync errors.
> (detect_meddling): New test which commits to the sync source
> and gets caught.
> (test_list): Add new test.
> ]]]

David, I reviewed, tweaked, and committed this patch in r20965. The key
tweaks I made are listed below (other include some formatting stuffs, mostly).

> === subversion/svnsync/main.c
> ==================================================================
> --- subversion/svnsync/main.c (revision 46915)
> +++ subversion/svnsync/main.c (local)
> @@ -29,11 +29,27 @@
> #include <apr_signal.h>
> #include <apr_uuid.h>
>
> +/* The following revision properties are set on revision 0 of
> + * destination repositories by svnsync: */
> #define PROP_PREFIX "svn:sync-"
>
> +/* Used to enforce mutually exclusive destination repository access. */
> #define LOCK_PROP PROP_PREFIX "lock"
> +/* Identifies the repository's source. */
> #define FROM_URL_PROP PROP_PREFIX "from-url"
> #define FROM_UUID_PROP PROP_PREFIX "from-uuid"
> +/* Identifies the last completely mirrored revision, and the revision
> + * (if any) that is in the process of being mirrored. (Mirroring is
> + * not an atomic action, because revision properties are copied
> + * separately from the revision's contents.
> + *
> + * At any time that currently-copying is not set, then last-merged-rev
> + * should be the HEAD revision of the destination repository.
> + *
> + * If currently-copying *is* set, it must be either last-merged-rev or
> + * last-merged-rev + 1, and the HEAD revision must be equal to either
> + * last-merged-rev or currently-copying. If this is not the case,
> + * somebody has meddled with the destination without using svnsync. */
> #define LAST_MERGED_REV_PROP PROP_PREFIX "last-merged-rev"
> #define CURRENTLY_COPYING_PROP PROP_PREFIX "currently-copying"

I moved this chunk of commentary down to the code that performs the sanity
checks.

> @@ -900,25 +916,43 @@
>
> {
> svn_string_t *currently_copying;
> - svn_revnum_t to_latest, copying;
> + svn_revnum_t to_latest, copying, last_merged;
>
> SVN_ERR(svn_ra_rev_prop(to_session, 0, CURRENTLY_COPYING_PROP,
> &currently_copying, pool));
>
> SVN_ERR(svn_ra_get_latest_revnum(to_session, &to_latest, pool));
> +
> + last_merged = atol (last_merged_rev->data);

Used SVN_STR_TO_REV() instead of atol() here.

>
> if (currently_copying)
> {
> copying = atol(currently_copying->data);

And here.

> === subversion/tests/cmdline/svnsync_tests.py

[...]

> + # Commit some change to the destination, which should be detected by svnsync
> + was_cwd = os.getcwd()
> + os.chdir(dest_sbox.wc_dir)
> + svntest.main.file_append(os.path.join('A', 'B', 'lambda'), 'new lambda text')
> + svntest.actions.run_and_verify_svn(None,
> + None,
> + [],
> + 'ci',
> + '-m', 'msg')
> + os.chdir(was_cwd)

I don't think all the chdir()ing is necessary, so I removed it (and instead
just used dest_sbox.wc_dir in the file_append and 'svn ci' paths).

> @@ -382,6 +429,7 @@
> copy_parent_modify_prop,
> basic_authz,
> copy_from_unreadable_dir,
> + detect_meddling,
> ]

I moved detect_meddling up to just prior to the two often-skipped tests.

-- 
C. Michael Pilato <cmpilato@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand