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