Hi, this is version 4 of my patch which aims to reject checkouts to obstructed targets in order to protect the user from doing unintended things. I've attached an interdiff to version 3. There are no changes to the logic, version 4 only addresses comments by Daniel:
- the listing callback now returns SVN_ERR_ITER_BREAK after
finding more than one entry, as we only need to know if there
is at least one file
- made pool usage more idiomatic
- renamed `error` variable to `err`
- renamed `absolute_path` to `target`
- changed code to handle unexpected errors of `svn_wc_status3`
- changed error messages to use `svn_dirent_local_style`
[[[
svn checkout: Refuse checkout to obstructed target directory.
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.
]]]
Regards
Patrick
--
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 56fd02b03c..0fa8ec8a78 100644
--- a/subversion/svn/checkout-cmd.c
+++ b/subversion/svn/checkout-cmd.c
@@ -61,6 +61,147 @@
Is this the same as CVS? Does it matter if it is not?
*/
+/* This implements the `svn_client_list_func2_t` interface. */
+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)++;
+
+ if ((*count) > 1)
+ return svn_error_create(SVN_ERR_ITER_BREAK, NULL, NULL);
+
+ 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 *scratch_pool)
+{
+ svn_node_kind_t kind;
+ const char *target;
+ size_t count = 0;
+ svn_wc_status3_t *status;
+ svn_error_t *err;
+ svn_boolean_t empty;
+
+ SVN_ERR(svn_dirent_get_absolute(&target,
+ target_dir,
+ scratch_pool));
+
+ SVN_ERR(svn_io_check_path(target,
+ &kind,
+ scratch_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 target '%s'"),
+ svn_dirent_local_style(target, scratch_pool));
+
+ /* 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, target, scratch_pool));
+ if (empty)
+ return SVN_NO_ERROR;
+
+ err = svn_wc_status3(&status,
+ ctx->wc_ctx,
+ target,
+ scratch_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 (err == 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,
+ scratch_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 if (err && err->apr_err == SVN_ERR_WC_NOT_WORKING_COPY)
+ svn_error_clear(err);
+ else
+ SVN_ERR(err);
+
+ err = svn_client_list4(repo_url,
+ peg_revision,
+ revision,
+ NULL,
+ svn_depth_immediates,
+ 0,
+ FALSE,
+ FALSE,
+ listing_cb,
+ &count,
+ ctx,
+ scratch_pool);
+
+ if (err && err->apr_err != SVN_ERR_ITER_BREAK)
+ SVN_ERR(err);
+ else
+ svn_error_clear(err);
+
+ /* If the remote repository 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'"),
+ svn_dirent_local_style(target, scratch_pool));
+
+ return SVN_NO_ERROR;
+}
/* This implements the `svn_opt_subcommand_t' interface. */
svn_error_t *
@@ -165,6 +306,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 49165e78fe..93415b9051 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)
#----------------------------------------------------------------------
diff --git a/subversion/svn/checkout-cmd.c b/subversion/svn/checkout-cmd.c
index b28224862f..0fa8ec8a78 100644
--- a/subversion/svn/checkout-cmd.c
+++ b/subversion/svn/checkout-cmd.c
@@ -61,6 +61,7 @@
Is this the same as CVS? Does it matter if it is not?
*/
+/* This implements the `svn_client_list_func2_t` interface. */
static svn_error_t *
listing_cb(void *baton,
const char *path,
@@ -75,6 +76,9 @@ listing_cb(void *baton,
(*count)++;
+ if ((*count) > 1)
+ return svn_error_create(SVN_ERR_ITER_BREAK, NULL, NULL);
+
return SVN_NO_ERROR;
}
@@ -97,25 +101,22 @@ verify_checkout_target(const char *repo_url,
const svn_opt_revision_t *peg_revision,
const svn_opt_revision_t *revision,
svn_client_ctx_t *ctx,
- apr_pool_t *pool)
+ apr_pool_t *scratch_pool)
{
svn_node_kind_t kind;
- apr_pool_t *scratch_pool;
- const char *absolute_path;
+ const char *target;
size_t count = 0;
svn_wc_status3_t *status;
- svn_error_t *error;
+ svn_error_t *err;
svn_boolean_t empty;
- scratch_pool = svn_pool_create(pool);
-
- SVN_ERR(svn_dirent_get_absolute(&absolute_path,
+ SVN_ERR(svn_dirent_get_absolute(&target,
target_dir,
- pool));
+ scratch_pool));
- SVN_ERR(svn_io_check_path(absolute_path,
+ SVN_ERR(svn_io_check_path(target,
&kind,
- pool));
+ scratch_pool));
/* If the directory does not exist yet, it will be created
* anew and is thus considered valid. */
@@ -127,26 +128,25 @@ verify_checkout_target(const char *repo_url,
return svn_error_createf(
SVN_ERR_ILLEGAL_TARGET,
NULL,
- _("Rejecting checkout to existing %s '%s'"),
- svn_node_kind_to_word(kind),
- absolute_path);
+ _("Rejecting checkout to existing target '%s'"),
+ svn_dirent_local_style(target, scratch_pool));
/* 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));
+ SVN_ERR(svn_io_dir_empty(&empty, target, scratch_pool));
if (empty)
return SVN_NO_ERROR;
- error = svn_wc_status3(&status,
- ctx->wc_ctx,
- absolute_path,
- pool,
- scratch_pool);
+ err = svn_wc_status3(&status,
+ ctx->wc_ctx,
+ target,
+ scratch_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)
+ if (err == SVN_NO_ERROR)
{
const char *repo_relpath, *repo_root, *repo_uuid;
@@ -154,7 +154,7 @@ verify_checkout_target(const char *repo_url,
&repo_uuid,
repo_url,
ctx,
- pool,
+ scratch_pool,
scratch_pool));
repo_relpath = svn_uri_skip_ancestor(repo_root,
@@ -166,25 +166,30 @@ verify_checkout_target(const char *repo_url,
&& !strcmp(status->repos_relpath, repo_relpath))
return SVN_NO_ERROR;
}
+ else if (err && err->apr_err == SVN_ERR_WC_NOT_WORKING_COPY)
+ svn_error_clear(err);
else
- {
- svn_error_clear(error);
- }
+ SVN_ERR(err);
- SVN_ERR(svn_client_list4(repo_url,
- peg_revision,
- revision,
- NULL,
- svn_depth_immediates,
- 0,
- FALSE,
- FALSE,
- listing_cb,
- &count,
- ctx,
- pool));
+ err = svn_client_list4(repo_url,
+ peg_revision,
+ revision,
+ NULL,
+ svn_depth_immediates,
+ 0,
+ FALSE,
+ FALSE,
+ listing_cb,
+ &count,
+ ctx,
+ scratch_pool);
- /* If the remote respotiory has more than one file (that is, it
+ if (err && err->apr_err != SVN_ERR_ITER_BREAK)
+ SVN_ERR(err);
+ else
+ svn_error_clear(err);
+
+ /* If the remote repository 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. */
@@ -193,9 +198,7 @@ verify_checkout_target(const char *repo_url,
SVN_ERR_ILLEGAL_TARGET,
NULL,
_("Rejecting checkout of non-empty repository into non-empty directory '%s'"),
- absolute_path);
-
- svn_pool_clear(scratch_pool);
+ svn_dirent_local_style(target, scratch_pool));
return SVN_NO_ERROR;
}
signature.asc
Description: PGP signature
