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) > > #---------------------------------------------------------------------- >
signature.asc
Description: PGP signature
