gmane.comp.version-control.monotone.devel

Re: Filesystem normalisation

Stephen Leake — 2008-12-01T14:38:16

Ok. Yes, there are case sensitivity issues on Windows. As I understand it, your patch provides a hook that lets a user function rewrite a file path. So the hook would have to handle that case. I didn't see the actual hook code in your attachment; did I miss it somewhere? Yes, sorry for the wrong name. Ah; on __APPLE__ it does nothing. But if you put the appropriate code there, would that solve your problem? That should be simpler than adding a hook. normalize_external_path is the inverse (approximately) of as_external. So it should contain the inverse path-conversion code for APPLE. Thanks for the clear example. I agree it would be _very_ nice if mtn gave some sort of error here. I gather your proposed hook would change "APPLE" to "apple" so the paths would collide, and mtn would say something like "'apple' already accounted for" on the second file. One way to make that happen is for mtn normalized paths to be all lowercase on case-insensitive file systems. However, I prefer case-insensitive but case-preserving tools, so mtn would have to store both the original user path, and the normalized path. That's a major complication to the current code. Another way is to always to path comparisons in a case-insensitive way, on case-insensitive file systems. That preserves the user case. Well, in the first case, it mentions both "APPLE" and "apple"; how would you make that better? In the second case, the it should mention "APPLE" in the error message. One way to make this better is to extend the new 'conflicts' commands to handle update conflicts as well as merge conflicts. I do think it's a problem. It is not at all clear how to fix it. For example, I can easily have several different file systems accessible to mtn, via NFS. So just knowing I'm on Windows doesn't tell me the case sensitivity of the file system. It would probably be a close approximation on most projects. The general "fix" is to not commit files that would conflict on any filesystem that they might be checked out on. Each project needs to determine the set of acceptable file systems, and thus acceptable file names. Currently, enforcing that set of acceptable file names is a manual process; it would be nice if mtn provided tools to enforce it. Providing a user hook that returns the case sensitivity of a path would work, if the internal mtn code handled case sensitivity correctly (which it doesn't now). Your hook goes beyond that by allowing an arbitrary conversion for any path. That is clearly more general. If we provide an example hook that works for Mac OSX and Windows filesystems, it's probably a good idea. The inverse conversion also needs to be available as another hook. That may require storing a translation table or some other internal state. The hooks need to be called from the right places in the code. I think that's in 'as_external' and 'normalize_filename', but we'd have to do a thorough review to be sure. I appologize for that. I could not figure out what you were trying to accomplish, since I didn't know what the "mtn add apple APPLE" problem was. I suspect the hostility you detect is just my frustration. For a test that you won't commit, you can add fs.cc to MOST_SOURCES in Makefile.am. But since this file is currently MAC OSX specific, to be fully correct you'll need to add a new variable MAXOSX_PLATFORM_SOURCES in Makefile.am, and use it in a way similar to WIN32_PLATFORM_SOURCES. The linker argument should go in mtn_LDFLAGS in Makefile.am (I think). If it's a MACOSX specific argument, that would need to be in an 'if'. Currently the code uses "__APPLE__" as a synonym for MACOSX. Since there are other operating systems for Apple hardware (and probably will be still more in the future), I think MACOSX is better. That would be a good first step. Then we can try to extend the test cases to the situations identified in the other posts on this thread, and experiment with moving the hook into 'as_external'. If you want to commit experimental code, we'll need a branch.

Re: Filesystem normalisation

Nathaniel Smith — 2008-12-01T10:10:25

This isn't about bad paths, just about figuring out which actual path is meant when a user gives us some string. As such, it doesn't actually belong in args_to_paths, but it does belong in file_path_from_external (or whatever the exact name is), which is the code that's actually responsible for taking user-provided strings and turning them into well-formed internal paths. (args_to_paths is just a thin wrapper around it.) There are other issues with filenames that collide on some systems and not on others, but this isn't about that in particular; even if we get better tools for handling such cases, we still want to prevent them from arising accidentally, and that's what this code is supposed to do. (Plus you can't solve the hard problems without the primitive "hey OS what path do *you* think this refers to?" operation, and so this gets us closer by giving us code to that on at least OS X.) By all means, Peter, send a patch.

Re: Filesystem normalisation

Derek Scherger — 2008-12-01T04:18:24

_______________________________________________ Monotone-devel mailing list Monotone-devel< at >nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel

Re: Filesystem normalisation

Peter Stirling — 2008-12-01T01:14:13

On 29 Nov 2008, at 11:48 am, Stephen Leake wrote: It is insensitive by default, and a number of programs won't work if you set it to case-sensitive, so the vast majority of people will not set it that way. According to http://www.venge.net/mtn-wiki/CaseInsensitiveFilesystems case insensitivity is also an issue on windows. Also, HFS+ stores files in NFD utf-16 (it makes case insensitive sorting/searching much quicker), this means that you will also have issues with filenames entered as precomposed unicode (and my patch covers that case too). grep -r to_external . returns nothing, I assume that you meant string any_path::as_external() const { #ifdef __APPLE__ // on OS X paths for the filesystem/kernel are UTF-8 encoded, regardless of // locale. return data; #else // on normal systems we actually have some work to do, alas. // not much, though, because utf8_to_system_string does all the hard work. // it is carefully optimized. do not screw it up. external out; utf8_to_system_strict(utf8(data), out); return out(); #endif } (paths.cc:639) Which is, as should be obvious, not all that useful. as_external() wasn't anywhere obvious in normalise_external_path(), so even if it did the right thing you could still add the same file several times. To demonstrate what I'm trying to solve: # make a new scratch directory ############## g5:test_paths peter$ mkdir temp g5:test_paths peter$ cd temp # create a new db and workspace ############## g5:temp peter$ mtn db init --db=temp.mtn g5:temp peter$ mtn setup --db=temp.mtn --branch=temp temp g5:temp peter$ cd temp # create a file add it to the workspace twice with # the same name and verify that both names refer to the same contents ############## g5:temp peter$ echo foo > apple g5:temp peter$ mtn add apple APPLE mtn: adding APPLE to workspace manifest mtn: adding apple to workspace manifest g5:temp peter$ mtn diff # # old_revision [] # # add_dir "" # # add_file "APPLE" # content [f1d2d2f924e986ac86fdf7b36c94bcdf32beec15] # # add_file "apple" # content [f1d2d2f924e986ac86fdf7b36c94bcdf32beec15] # ============================================================ --- APPLEf1d2d2f924e986ac86fdf7b36c94bcdf32beec15 +++ APPLEf1d2d2f924e986ac86fdf7b36c94bcdf32beec15 < at >< at > -0,0 +1 < at >< at > +foo ============================================================ --- applef1d2d2f924e986ac86fdf7b36c94bcdf32beec15 +++ applef1d2d2f924e986ac86fdf7b36c94bcdf32beec15 < at >< at > -0,0 +1 < at >< at > +foo g5:temp peter$ mtn commit -m 'temp' mtn: beginning commit on branch 'temp' mtn: committed revision c28046695cac17a722edf6a591d5f52e13d331b7 # create a dummy child revision to demonstrate that we can't update # to the old revision ############## g5:temp peter$ echo bar > apple g5:temp peter$ mtn diff # # old_revision [c28046695cac17a722edf6a591d5f52e13d331b7] # # patch "APPLE" # from [f1d2d2f924e986ac86fdf7b36c94bcdf32beec15] # to [e242ed3bffccdf271b7fbaf34ed72d089537b42f] # # patch "apple" # from [f1d2d2f924e986ac86fdf7b36c94bcdf32beec15] # to [e242ed3bffccdf271b7fbaf34ed72d089537b42f] # ============================================================ --- APPLEf1d2d2f924e986ac86fdf7b36c94bcdf32beec15 +++ APPLEe242ed3bffccdf271b7fbaf34ed72d089537b42f < at >< at > -1 +1 < at >< at > -foo +bar ============================================================ --- applef1d2d2f924e986ac86fdf7b36c94bcdf32beec15 +++ applee242ed3bffccdf271b7fbaf34ed72d089537b42f < at >< at > -1 +1 < at >< at > -foo +bar g5:temp peter$ mtn commit -m temp2 mtn: beginning commit on branch 'temp' mtn: committed revision 84ae9cb7bcfcd84343993a66f09a60629075922b g5:temp peter$ mtn update -r p: mtn: expanding selection 'p:' mtn: expanded to 'c28046695cac17a722edf6a591d5f52e13d331b7' mtn: selected update target c28046695cac17a722edf6a591d5f52e13d331b7 mtn: [left] 6d7859995aaff97d24086a456d0d9e5cc22c44d4 mtn: [right] c28046695cac17a722edf6a591d5f52e13d331b7 mtn: modifying APPLE mtn: error: content of file 'apple' has changed, not overwriting # since that didn't work, let's try checking it out into a new # workspace ############## g5:temp peter$ cd .. g5:temp peter$ mtn co --db=temp.mtn --branch=temp temp2 mtn: misuse: rename target 'apple' already exists # nuke from orbit ############## g5:temp peter$ cd .. g5:test_paths peter$ rm -r temp As you can read above, there's no hint of friendliness in the diagnostic And you don't think this is a problem? I didn't say that it did that, I said that it could be done later, if this code gets into the monotone tree. It seemed silly to build castles in the sky before obtaining a positive response. It may just have been a side-effect of the point-by-point examination, but your email came across as quite hostile. I'm only trying to improve a defect that affects OS X, which has the potential as a start toward windows too. To that end, can someone point me as to where I should change the build configuration scripts to add this file, and the required linker argument? I need that so that I can check whether it works (ie hook in the right place), and hopefully write some tests to show that it works.

Re: fatal: std::logic_error:paths.cc:728:invariant'I(!empty())' violated

Vaclav Haisman — 2008-11-30T23:09:42

Thanks. That fixed it for me. I actually have a key but I forgot to import it into the root account. -- VH