On Sat, Apr 16, 2011 at 10:17 AM, Johan Corveleyn <jcor...@gmail.com> wrote:
> Hi,
>
> Following discussion in [1], I tried to write a patch for issue #3702
> ("svn ren TODO todo" not work on windows). As mentioned by Bert in
> comment to the issue [2], we need to avoid letting 'svn move' convert
> the destination path to on-disk casing in this case, so that's what
> the below patch does.
>
> However, it seems that's not enough. I'm getting further, but now the
> move is blocked libsvn_client/copy.c. The error I'm getting now is:
>
> svn: E155007: Path 'C:\Temp\test\todo' is not a directory
>
> The problem is that, in copy.c#svn_client_move6, it first tries the
> move, but if the dst_path already exists (which comes down to an
> apr_stat inside copy.c#verify_wc_srcs_and_dsts), it tries the move
> again with moving the src_path to a child of (the presumed directory)
> dst_path:
>
> libsvn_client/copy.c: 2313
> [[[
> err = try_copy(sources, dst_path,
> TRUE /* is_move */,
> make_parents,
> FALSE,
> revprop_table,
> commit_callback, commit_baton,
> ctx,
> subpool);
>
> /* If the destination exists, try to move the sources as children of the
> destination. */
> if (move_as_child && err && (src_paths->nelts == 1)
> && (err->apr_err == SVN_ERR_ENTRY_EXISTS
> || err->apr_err == SVN_ERR_FS_ALREADY_EXISTS))
> {
> ...
> err = try_copy(sources,
> dst_is_url
> ? svn_path_url_add_component2(dst_path,
> src_basename, pool)
> : svn_dirent_join(dst_path, src_basename, pool),
> ...
> ]]]
>
> So, a fix in the client layer is needed as well. Any suggestions on
> how to best approach this?
>
> Right now, I'm thinking the fix should be in verify_wc_srcs_and_dsts
> (similar to the "special case" of my below patch for move-cmd.c):
> while checking each copy_pair, if the dst_path already exists, check
> the "true path" of dst_path (APR_FILEPATH_TRUENAME), and see if it's
> the same as the src_path. If so, don't return an already-exists-error.
> But I'm not sure if that's correct, may have side-effects, ...
In attachment is a second attempted patch that implements the above
suggestion: in copy.c, when verifying srcs_and_dsts, if a dst clashes
with an existing file, check if its truepath is the same as the src
(if so, let it go through). This uses some copy-pasted code from
opt.c, so maybe some refactoring is needed here ... Or another way to
pass both the truepath and the originally-requested-path.
After that the "svn move" went through, but the moved file was deleted
from the file system. I fixed that by setting keep_local to TRUE in
the call to svn_wc_delete4 in do_wc_to_wc_moves_with_locks2. At first
sight this seems ok to me, but I'm very much in unfamiliar territory,
so I'm not sure at all :-). See the comment in the patch for some
additional explanation.
Comments, review, ... very much appreciated.
With this patch, one can perform case-only renames on Windows.
BUT it raises some additional questions/issues:
- How to commit such a move? Committing the parent directory
recursively works fine, but if you try to specify only the move
targets (src and dst paths), commit runs into the same problem as what
I was trying to address here: both paths are converted to their
"truepath", which means only the added side is seen by commit (the
only part that's still on the filesystem):
C:\Temp\test4>svn mv foo2 Foo2
A Foo2
D foo2
C:\Temp\test4>svn commit -mtest foo2 Foo2
Adding Foo2
Committed revision 96322.
This is very undesirable, because after this commit this cannot be
checked out or updated anymore on a Windows client (case-clashing
files).
- In fact: the same problem holds true for other commands as well: how
to revert both sides of this move? Ok, one can revert in two steps ...
- Maybe a more general solution is needed, so all commands can
adequately see which path the user actually means? The "truepath"
corresponding to a given path (modulo case), or really the path in the
case given by the user? A shot in the dark: (1) first look in the
wc-db - if the path matches a path in the wc-db, accept it as is, else
(2) convert it to its truepath (path on the filesystem that matches
modulo case). Except for "svn move", as implemented in this patch ...
- Note that the above problem is already present now on trunk (without
my patch): since we can now represent case-clashing paths in the WC
even on a case-insensitive filesystem. (See Bert's example in [2],
"svn ren TODO todoQ; svn ren todoQ todo").
Cheers,
--
Johan
> [1] http://svn.haxx.se/dev/archive-2011-04/0232.shtml
> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=3702#desc6
Log message:
[[[
Allow case-only renames on case-insensitive filesystems (issue #3702).
* subversion/svn/move-cmd.c
(svn_cl__move): If the targets are paths, and there are exactly two, and
the normalized DST_PATH and SRC_PATH are equal, canonicalize the original
DST_PATH again, but without converting it to on-disk casing.
* subversion/libsvn_client/copy.c
(do_wc_to_wc_moves_with_locks2): Pass TRUE for keep_local when calling
svn_wc_delete4 after just having performed the svn_io_file_rename. Because
it has just been renamed, the src_abspath_or_url should already be gone.
This avoids deleting the destination in case of a case-only rename.
(verify_wc_srcs_and_dsts): When the dst_abspath_or_url already exists,
see if its "truepath" (matching path on the filesystem, module case) is
the same as the src_abspath_or_url, to account for a possible case-only
rename. In that case, don't complain about that the dst already exists.
]]]
Index: subversion/svn/move-cmd.c
===================================================================
--- subversion/svn/move-cmd.c (revision 1092816)
+++ subversion/svn/move-cmd.c (working copy)
@@ -30,6 +30,7 @@
#include "svn_client.h"
#include "svn_error.h"
#include "svn_path.h"
+#include "svn_utf.h"
#include "cl.h"
#include "svn_private_config.h"
@@ -76,6 +77,27 @@ svn_cl__move(apr_getopt_t *os,
(SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE, NULL,
_("Local, non-commit operations do not take a log message "
"or revision properties"));
+
+ /* For allowing case-only renames on Windows (issue #3702):
+ If DST_PATH is a path, and there is exactly one source path, check if
+ the normalized DST_PATH and SRC_PATH are equal. If so, canonicalize
+ the original DST_PATH again, but without converting it to on-disk
+ casing, so we can pass the intended destination to
+ svn_client_move6. */
+ if (targets->nelts == 1)
+ {
+ const char *src_path = APR_ARRAY_IDX(targets, targets->nelts - 1,
+ const char *);
+
+ if (strcmp(src_path, dst_path) == 0)
+ {
+ const char *utf8_target;
+
+ SVN_ERR(svn_utf_cstring_to_utf8(&utf8_target,
+ os->argv[os->argc - 1], pool));
+ dst_path = svn_dirent_canonicalize(utf8_target, pool);
+ }
+ }
}
if (ctx->log_msg_func3)
Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c (revision 1092816)
+++ subversion/libsvn_client/copy.c (working copy)
@@ -347,8 +347,14 @@ do_wc_to_wc_moves_with_locks2(void *baton,
SVN_ERR(svn_io_file_rename(b->pair->src_abspath_or_url, dst_abspath,
scratch_pool));
+ /* ### JC: Deleting with keep_local==TRUE should be fine, because the above
+ rename should have already (re)moved src_abspath_or_url, so it should no
+ longer be present on the filesystem. Without keep_local, a case-only
+ rename on a case-insensitive filesystem doesn't work: the dst of the
+ rename is deleted here (because it's equal to the src, modulo case). */
SVN_ERR(svn_wc_delete4(b->ctx->wc_ctx, b->pair->src_abspath_or_url,
- FALSE, FALSE,
+ TRUE /* keep_local */,
+ FALSE /* delete_unversioned_target */,
b->ctx->cancel_func, b->ctx->cancel_baton,
b->ctx->notify_func2, b->ctx->notify_baton2,
scratch_pool));
@@ -482,11 +488,46 @@ verify_wc_srcs_and_dsts(const apr_array_header_t *
SVN_ERR(svn_io_check_path(pair->dst_abspath_or_url, &dst_kind,
iterpool));
if (dst_kind != svn_node_none)
- return svn_error_createf(
- SVN_ERR_ENTRY_EXISTS, NULL,
- _("Path '%s' already exists"),
- svn_dirent_local_style(pair->dst_abspath_or_url, pool));
+ {
+ /* ### JC: Get the dst's truepath, just in case we passed a
+ non-truepath dst for doing a case-only rename, so we can check
+ here if we're doing a case-only rename (which shouldn't be
+ blocked here). This code is mainly copy-pasted from
+ libsvn_subr/opt.c#svn_opt__arg_canonicalize_path - should
+ probably end up somewhere else ... */
+ const char *apr_target;
+ const char *dst_truepath;
+ const char *dst_truepath_utf8;
+ apr_status_t apr_err;
+
+ SVN_ERR(svn_path_cstring_from_utf8(&apr_target,
+ pair->dst_abspath_or_url,
+ iterpool));
+ apr_err = apr_filepath_merge(&dst_truepath, "", apr_target,
+ APR_FILEPATH_TRUENAME, iterpool);
+ /* convert back to UTF-8. */
+ SVN_ERR(svn_path_cstring_to_utf8(&dst_truepath_utf8,
+ dst_truepath, iterpool));
+ dst_truepath_utf8 = svn_dirent_canonicalize(dst_truepath_utf8,
+ iterpool);
+ if (dst_kind == pair->src_kind
+ && strcmp(pair->src_abspath_or_url,
+ dst_truepath_utf8) == 0
+ && strcmp(pair->src_abspath_or_url,
+ pair->dst_abspath_or_url) != 0)
+ {
+ /* It's a case-only rename, just let it go through */
+ }
+ else
+ {
+ return svn_error_createf(
+ SVN_ERR_ENTRY_EXISTS, NULL,
+ _("Path '%s' already exists"),
+ svn_dirent_local_style(pair->dst_abspath_or_url, pool));
+ }
+ }
+
svn_dirent_split(&pair->dst_parent_abspath, &pair->base_name,
pair->dst_abspath_or_url, pool);
