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

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)

-- 
Johan