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

On Wed, Jan 25, 2012 at 10:29 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Johan Corveleyn [mailto:jcorvel_at_gmail.com]
>> Sent: woensdag 25 januari 2012 1:48
>> To: Julian Foad
>> Cc: Subversion Development; Bert Huijben; Philip Martin
>> Subject: Re: How to fix issue #4023 (on Windows, 'svn rm missingItem'
>> deletes the unversioned 'MissingItem' from disk)?
>>
>> On Tue, Jan 24, 2012 at 1:18 AM, Johan Corveleyn <jcorvel_at_gmail.com>
>> wrote:
>>
>> [ ... ]
>>
>> > I'd like to note though that #4023 is also potentially an issue for
>> > svn < 1.7 (pre-wcng). At least if you're thinking about
>> > non-commandline usage. As of #3865, one can now also run into this
>> > situation with the commandline client, because you're now able to
>> > successfully invoke 'svn rm foo' while foo is missing and unversioned
>> > FOO is in the way. But if you're talking directly to the client layer,
>> > it's easy to pass 'foo', and the current code will then happily remove
>> > foo from wc.db, and FOO from disk. There is no need for any of my
>> > "fixes" to do that.
>>
>> Just to confirm this point: I've been able to reproduce #4023 (or a
>> variation thereof) with TortoiseSVN 1.6:
>>
>> - Add 'foo' as a versioned file. Commit.
>> - Delete 'foo' from filesystem (non-svn delete).
>> - Create new file 'FOO'.
>> - Right-click and TSVN -> Add
>> - You get a warning that another item which only differs in case
>> already exists. Click 'Add the item anyway'.
>> - Right-click in the directory, and choose 'Check for modifications'
>> (i.e. 'svn status')
>> - Now you can see foo as missing, and FOO as added.
>> - Right-click foo, and choose Delete. Now FOO is deleted from disk.
>>
>> I'll try to update the issue later tonight, to remove the 'since 1.7'
>> denomination, and rephrase it independently of the use of 'svn' (the
>> commandline executable). It's a libsvn_client or libsvn_wc issue.
>>
>> So my conclusion is: libsvn_client / libsvn_wc shouldn't assume that
>> they always receive a truepath-canoncalized local path. They can
>> always receive a path that's a valid wc-item, but is not present on
>> disk and is case-shadowed by another item on disk [1]. This means that
>> they should be very careful when calling filesystem API's where they
>> don't care whether the exact same file exists or not (in casu:
>> 'delete'): better to check whether the file (with the same exact case)
>> exists, before calling delete, to avoid accidentally deleting the
>> wrong file.
>
> libsvn_wc never used true path assumptions, except that it assumed that the
> caller already did all the work, which is usually 'svn'.

IMO, the caller still does all the work, except that the caller must
be able to do things to wc-items which are case-shadowed. If it can't,
well, let's just revert case-only renames on Windows then.

> The 'svn rm' case that opened this thread was the first case where libsvn_wc
> would be involved and this opens the door to many other changes.
> If we change svn rm, then it no longer matches 'svn revert' for these cases.
>
> Where do we stop?
>
> Do we break all shell scripts and library userw that assume the current
> behavior on the way?

I'm sorry, but this is clearly a bug, not a feature. If a script makes
use of this, it's already broken.

>
> Julian made a good point in asking for a design on how we should handle
> this.
> We don't want to go the same way as with the file externals implementation:
> exceptions everywhere.

Agreed that someone needs to make a design. Just the amount of
discussion in this thread warrants some decent design documentation.

> The fix for allowing multiple path forms from 'svn' was a very local change.
> Changing 'svn rm' scemantics in libsvn_wc is not. This has impact on the
> entire client library (E.g. svn merge).
>
> Just case normalizing everywhere will make Subversion at least 5* slower on
> Windows than on other platforms in the normal cases where the user didn't
> care before 1.7.

Ok, but we don't need to case-normalize everywhere. I'm perhaps naïve,
but it's definitely limited to places where we call I/O API's without
caring if the file really exists or not (in this case: calling
'delete' on a file, it doesn't matter if it exists or not, so I'm not
going to check, so I might as well just call 'delete' immediately).

> I don't think this 'svn rm wROngCased' is such a common case that we should
> use it to go that way. At least not without a proper design.
>
> I was never bitten by this problem. How many users were?

Ok, it's definitely an edge case. Only been reported by 1 user on the
users-list.

> If some users are surprised by the current behavior that is not necessary a
> reason to change the implementation if there are good reasons for the
> current design.

Ok, we'll leave #4023 as an open issue / known problem then. No problem for me.

-- 
Johan