Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Date: Tue, 18 Oct 2016 14:22:25 +0200
From: Patrick Steinhardt <patrick.steinhardt@elegosoft.com>
To: Subversion <dev@subversion.apache.org>
Cc: Stefan Sperling <stsp@elegosoft.com>, Ivan Zhakov <ivan@visualsvn.com>,
	Stefan <luke1410@posteo.de>
Subject: [PATCH v2] Reject checkouts to existing directory
Message-ID: <20161018122225.GA6726@pks-xps>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="LpQ9ahxlCli8rRTG"
Content-Disposition: inline
archived-at: Tue, 18 Oct 2016 12:22:43 -0000


--LpQ9ahxlCli8rRTG
Content-Type: multipart/mixed; boundary="2oS5YaxWCcQjTEyO"
Content-Disposition: inline


--2oS5YaxWCcQjTEyO
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

finally got around to update my patch regarding checkouts to
existing directories. The semantics have been changed to accept
checkouts iff

- the target directory does not exist
- the target directory is empty
- the repository to check out is empty
- the --force flag is given

This should treat all cases pointed out by Ivan. There were quite
a lot of test cases that needed adjustment following this change.
I've mostly added the '--force' flag, but also replaced a test
which didn't make sense anymore with a new one (that is 'co to a
dir without --force with obstructions'). The new test case simply
check is we reject checking out a non-empty repo to a non-empty
dir.

All but two unrelated tests pass with this.

[[[
Reject checkouts which would create tree conflicts.

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 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.

* 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 is specified.
* subversion/tests/cmdline/autoprop_tests.py:
  (autoprops_test,
   inheritable_autoprops_test): use 'co --force'.
* subversion/tests/cmdline/basic_tests.py:
  (basic_checkout,
   basic_auth_test): use 'co --force'.
* subversion/tests/cmdline/checkout_tests.py:
  (checkout_with_obstructions): Replace old test and now assert
   that subversion refuses to checkout to non-empty dirs.
* subversion/tests/cmdline/svntest/actions.py:
  (run_and_verify_checkout): use 'co --force'.
* subversion/tests/cmdline/tree_conflict_tests.py:
  (ensure_tree_conflict): use 'co --force'.
]]]

Regards
Patrick
--=20
Patrick Steinhardt, Entwickler

elego Software Solutions GmbH, http://www.elego.de
Geb=E4ude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany

Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Gesch=E4ftsf=FChrer: Olaf Wagner

--2oS5YaxWCcQjTEyO
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="checkout-to-existing.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/subversion/svn/checkout-cmd.c b/subversion/svn/checkout-cmd.c
index 56fd02b..b78ee74 100644
--- a/subversion/svn/checkout-cmd.c
+++ b/subversion/svn/checkout-cmd.c
@@ -61,6 +61,87 @@
   Is this the same as CVS?  Does it matter if it is not?
 */
=20
+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 =3D (size_t *) baton;
+
+  (*count)++;
+
+  return SVN_NO_ERROR;
+}
+
+static svn_error_t *
+verify_checkout_target(const char *path_or_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;
+  const char *absolute_path;
+  size_t count =3D 0;
+
+  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 =3D=3D svn_node_none)
+    return SVN_NO_ERROR;
+
+  /* Checking out to a file or symlink cannot work. */
+  if (kind !=3D 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. */
+  if (svn_path_is_empty(absolute_path))
+    return SVN_NO_ERROR;
+
+  SVN_ERR(svn_client_list3(path_or_url,
+                           peg_revision,
+                           revision,
+                           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 directo=
ry '%s'"),
+      absolute_path);
+
+  return SVN_NO_ERROR;
+}
=20
 /* This implements the `svn_opt_subcommand_t' interface. */
 svn_error_t *
@@ -165,6 +246,16 @@ svn_cl__checkout(apr_getopt_t *os,
           revision.kind =3D svn_opt_revision_head;
       }
=20
+      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/autoprop_tests.py b/subversion/tests/=
cmdline/autoprop_tests.py
index 0987cf1..b5c538b 100755
--- a/subversion/tests/cmdline/autoprop_tests.py
+++ b/subversion/tests/cmdline/autoprop_tests.py
@@ -169,7 +169,7 @@ def autoprops_test(sbox, cmd, cfgenable, clienable, sub=
dir):
   # do an svn co if needed
   if cmd =3D=3D 'import':
     svntest.main.run_svn(None, 'checkout', repos_url, files_wc_dir,
-                        '--config-dir', config_dir)
+                        '--force', '--config-dir', config_dir)
=20
   # check the properties
   if enable_flag:
@@ -514,7 +514,7 @@ def inheritable_autoprops_test(sbox, cmd, cfgenable, cl=
ienable, subdir,
     # do an svn co if needed
     if cmd =3D=3D 'import':
       svntest.main.run_svn(None, 'checkout', repos_url + '/A', files_wc_di=
r,
-                          '--config-dir', config_dir)
+                          '--force', '--config-dir', config_dir)
=20
     check_inheritable_autoprops(sbox, auto_props_cfg_enabled,
                                 inheritable_auto_props_enabled)
diff --git a/subversion/tests/cmdline/basic_tests.py b/subversion/tests/cmd=
line/basic_tests.py
index 0ad7316..b7d7078 100755
--- a/subversion/tests/cmdline/basic_tests.py
+++ b/subversion/tests/cmdline/basic_tests.py
@@ -88,7 +88,7 @@ def basic_checkout(sbox):
   url =3D sbox.repo_url
=20
   svntest.actions.run_and_verify_svn(None, [],
-                                     'co', url,
+                                     'co', '--force', url,
                                      wc_dir)
=20
   # lambda is restored, modifications remain, deletes remain scheduled
@@ -2546,22 +2546,22 @@ def basic_auth_test(sbox):
   # Checkout with jrandom
   exit_code, output, errput =3D svntest.main.run_command(
     svntest.main.svn_binary, None, True, 'co', sbox.repo_url, wc_dir,
-    '--username', 'jrandom', '--password', 'rayjandom',
+    '--force', '--username', 'jrandom', '--password', 'rayjandom',
     '--config-dir', config_dir)
=20
   exit_code, output, errput =3D svntest.main.run_command(
     svntest.main.svn_binary, None, True, 'co', sbox.repo_url, wc_dir,
-    '--username', 'jrandom', '--non-interactive', '--config-dir', config_d=
ir)
+    '--force', '--username', 'jrandom', '--non-interactive', '--config-dir=
', config_dir)
=20
   # Checkout with jconstant
   exit_code, output, errput =3D svntest.main.run_command(
     svntest.main.svn_binary, None, True, 'co', sbox.repo_url, wc_dir,
-    '--username', 'jconstant', '--password', 'rayjandom',
+    '--force', '--username', 'jconstant', '--password', 'rayjandom',
     '--config-dir', config_dir)
=20
   exit_code, output, errput =3D svntest.main.run_command(
     svntest.main.svn_binary, None, True, 'co', sbox.repo_url, wc_dir,
-    '--username', 'jconstant', '--non-interactive',
+    '--force', '--username', 'jconstant', '--non-interactive',
     '--config-dir', config_dir)
=20
   # Checkout with jrandom which should fail since we do not provide
@@ -2569,7 +2569,7 @@ def basic_auth_test(sbox):
   expected_err =3D ["authorization failed: Could not authenticate to serve=
r:"]
   exit_code, output, errput =3D svntest.main.run_command(
     svntest.main.svn_binary, expected_err, True, 'co', sbox.repo_url, wc_d=
ir,
-    '--username', 'jrandom', '--non-interactive', '--config-dir', config_d=
ir)
+    '--force', '--username', 'jrandom', '--non-interactive', '--config-dir=
', config_dir)
=20
 def basic_add_svn_format_file(sbox):
   'test add --parents .svn/format'
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=3DFalse, add_unve=
rsioned=3DFalse):
 #----------------------------------------------------------------------
=20
 def checkout_with_obstructions(sbox):
-  """co with obstructions conflicts without --force"""
+  """obstructed co without --force"""
=20
   make_local_tree(sbox, False, False)
=20
-  #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 =3D sbox.wc_dir
   url =3D sbox.repo_url
=20
   # Checkout with unversioned obstructions causes tree conflicts.
   # svn co url wc_dir
-  expected_output =3D svntest.wc.State(wc_dir, {
-    'iota'              : Item(status=3D'  ', treeconflict=3D'C'),
-    'A'                 : Item(status=3D'  ', treeconflict=3D'C'),
-    # And the updates below the tree conflict
-    'A/D'               : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/gamma'         : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/G'             : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/G/rho'         : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/G/pi'          : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/G/tau'         : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/H'             : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/H/chi'         : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/H/omega'       : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/D/H/psi'         : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/B'               : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/B/E'             : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/B/E/beta'        : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/B/E/alpha'       : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/B/F'             : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/B/lambda'        : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/C'               : Item(status=3D'  ', treeconflict=3D'A'),
-    'A/mu'              : Item(status=3D'  ', treeconflict=3D'A'),
-  })
+  expected_err =3D ".*Rejecting checkout of non-empty repository into non-=
empty directory.*"
=20
   expected_disk =3D 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 =3D actions.get_virginal_state(wc_dir, 1)
-  # A and iota are tree conflicted and obstructed
-  expected_status.tweak('A', 'iota', status=3D'D ', wc_rev=3D1,
-                        treeconflict=3D'C')
-
-  expected_status.tweak('A/D', 'A/D/G', 'A/D/G/rho', 'A/D/G/pi', 'A/D/G/ta=
u',
-    '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=3D'D ')
-  # A/mu exists on disk, but is deleted
-  expected_status.tweak('A/mu', status=3D'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 =3D svntest.wc.State(wc_dir, {
-  })
-
-  expected_disk =3D svntest.main.greek_state.copy()
-
-  expected_status =3D actions.get_virginal_state(wc_dir, 1)
-
-  actions.run_and_verify_update(wc_dir, expected_output, expected_disk,
-                                expected_status,)
-
=20
+  actions.run_and_verify_svn(None, expected_err, "co", url, wc_dir)
=20
 #----------------------------------------------------------------------
=20
diff --git a/subversion/tests/cmdline/svntest/actions.py b/subversion/tests=
/cmdline/svntest/actions.py
index ef6e2dd..fb91e34 100644
--- a/subversion/tests/cmdline/svntest/actions.py
+++ b/subversion/tests/cmdline/svntest/actions.py
@@ -548,7 +548,7 @@ def run_and_verify_checkout(URL, wc_dir_name, output_tr=
ee, disk_tree,
   ### repositories with no auth/auth requirements
   exit_code, output, errput =3D run_and_verify_svn(None, expected_stderr,
                                                  'co', URL, wc_dir_name,
-                                                 *args)
+                                                 '--force', *args)
   actual =3D tree.build_tree_from_checkout(output)
=20
   # Verify actual output against expected output.
diff --git a/subversion/tests/cmdline/tree_conflict_tests.py b/subversion/t=
ests/cmdline/tree_conflict_tests.py
index 84a78fa..324d1f6 100755
--- a/subversion/tests/cmdline/tree_conflict_tests.py
+++ b/subversion/tests/cmdline/tree_conflict_tests.py
@@ -418,7 +418,7 @@ def ensure_tree_conflict(sbox, operation,
       head_rev +=3D 1
       target_start_rev =3D head_rev
=20
-    main.run_svn(None, 'checkout', '-r', str(target_start_rev), sbox.repo_=
url,
+    main.run_svn(None, 'checkout', '--force', '-r', str(target_start_rev),=
 sbox.repo_url,
                  wc_dir)
=20
     saved_cwd =3D os.getcwd()

--2oS5YaxWCcQjTEyO--

--LpQ9ahxlCli8rRTG
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIcBAABCAAGBQJYBhP/AAoJEBF8Z7aeq/Es1+8P/i+7SM1jfDitduw+2PIpLcwW
UfEvBjpMgfdqW/q9jddnSuyw3P9dgDtqLaH14N72kOuU+DrMMIRHNd4Vcw8wjlwD
QSM5dCK4+b3NrO63vqneiH+Diq+MQvqA8AXsSgbceAZbEMXowwzGQ3CHbkkzXT8i
05WQpbKUGDLfYuZ2V1SLHT448uXS2kKqglAM0fvwCfyJcL+yAnYm6pQLH/YMkZiT
sFQsnR5oHBMwWHhV0gZ0DSDYdgQyoOsDvkbuFF66SVKc3qa38TLoUyvWFTZEagLI
pw1qA2FhWCUBDCzW7TtmZMubz9RJtLu4b0ztWLvNVJrI4sMLFlyWv8esPfLrIBQD
nSsL/ombRsxOwPPEbk/sQLPYmpTZBu8GBNiPjh1G1w/UBdohM8FU6MuZuOyNBXWu
vrUxGkfcmgKT/D2AM7UQk+1wXELzOwrV22leZ5LD1WCjLi6AX8suyCdfAWLF1hW9
4+p6+inuWWCqtxhe/mDWnBotKLHzjPw1LFpr/rkIx6U9ekzAkRAnnMEuJPP9b8Qi
Y13bKQtWgMUaoVqAmsZydIhynexoh3aoFfFrXvXs7vu4htMLSJ8zjiuimGzKrV+e
X53qitPDrwuZNbPd+oyQDHVhsRYFly26i8neJplVdRBuHvBbU26delLuOePHKOZj
GaO6FT0oG+b/hvv8UXYW
=ls8b
-----END PGP SIGNATURE-----

--LpQ9ahxlCli8rRTG--