Re: [PATCH v3] Reject checkouts to existing directory

Hi,

short ping. Anybody got time to review this?

Regards
Patrick

On Wed, Nov 09, 2016 at 12:54:08PM +0100, Patrick Steinhardt wrote:
> Hi,
>
> this is version three of my patch regarding checkouts to existing
> directories. Thanks for the feedback on the previous two patches.
>
> This patch requires my currently in-flight patch
> `svn_client_list4: accept `NULL patterns'. If it will not go into
> trunk, I'll convert `verify_checkout_target` to use an empty
> array instead of passing `NULL`.
>
> The new version fixes an issue where I did not check correctly
> whether the target directory is empty (that is I previously used
> `svn_path_is_empty`, where I now correctly use
> `svn_io_dir_empty`). Furthermore, I now check if the target is a
> working copy and, if so, compare UUID and relative paths of the
> wc and remote repository. These changes allowed me to drop all
> previously required '--force' flags in our test suite, indicating
> the changes now match more closely with existing use cases.
>
> I stopped short of adding a new '--force-obstructed-checkout'
> flag, which might be used instead of '--force' if the new checks
> reject the checkout. After reading '--force's doc string, it
> should actually do exactly what the new flag would be doing.
> Furthermore, I no hope that it's not required that frequently.
>
> Regards
> Patrick
>
> [[[
> checkout_cmd: refuse obstructed checkouts
>
> When a new checkout is done where the target dirctory already
> exists, subversion will usually create a lot of tree conflicts
> which are intimidating, especially to new users. This behavior
> stems from release 1.8, where we started accepting checkouts to
> existing directories without any safety-checks.
>
> This patch changes semantics in that it introduces new safety
> checks so that the user does not accidentally shoots himself into
> the foot. We now only allow checkouts if one of the following
> conditions holds true:
>
> - the target directory does not exist
> - the target directory is empty
> - the target directory is a repository and has the same UUID and
> relative path as the repository that is to be checked out
> - the repository to check out is empty
> - the --force flag is given
>
> The main use case solved by the above conditions is for
> converting existing directories into a repository when the
> repository is newly created as well as resuming checkouts.
>
> * subversion/svn/checkout-cmd.c:
> (listing_cb): New callback to check whether the remote
> repository is empty.
> (verify_checkout_target): New function to check whether the
> target checkout directory is a valid
> one.
> (svn_cl__checkout): Now calls `verify_checkout_target` if no
> --force flag is specified.
> * subversion/tests/cmdline/checkout_tests.py:
> (checkout_with_obstructions): Replace old test and now assert
> that subversion refuses to
> checkout to non-empty dirs.
> ]]]
> --
> Patrick Steinhardt, Entwickler
>
> elego Software Solutions GmbH, http://www.elego.de
> Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany
>
> Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
> Handelsregister: Amtsgericht Charlottenburg HRB 77719
> Geschäftsführer: Olaf Wagner

> diff --git a/subversion/svn/checkout-cmd.c b/subversion/svn/checkout-cmd.c
> index 56fd02b..b282248 100644
> --- a/subversion/svn/checkout-cmd.c
> +++ b/subversion/svn/checkout-cmd.c
> @@ -61,6 +61,144 @@
> Is this the same as CVS? Does it matter if it is not?
> */
>
> +static svn_error_t *
> +listing_cb(void *baton,
> + const char *path,
> + const svn_dirent_t *dirent,
> + const svn_lock_t *lock,
> + const char *abs_path,
> + const char *external_parent_url,
> + const char *external_target,
> + apr_pool_t *scratch_pool)
> +{
> + size_t *count = (size_t *) baton;
> +
> + (*count)++;
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Check if the target directory which should be checked out to
> + * is a valid target directory. A target directory is valid if we
> + * are sure that a checkout to the directory will not create any
> + * unexpected situations for the user. This is the case if one of
> + * the following criteria is true:
> + *
> + * - the target directory does not exist
> + * - the target directory is empty
> + * - the target directory is the same repository with the same
> + * relative URL as the one that is to be checked out (e.g. we
> + * resume a checkout)
> + * - the repository that is to be checked out is empty
> + */
> +static svn_error_t *
> +verify_checkout_target(const char *repo_url,
> + const char *target_dir,
> + const svn_opt_revision_t *peg_revision,
> + const svn_opt_revision_t *revision,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_node_kind_t kind;
> + apr_pool_t *scratch_pool;
> + const char *absolute_path;
> + size_t count = 0;
> + svn_wc_status3_t *status;
> + svn_error_t *error;
> + svn_boolean_t empty;
> +
> + scratch_pool = svn_pool_create(pool);
> +
> + SVN_ERR(svn_dirent_get_absolute(&absolute_path,
> + target_dir,
> + pool));
> +
> + SVN_ERR(svn_io_check_path(absolute_path,
> + &kind,
> + pool));
> +
> + /* If the directory does not exist yet, it will be created
> + * anew and is thus considered valid. */
> + if (kind == svn_node_none)
> + return SVN_NO_ERROR;
> +
> + /* Checking out to a file or symlink cannot work. */
> + if (kind != svn_node_dir)
> + return svn_error_createf(
> + SVN_ERR_ILLEGAL_TARGET,
> + NULL,
> + _("Rejecting checkout to existing %s '%s'"),
> + svn_node_kind_to_word(kind),
> + absolute_path);
> +
> + /* Checking out to a non-empty directory will create
> + * tree-conflicts, which is usually not what the user wants. */
> + SVN_ERR(svn_io_dir_empty(&empty, absolute_path, scratch_pool));
> + if (empty)
> + return SVN_NO_ERROR;
> +
> + error = svn_wc_status3(&status,
> + ctx->wc_ctx,
> + absolute_path,
> + pool,
> + scratch_pool);
> +
> + /* If the remote repository UUID and working copy UUID are the
> + * same and if the relative paths inside the repository match,
> + * we assume that the command is a resumed checkout. */
> + if (error == SVN_NO_ERROR)
> + {
> + const char *repo_relpath, *repo_root, *repo_uuid;
> +
> + SVN_ERR(svn_client_get_repos_root(&repo_root,
> + &repo_uuid,
> + repo_url,
> + ctx,
> + pool,
> + scratch_pool));
> +
> + repo_relpath = svn_uri_skip_ancestor(repo_root,
> + repo_url,
> + scratch_pool);
> +
> + if (repo_relpath
> + && !strcmp(status->repos_uuid, repo_uuid)
> + && !strcmp(status->repos_relpath, repo_relpath))
> + return SVN_NO_ERROR;
> + }
> + else
> + {
> + svn_error_clear(error);
> + }
> +
> + SVN_ERR(svn_client_list4(repo_url,
> + peg_revision,
> + revision,
> + NULL,
> + svn_depth_immediates,
> + 0,
> + FALSE,
> + FALSE,
> + listing_cb,
> + &count,
> + ctx,
> + pool));
> +
> + /* If the remote respotiory has more than one file (that is, it
> + * is not an empty root directory only), we refuse to check out
> + * into a non-empty target directory. Otherwise Subversion
> + * would create tree conflicts. */
> + if (count > 1)
> + return svn_error_createf(
> + SVN_ERR_ILLEGAL_TARGET,
> + NULL,
> + _("Rejecting checkout of non-empty repository into non-empty directory '%s'"),
> + absolute_path);
> +
> + svn_pool_clear(scratch_pool);
> +
> + return SVN_NO_ERROR;
> +}
>
> /* This implements the `svn_opt_subcommand_t' interface. */
> svn_error_t *
> @@ -165,6 +303,16 @@ svn_cl__checkout(apr_getopt_t *os,
> revision.kind = svn_opt_revision_head;
> }
>
> + if (! opt_state->force)
> + {
> + SVN_ERR(verify_checkout_target(true_url,
> + target_dir,
> + &peg_revision,
> + &revision,
> + ctx,
> + subpool));
> + }
> +
> SVN_ERR(svn_client_checkout3
> (NULL, true_url, target_dir,
> &peg_revision,
> diff --git a/subversion/tests/cmdline/checkout_tests.py b/subversion/tests/cmdline/checkout_tests.py
> index 49165e7..93415b9 100755
> --- a/subversion/tests/cmdline/checkout_tests.py
> +++ b/subversion/tests/cmdline/checkout_tests.py
> @@ -158,94 +158,20 @@ def make_local_tree(sbox, mod_files=False, add_unversioned=False):
> #----------------------------------------------------------------------
>
> def checkout_with_obstructions(sbox):
> - """co with obstructions conflicts without --force"""
> + """obstructed co without --force"""
>
> make_local_tree(sbox, False, False)
>
> - #svntest.factory.make(sbox,
> - # """# Checkout with unversioned obstructions lying around.
> - # svn co url wc_dir
> - # svn status""")
> - #svntest.factory.make(sbox,
> - # """# Now see to it that we can recover from the obstructions.
> - # rm -rf A iota
> - # svn up""")
> - #exit(0)
> -
> wc_dir = sbox.wc_dir
> url = sbox.repo_url
>
> # Checkout with unversioned obstructions causes tree conflicts.
> # svn co url wc_dir
> - expected_output = svntest.wc.State(wc_dir, {
> - 'iota' : Item(status=' ', treeconflict='C'),
> - 'A' : Item(status=' ', treeconflict='C'),
> - # And the updates below the tree conflict
> - 'A/D' : Item(status=' ', treeconflict='A'),
> - 'A/D/gamma' : Item(status=' ', treeconflict='A'),
> - 'A/D/G' : Item(status=' ', treeconflict='A'),
> - 'A/D/G/rho' : Item(status=' ', treeconflict='A'),
> - 'A/D/G/pi' : Item(status=' ', treeconflict='A'),
> - 'A/D/G/tau' : Item(status=' ', treeconflict='A'),
> - 'A/D/H' : Item(status=' ', treeconflict='A'),
> - 'A/D/H/chi' : Item(status=' ', treeconflict='A'),
> - 'A/D/H/omega' : Item(status=' ', treeconflict='A'),
> - 'A/D/H/psi' : Item(status=' ', treeconflict='A'),
> - 'A/B' : Item(status=' ', treeconflict='A'),
> - 'A/B/E' : Item(status=' ', treeconflict='A'),
> - 'A/B/E/beta' : Item(status=' ', treeconflict='A'),
> - 'A/B/E/alpha' : Item(status=' ', treeconflict='A'),
> - 'A/B/F' : Item(status=' ', treeconflict='A'),
> - 'A/B/lambda' : Item(status=' ', treeconflict='A'),
> - 'A/C' : Item(status=' ', treeconflict='A'),
> - 'A/mu' : Item(status=' ', treeconflict='A'),
> - })
> + expected_err = ".*Rejecting checkout of non-empty repository into non-empty directory.*"
>
> expected_disk = svntest.main.greek_state.copy()
> - expected_disk.remove('A/B', 'A/B/E', 'A/B/E/beta', 'A/B/E/alpha', 'A/B/F',
> - 'A/B/lambda', 'A/D', 'A/D/G', 'A/D/G/rho', 'A/D/G/pi', 'A/D/G/tau',
> - 'A/D/H', 'A/D/H/psi', 'A/D/H/omega', 'A/D/H/chi', 'A/D/gamma', 'A/C')
> -
> - actions.run_and_verify_checkout(url, wc_dir, expected_output,
> - expected_disk)
> -
> - # svn status
> - expected_status = actions.get_virginal_state(wc_dir, 1)
> - # A and iota are tree conflicted and obstructed
> - expected_status.tweak('A', 'iota', status='D ', wc_rev=1,
> - treeconflict='C')
> -
> - expected_status.tweak('A/D', 'A/D/G', 'A/D/G/rho', 'A/D/G/pi', 'A/D/G/tau',
> - 'A/D/H', 'A/D/H/chi', 'A/D/H/omega', 'A/D/H/psi', 'A/D/gamma', 'A/B',
> - 'A/B/E', 'A/B/E/beta', 'A/B/E/alpha', 'A/B/F', 'A/B/lambda', 'A/C',
> - status='D ')
> - # A/mu exists on disk, but is deleted
> - expected_status.tweak('A/mu', status='D ')
> -
> - actions.run_and_verify_unquiet_status(wc_dir, expected_status)
> -
> -
> - # Now see to it that we can recover from the obstructions.
> - # rm -rf A iota
> - svntest.main.safe_rmtree( os.path.join(wc_dir, 'A') )
> - os.remove( os.path.join(wc_dir, 'iota') )
> -
> -
> - svntest.main.run_svn(None, 'revert', '-R', os.path.join(wc_dir, 'A'),
> - os.path.join(wc_dir, 'iota'))
> -
> - # svn up
> - expected_output = svntest.wc.State(wc_dir, {
> - })
> -
> - expected_disk = svntest.main.greek_state.copy()
> -
> - expected_status = actions.get_virginal_state(wc_dir, 1)
> -
> - actions.run_and_verify_update(wc_dir, expected_output, expected_disk,
> - expected_status,)
> -
>
> + actions.run_and_verify_svn(None, expected_err, "co", url, wc_dir)
>
> #----------------------------------------------------------------------
>