RE: How to fix issue #4023 (on Windows, 'svn rm missingItem' deletes the unversioned 'MissingItem' from disk)?

This introduces the same problem as reported earlier this week (copy
gives error on invalid casing) when the casing of the wcroot doesn't
match but the suffix after the root does match.

The other 'fix' was in svn, but this one can't be worked around at the
client level.

In this case I'm not sure if we should really fix it if it has any
unwanted side effects as a normal 'del fILE.eXT' would just delete
file.ext.

We can never 100% emulate a case sensitive filesytem on a case
insensitive one.

Bert Huijben (Cell phone)
From: Johan Corveleyn
Sent: 22-1-2012 2:48
To: Bert Huijben
Cc: Subversion Development
Subject: Re: How to fix issue #4023 (on Windows, 'svn rm missingItem'
deletes the unversioned 'MissingItem' from disk)?
On Tue, Jan 17, 2012 at 3:02 PM, Johan Corveleyn <jcorvel_at_gmail.com> wrote:
> On Sun, Jan 15, 2012 at 10:14 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Johan Corveleyn [mailto:jcorvel_at_gmail.com]
>>> Sent: zondag 15 januari 2012 21:26
>>> To: Subversion Development
>>> Subject: How to fix issue #4023 (on Windows, 'svn rm missingItem' deletes
>>> the unversioned 'MissingItem' from disk)?
>>>
>>> Hi,
>>>
>>> I'm looking into fixing issue #4023. I'd like to have some feedback on
>>> a possible solution, and some hints on how to best implement it.
>>>
>>> The problem
>>> -----------
>>> - 'svn rm missingItem' (while there is a 'MissingItem' on disk, and
>>> not a 'missingItem') will eventually call
>>> adm.ops.c#svn_wc__delete_many, with 'missingItem' as one of the
>>> targets (so far so good).
>>>
>>> - svn_wc__delete_many, after updating wc.db, will eventually call
>>> erase_unversioned_from_wc, which will call
>>> svn_io_remove_file2('missingItem').
>>>
>>> - At this point, we have a problem, because
>>> svn_io_remove_file2('missingItem') will eventually call the Windows
>>> API DeleteFileW, which will happily delete 'MissingItem' when given
>>> the 'missingItem' argument.
>>>
>>>
>>> Proposed solution
>>> -----------------
>>> Inside svn_wc__delete_many, we could find out that 'missingItem' is,
>>> you know, missing, before we try to delete it from disk. In that case,
>>> there is no need to call erase_unversioned_from_wc, so no call to
>>> DeleteFileW occurs.
>>>
>>>
>>> I can't come up with a better way, but if someone has other ideas, shoot.
>>>
>>> If this is indeed the way to proceed, then I'd appreciate a small
>>> hint/pointer: how do I find out (as cheaply as possible) whether the
>>> target in question is missing? In the beginning of
>>> svn_wc__delete_many, a call is done to svn_wc__db_read_info, but this
>>> doesn't seem to give this information (status == normal, kind == file,
>>> whether or not the file in question is missing from disk).
>>
>> The cheapest way to check would be (indirectly) calling the
>> apr_filepath_merge call to find the paths true name. If the basename of the
>> returned path is (case-)different than the path you entered, then the file
>> would be missing.
>> If the file is really missing, you would get the identical path or an error
>> from that call.
>
> Thanks. I'll look into that.
>
> I'll try to complete it during this week, but these are busy times, so
> if someone beats me to it: no problem.
>
>> On Windows this is cheaper than retrieving all directory entries in the
>> parent directory and checking yourself and on other platforms getting the
>> truename is a no-op.
>>
>>
>> We might have a wrapper for that apr_filepath_merge, for handling the path
>> encoding conversion.
>> If we have, then we probably used it for the file move fix we programmed in
>> Berlin. If not, then we should probably add one :)
>
> Yes, apr_filepath_merge (with APR_FILEPATH_TRUENAME flag) is wrapped
> inside svn_opt__arg_canonicalize_path. Am I allowed to call this
> function from inside svn_wc__delete_many? In that case, I can just
> call that function and compare its output with the output of
> svn_dirent_internal_style, just like we did for the 'case-only move'
> code.
>
> Actually, during the Berlin hackathon last year, we fixed the
> case-only moves without adding another call to
> svn_opt__arg_canonicalize_path. Instead, we did it by inserting some
> additional logic inside cmdline.c#svn_client_args_to_target_array,
> where the truepath conversion is done anyway, for all the path
> arguments. At that point we already had knowledge of both the original
> form and the truepath-converted form, so we could compare the two.
> (see r1124469 and r1131326)

Hi Bert,

Ok, the following patch fixes issue #4023 (and passes make check). But
I'm not sure if it's a "good fix" (more below). So I'd like some extra
eyes if possible ...

[[[
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 1233695)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -59,6 +59,7 @@
 #include "svn_private_config.h"
 #include "private/svn_io_private.h"
 #include "private/svn_wc_private.h"
+#include "private/svn_opt_private.h"

@@ -742,14 +743,29 @@ svn_wc__delete_many(svn_wc_context_t *wc_ctx,
    * unversioned. */
   if (!keep_local)
     {
+ const char *local_truepath;
+
       for (i = 0; i < versioned_targets->nelts; i++)
         {
           svn_pool_clear(iterpool);

           local_abspath = APR_ARRAY_IDX(versioned_targets, i, const char *);
- SVN_ERR(erase_unversioned_from_wc(local_abspath, TRUE,
- cancel_func, cancel_baton,
- iterpool));
+
+ /* Canonicalization will, on Windows, return the 'truepath' of the
+ given target (the actual casing on disk corresponding to the
+ given path). If this differs from the original target, we know
+ that the latter is missing from disk, and another case-clashing
+ file is present. In that case, we avoid the call to
+ erase_unversioned_from_wc (which would otherwise erase the wrong
+ file from disk). See also issue #4023. */
+ SVN_ERR(svn_opt__arg_canonicalize_path(&local_truepath,
+ local_abspath, iterpool));
+ if (strcmp(local_abspath, local_truepath) != 0)
+ continue;
+ else
+ SVN_ERR(erase_unversioned_from_wc(local_abspath, TRUE,
+ cancel_func, cancel_baton,
+ iterpool));
         }
     }

]]]

Some things on my mind:

- Is it ok to call svn_opt__arg_canonicalize_path from libsvn_wc, or
is this some kind of layering violation? (if so, we'd have to write
another wrapper for apr_filepath_merge (if so, where should we put
this?))

- It can only detect real 'case-clashes' (which is sufficient to fix
#4023), not 'missing' in general (like we discussed above). That's
because, going through svn_opt__arg_canonicalize_path, one cannot see
whether apr_filepath_merge returned an error, or just canonicalized to
the identical path (meaning the exact path was present). Both will
return the given path as is. OTOH, this is not really a problem, it's
enough to get #4023 fixed ...

-- 
Johan