Re: [Monotone-devel] heads up: file system changes

[Zack Weinberg]

On Wed, Sep 23, 2009 at 4:30 AM, Stephen Leake
<address@hidden> wrote:
> Zack Weinberg <address@hidden> writes:
>
>> On Tue, Sep 22, 2009 at 1:55 AM, Stephen Leake >
>>> I'll look at it more later, but I suspect the simplest fix is to just
>>> move the original do_remove_recursive into win32/fs.cc.
>>
>> Yah, or you should be able to copy the unix version, which isn't very
>> unix specific.  You might need more make_accessible calls though.
>
> It turns out the fix to your code was simple; SHFileOperationA does not
> like '/' directory separators, and it returns non-zero when the path
> doesn't exist.

Ugh.  Well, thanks for fixing it.

> Side note: do_remove_recursive attempts to generate a nice error
> message, but it doesn't come out anywhere that I can see.

It's probably being eaten by Lua.  This is a long-standing general bug
which would not be hard to fix, but it's tedious and fiddly --
basically, all of the points where control transfers in or out of the
Lua interpreter need to convert between C++ and Lua exceptions, rather
than throwing them away.  Some, but not all, Lua exceptions are
supposed to be thrown away -- for instance, if a hook is not defined
-- so it isn't purely mechanical, either.

> I guess the only place raw pathname strings occur is inside
> do_remove_recursive, as it fetches file names from the OS?

I think that right now there is no other place that needs to read *and
process* arbitrary OS file names.  Many other places fetch file names
from the OS but can refuse to operate on pathnames that would be
invalid any_path objects.

> Why is do_remove in platform.hh? The unix implementation requires C90
> 'remove'. Are we assuming C90 is not available on Windows?

Yes (see Daniel's reply).  The platform-independent code assumes C90 semantics.

> But it would seem a better approach is to enhance any_path objects to
> support all OS-supported filenames.

That's how we keep people from checking in files that can't be checked
out again on some other platform (e.g. the file named "\" that the
skip_invalid_paths test creates) so we certainly do not want that for
file_path and bookkeeping_path.  I could see the argument for
system_path.  I think we would then want to templatify most of
file_io.cc rather than continuing to have it manipulate bare any_path
objects, so that each class of path got the appropriate checks.

Closely related question: what the hell is going on with
new_optimal_path?  AFAICT all users of that function ought to be using
system_path, full stop.

> Does C90 not deal with this in a portable way?

*hollow laughter*

zw